Kore icon indicating copy to clipboard operation
Kore copied to clipboard

Control volume with hardware buttons when Kore is not focused (#157)

Open caffeineFox opened this issue 3 years ago • 2 comments

Since MediaSession was implemented, I thought it'd be a good point to add volume control via the VolumeProviderCompat. This way, playback volume can be adjusted even when Kore is not in focus and/or the screen is locked (but turned on). To my mind, it resolves Issue #157 as far as possible for now.

caffeineFox avatar Aug 07 '22 16:08 caffeineFox

Thanks, and sorry for the delay in reviewing this.

Looks good, but a couple of comments:

  • There's a setting that defines whether the hardware volume keys should control Kodi's volume, and that should be respected. Take a look at Settings.java, look for KEY_PREF_USE_HARDWARE_VOLUME_KEYS and check how it's used in VolumeControllerDialogFragmentListener. I'll point out in the code where to check if the setting is active.
  • For consistency with the current code, please use spaces, with 4 spaces per level.

SyncedSynapse avatar Aug 14 '22 18:08 SyncedSynapse

Hi, thanks for the feedback! I'll add the changes in the next days :)

caffeineFox avatar Aug 14 '22 20:08 caffeineFox

Hi @SyncedSynapse I guess it's ready now :D I reindented the file and now support the user preference concerning hardware volume key usage

caffeineFox avatar Aug 15 '22 17:08 caffeineFox

Ok, thanks. Just one thing: Note that in the constructor for RemoteVolumeProviderCompat you're calling setCurrentVolume, and you're constructing it, before checking if the hardware volume keys are enabled. I don't think it's serious, but can you please consider this. I'll point out in the code.

SyncedSynapse avatar Aug 15 '22 18:08 SyncedSynapse

By the way, i'll probably be unavailable for the rest of the week, so i might not be able to merge this before next week.

SyncedSynapse avatar Aug 15 '22 18:08 SyncedSynapse

Thanks for pointing that out. To my mind, it was not relevant to check the preference there since setCurrentVolume solely affects the RemoteVolumeProviderCompat. I thought it was necessary to get the currently set volume on Kodi and set it to the Compat so the volume used as reference for display (in the Android volume slider) is in sync. After doing some explorative testing, it seems that everything works just fine without initial synchronization, so I removed the call and method.

caffeineFox avatar Aug 15 '22 20:08 caffeineFox

I didn't mind the syncing on the constructor, i just thought that you might only create remoteVolumePC after checking if the hardware volume keys were enabled, but this is ok also.

There's a slight bug: when you call any of the HostConnectionObserver.register* you need to call unregister somewhere (in this case in onDestroy), or it will keep a reference to the object, preventing GC, as hostConnectionObserver is a singleton.

I'll merge this as is and fix that later.

Thanks

SyncedSynapse avatar Aug 16 '22 10:08 SyncedSynapse