google / horologist

Horologist is a group of libraries that aim to supplement Wear OS developers with features that are commonly required by developers but not yet available.
https://google.github.io/horologist/
Apache License 2.0
566 stars 94 forks source link

The volume is not being controlled using onRotaryInputAccumulated #1017

Closed raghav2945 closed 1 year ago

raghav2945 commented 1 year ago

I am working with a music app Audiomack, and I created a controller app for wearable devices. I used your library, which is fantastic.

However, after upgrading to version 0.3.1 and testing on a real device, I found that the volume is not being controlled with the physical rotating key available on my watch. I used the code below to implement this feature.

val volumeState by volumeViewModel.volumeState.collectAsStateWithLifecycle()
    val onVolumeChangeByScroll = volumeViewModel::onVolumeChangeByScroll

    Scaffold(
        modifier = modifier
            .fillMaxSize()
            .onRotaryInputAccumulated(onValueChange = onVolumeChangeByScroll)
            .focusRequester(playerFocusRequester)
            .focusable(),
        positionIndicator = { VolumePositionIndicator(volumeState = { volumeState }) },
        timeText = { TimeText() }
    ) {
        PlayerScreen(
            playerViewModel = mediaPlayerScreenViewModel,
            volumeViewModel = volumeViewModel,
            buttons = {
                AMSettingButton(
                    modifier = modifier.size(ButtonDefaults.LargeButtonSize),
                    enabled = enabled
                )
            }
        )
    }
yschimke commented 1 year ago

Great to hear you are using this functionality, we'll take a look and if we can confirm the problem we should be able to advise or fix and release.

yschimke commented 1 year ago

Is your sample above contained in any other structure, such as a pager or navigation? Or is top of the activity?

laiyichin commented 1 year ago

I couldn't seem to reproduce the issue in our sample app in my physical device. Do you mind trying out the media-sample in your device and see if you have the same issue from the sample app?

Also, I am curious what device you are testing with? I am testing with Pixel Watch.

yschimke commented 1 year ago

@raghav2945 what's calling playerFocusRequester.requestFocus() for you?

raghav2945 commented 1 year ago

Is your sample above contained in any other structure, such as a pager or navigation? Or is top of the activity?

Yes it is on top of an activity

Also, I am curious what device you are testing with? I am testing with Pixel Watch.

I am also using the pixel watch

@raghav2945 what's calling playerFocusRequester.requestFocus() for you?

focusRequesterNodes.isNotEmpty() returns positive and I get below suggestion. Not sure how to implement it. Request your help


   FocusRequester is not initialized. Here are some possible fixes:

   1. Remember the FocusRequester: val focusRequester = remember { FocusRequester() }
   2. Did you forget to add a Modifier.focusRequester() ?
   3. Are you attempting to request focus during composition? Focus requests should be made in
   response to some event. Eg Modifier.clickable { focusRequester.requestFocus() }
yschimke commented 1 year ago

@raghav2945 I created a short self contained example here https://github.com/google/horologist/pull/1020

It seems to work against 0.3.1 for me.

Can you test if it works for you?

raghav2945 commented 1 year ago

Still not working for me with the above shared changes. Although the animation is visible when we scroll but there is no affect on the real volume out from the connected mobile phone. Let's share the video from both the phone and watch as a reference, so that we are all on the same page.

Watch Screen record

https://user-images.githubusercontent.com/4385611/219677366-3d030f4c-c1eb-4e68-8cc8-cc277854641b.mp4

Phone Screen record

https://user-images.githubusercontent.com/4385611/219677936-2a501376-016d-48ef-9b6d-669a72c23c0c.mp4

laiyichin commented 1 year ago

@raghav2945, If I am understanding it correctly, you are playing the media on phone, and would like to adjust the volume on phone from watch, right?

The media sample app is a native watch app and would adjust the volume on watch only (and the animation on the watch screen also shows that the volume is indeed changing when you rotated the crown).

If your goal to create a native watch app (such as the media sample or the example @yschimke showed) to control volume on your phone, you will need to override the volume changing events and send them across the device to the phone in the native watch app.

Otherwise, you can let the Media Controls on the watch manage the phone media session volume for you.

I hope I did not misunderstand your issue, please let me know if I misunderstood. Or maybe you can clarify a little bit more on how it was working before version 0.3.1 with a video to help us troubleshoot further.

raghav2945 commented 1 year ago

@laiyichin, I agree with you. I would like to be able to manage the media volume on my phone using my wearable device. Thank you for pointing out that overriding the volume change event may be necessary to accomplish this, since the default media control is not currently adjusting the volume as desired.

Due to the Java documentation of the VolumeRepository class stating that it is a state repository for audio volume, typically the system AudioManager, but possibly a remote app in paired situations, I believed that it is responsible for controlling the audio output, including that of a paired device. However, it appears that my interpretation was wrong.

Do you have an example of how to override the volume change event and transmit it to the phone? If you could offer some suggestions, it would be greatly appreciated.

yschimke commented 1 year ago

VolumeRepository is an interface, there is one non test implementation SystemAudioRepository, which is for the system AudioManager. For a remote app, you'll need another implementation, which isn't provided here.

theGeekyLad commented 1 year ago

I have run into the same issue. Looks like modifier. onRotaryInputAccumulated() is overridden inside the PlayerScreen composable thereby masking your event handler functionality. This makes sense as it sort of enforces developers to use the VolumeViewModel class that is used pretty much everywhere (even in the MediaPlayerScaffold) to manage volume.

It makes the most sense to "extend" this class and "override" its increaseVolume(), decreaseVolume(), setVolume(), and so on. The VolumeViewModel class is open for inheritance but turns out these functions are closed.

Therefore, in conclusion, the VolumeViewModel,

  1. Is pretty much the one-stop-shop for volume management
  2. Effectively closed for inheritance

What would you suggest in this case?

yschimke commented 1 year ago

VolumeViewModel takes a VolumeRepository, which is where it is expected you will change the implementation. It's designed to support any volume system you need.

a) by default local volume changes via SystemAudioRepository b) your own implementation that works as a remote control?

Alternatively, drop one level lower and use PlayerScreen composable without ViewModels, that doesn't apply the onRotaryInputAccumulated, so you'll have to do it yourself.

theGeekyLad commented 1 year ago

Ah I see. Turns out I too was thinking on similar lines. I managed to get it working through a custom VolumeRepository implementation for remote playback control.

Thanks @yschimke !