nikclayton / android-squeezer

Android SqueezeCenter / SqueezeBox remote control client.
https://nikclayton.github.io/android-squeezer/
Apache License 2.0
100 stars 42 forks source link

Added home screen widget #642

Closed idontusenumbers closed 4 years ago

idontusenumbers commented 4 years ago

I'm trying to add a home screen widget: image

It asks which player group upon insert: image

Then it asks which action: image

I refactored the base activity so that I could reuse the player list code; it's not perfect.


This change is Reviewable

kaaholst commented 4 years ago

This is looking great, it's appreciated :-)

I think the reason instantiating SqueezeService doesn't work, is because unbindService is called before startForegroundService is called. SqueezerRemoteControl calls squeezeService.startConnect() which initiates an off thread connection. The result of the connection progress is communicated via ConnectionChanged events. SqueezeService calls startForeground on such an event. It then calls start[Foreground]Service to avoid getting destroyed by the system. (see Managing the lifecycle of a service). If it's unbound before start[Foreground]Service is called, it will be destroyed however. SqueezeService disconnects in it's onDestroy, which in turn calls stopForeground, which sets ongoingNotification to null. This explains some of the other issues you are experiencing. So the solution is to somehow synchronize SqueezerRemoteControl to call unbindService after startForeground is called and completed.

I managed to produce this which might help understanding the issue.

2020-05-28 23:39:49.659 9722-9757/? I/ConnectionState: setConnectionState(0 => 1) 2020-05-28 23:39:49.661 9722-9757/? I/SqueezeService: startForeground 2020-05-28 23:39:49.661 9722-9722/? I/ConnectionState: setConnectionState(1 => 0) 2020-05-28 23:39:49.661 9722-9722/? I/SqueezeService: stopForeground 2020-05-28 23:39:49.667 9722-9757/? E/Event: Could not dispatch event: class uk.org.ngo.squeezer.service.event.ConnectionChanged to subscribing class class uk.org.ngo.squeezer.service.SqueezeService java.lang.NullPointerException: Attempt to read from field 'java.lang.String uk.org.ngo.squeezer.service.NotificationState.songName' on a null object reference at uk.org.ngo.squeezer.service.SqueezeService$NotificationData.(SqueezeService.java:524) at uk.org.ngo.squeezer.service.SqueezeService$NotificationData.(SqueezeService.java:492) at uk.org.ngo.squeezer.service.SqueezeService.startForeground(SqueezeService.java:656) at uk.org.ngo.squeezer.service.SqueezeService.onEvent(SqueezeService.java:643) at java.lang.reflect.Method.invoke(Native Method)

The NoClassDefFoundError: Failed resolution of: ... stuff I'm afraid is my fault. I also see the exceptions on the develop branch (on API level 26 at least). I have recently changed to Material Themes and it's not all done.

About the missing icons you could probably figure this out by yourself, since you got this far, but here are some hints. To show the icon you need to move getModelIcon along with its dependencies to PlayerBaseView, add setViewParams(VIEW_PARAM_ICON) to the constructor in your extension in SqueezerRemoteControlConfigureActivity (convert anonymous to inner), and override bindview like this:

        @Override
        public void bindView(View view, Player item) {
            super.bindView(view, item);
            ViewHolder viewHolder = (ViewHolder) view.getTag();
            viewHolder.icon.setImageResource(getModelIcon(item.getModel()));
        }
idontusenumbers commented 4 years ago

Thank you for your feedback!

That bindView suggestion did the trick, that would have taken me forever to figure out. I also added the opacity change for the player power status.

idontusenumbers commented 4 years ago

Some missing features maybe for a later iteration:

I'd be curious to learn more about the intended API compatibility situation. There are a number of Java features an updated SDK would enable like lambdas; it would really help clean up a lot of the one method anonymous inner classes. I tried my best to stay within the current confines of the older API; only issue is those vector drawables I called out.

kaaholst commented 4 years ago

I ran the latest version of the home-widgets.

Overall it's looking fine and well functioning.

I have found two outstanding issues.

  1. unbindService is called to soon, so SqueezeService is stopped, and the requested button action is not performed. This can be reproduced by force stopping the Squeezer-app and then press a button on the home-screen-widget.
  2. Create a new home-screen-widget when Squeezer is disconnected (i.e. showing the disconnected activity), the configuration screen shows an empty players list.
kaaholst commented 4 years ago

Presets are currently not used/supported in Squeezer. I don't know how they work.

I think users expects the widget to reflect the live status of the controlled player (or indicate that it's not connected), so I think we eventually have to implement that.

The intended API compatibility status is to support as much devices as possible. I recently upgraded to min SDK 19, because jetty-client doesn't work on lower API's. It appears that with the latest build tool we can use Java 8 features without requiring a minimum API level. (see https://developer.android.com/studio/write/java8-support), so I upgraded, and now you are free to use lambdas etc. I have some other changes pending, which makes this pull request not mergeable. I have created a new branch homescreen-widget and cherry-picked the latest commits. You can change your pull request to target this branch, to avoid merge issues.

idontusenumbers commented 4 years ago

It looks like I was unbinding unconditionally right after setting up an async event handler; definitely a mistake. I'll adjust.

Which branch is the lambda support on? Develop looks unchanged.

One thing I wanted to avoid was the widgets holding on to a handle to the service (causing a lingering notification and possibly wasted CPU/Memory/energy). Is there a semaphore, latch, or similar that the service maintains so it can shut itself down when not needed? Is there an event I can intercept to know that the call was completed? Is a lazy 'just set a timer to unbind' a good option? I'm open to suggestions. I'll push up a fix for the early unbind and failure to start the service in the player selector screen in a moment.

I think I have a good handle on the presets. I dove into the perl code to figure it out and used squeezer breakpoints to confirm. They are enumerated in the status response from squeezebox and can be easily 'fire and forget' triggered with a fake remote control button send. In the end, I never needed to interrogate the status to determine the actual presets (except maybe to hide buttons for presets that aren't populated). I added all the necessary plumbing to a separate branch, it seems to be working pretty well.

ghost commented 4 years ago

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that. Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

kaaholst commented 4 years ago

Lambdas should be available on both the develop and homecreen-widget branches.

If you wish to remove the notification after the widget operation is done, you can call service.disconnect(). If you decide to do this, please only disconnect if the connection was done in the widget. Also note that live tracking is possible only with an open connection. Currently in Squeezer there is only manual disconnect, we have no timers or others to disconnect after a certain period of inactivity. I little background on the lingering notification is that a foreground service must display a notification (when targeting API level 26 or higher we are not allowed to use a background service). This is implemented is Squeezer so the service is in foreground when connected to a LMS.

The presets will be a good candidate for the main app also in a later release. I just checked the squeezeplay code, and they seem to use the "presetloop" from the playerstatus response to determine which presets are available, and send "preset" .. number .. '.single' when a preset button is pressed.

idontusenumbers commented 4 years ago

I'll skip disconnecting to hide the notification.

Herp derp, I was looking at my fork's develop branch and not the upstream. Lambda support works.

Would you like me to merge the preset stuff into this branch?

kaaholst commented 4 years ago

Yes, please go ahead and merge presets thank you.

idontusenumbers commented 4 years ago

Big update:

Any remaining issues?

kaaholst commented 4 years ago

Merged, thanks :-)

I noticed you updated this to the latest commits on the develop branch, so I merged directly to develop.

No outstanding issues, it's building and running and the code is very high quality.

Thanks again for the contribution.

idontusenumbers commented 4 years ago

Thanks for maintaining such a great client!