search

kaaholst / android-squeezer

Remote control for your Logitech Media Server ("Squeezeserver" etc) and players.
Apache License 2.0
75 stars 15 forks source link

Upgrading to Eventbus 3.1.0 #762

Closed viertelb closed 2 years ago

viertelb commented 2 years ago

This is working but has some issues:

I would rather merge this fast and fix (upcoming) issues later.

kaaholst commented 2 years ago

That's a lot of changed files, so I merged it to avoid to much merging. It's also good to get it in, so it can run for while to see if we encounter any issues. Yes I think the reverting slider is an existing issue, so this should not block this PR.

Well done!

kaaholst commented 2 years ago

Hi Benjamin,

I merged the upgrade of the EventBus library and found no issues so far. I looked into the TODO about the crash if all specialisations of onEventMainThread(HandshakeCompleted) are annotated. It appears that EventBus takes care of calling the specialization for the current object like in normal Java, so it's not necessary to annotate all specialisations.

I googled the exception found a couple of hits: https://github.com/greenrobot/EventBus/issues/574 https://github.com/greenrobot/EventBus/issues/539 where they recommend annotating the child/subclass. I don't understand that advice, because it's not well defined. In our case it would be PlayerListActivity, BaseListActivity, JiveItemListActivity and HomeActivity. It's much better to annotate the base class, so I did that and added some log statements, to verify that the correct methods are called, and it seems to work well. So I removed the TODO. The log statements also verified that the priority you set, works as intended, so I removed that TODO also. Consider this done, provided we don't run into issues with events.

While working with events, I noticed that if, for some reason the HomeMenuEvent comes before the ActivePlayerChanged, then the loading view would be shown. So I added an empty clearAndReOrderItems in HomeMenuActivity (clearAndReOrderItems is called on ActivePlayerChanged and shows the loading view). I think I remember you noticed that sometimes the loading view would be shown instead of the home menu after connection. When you have pulled the latest, please check if that still happens, and report back if it does.

Regards Kurt

https://github.com/greenrobot/EventBus/issues/574

Den tir. 22. mar. 2022 kl. 21.35 skrev viertelb @.***>:

This is working but has some issues:

  • There was one class which had a higher priority. I added this priority to the methods but I did not grasp why it has to be done or if it is usefull the way I did it.
  • I added one TODO for an issue where I could not add the annotation because it resulted in the app crashing upon start.
  • Moving the slider for a song forward or backward will result in the song playing correctly from the new position. But the view will revert to the old time stamp for a short instance before it will show the valid time. I think this was already an issue before the upgrade (or in the past) and it could be related to a sticky event that should be deleted. Or we might remove the stickyness.

I would rather merge this fast and fix (upcoming) issues later.

You can view, comment on, or merge this pull request online at:

https://github.com/kaaholst/android-squeezer/pull/762 Commit Summary

File Changes

(22 files https://github.com/kaaholst/android-squeezer/pull/762/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/kaaholst/android-squeezer/pull/762, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADISEDBGJRFEHFUGTRJQXLVBIVJZANCNFSM5RMAD6UA . You are receiving this because you are subscribed to this thread.Message ID: @.***>