nikclayton/android-squeezer — pull request #14 by kaaholst (2014-01-19 13:26:43+00:00)
Title: Bugsense

Fixes for the "beginBroadcast() called while already in a broadcast" crashes.

According to the documentation "Note that only one broadcast can be active at a time, so you must be sure to always call this from the same thread", all broadcasts should be done from the same thread.

While I haven't been able to trigger ANY crashes we do call beginBroadcast from both the listening thread and the main thread.

The first commit fixes the updatePlayerSubscriptionState method which was called from onResume/onPause. The method didn't actually do any broadcast, but called beginBroadcast to get the number of clients interested in second to second updates. Instead I keep track of the number of clients, so it's no longer necessary to call beginBroadcast.

While at it, I removed the requirement that scrobling turns on second to second updates, because we already receive (and handle) these events via the notifications enabled by the listen command. Mind you, it won't make any difference, because the now playing fragment is on any activity, and it always requires second to second updates, even if they are only used when the fragment is full height.

The second commit is more problematic. The user can change the active player (from the ui thread), which triggers a callback (onPlayerChanged) via beginBroadcast. While it seems more unlikely for this to happen at the same we receive an input event, eventually it will occur, so I guess it has to be fixed. The same applies to ConnectionState.setConnectionState which may be initiated by via user via disconnect, and by the listening thread when the socket disconects.

The solution in this commit, is to have the executor run all broadcasts. While this seems like a good design at first glimpse, because it frees up the listening thread to process more messages, multiple requests are now queued up in the executor where as previously in the input thread, essentially the same. Furthermore it complicates the code a bit, and introduces an overhead.