Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addVolumeListener callback doesn't have the same behaviour on Android and iOS #102

Open
tt-driver-apps opened this issue Jan 21, 2020 · 3 comments · May be fixed by #103
Open

addVolumeListener callback doesn't have the same behaviour on Android and iOS #102

tt-driver-apps opened this issue Jan 21, 2020 · 3 comments · May be fixed by #103

Comments

@tt-driver-apps
Copy link

It seems that the behaviour of the addVolumeListener callback is called differently from iOS and Android when the volume is set within the library.

On Android, when you set the volume with setVolume, listeners are unregistered:

    @ReactMethod
    public void setVolume(float val, ReadableMap config) {
        unregisterVolumeReceiver();
        ...
        registerVolumeReceiver();
    }

then registered again, when the volume has been updated, so the addVolumeListener callback is not called.

On iOS, there is not such things:

RCT_EXPORT_METHOD(setVolume:(float)val config:(NSDictionary *)config){
    dispatch_sync(dispatch_get_main_queue(), ^{
        id showUI = [config objectForKey:@"showUI"];
        [self showVolumeUI:(showUI != nil && [showUI boolValue])];
        volumeSlider.value = val;
    });
}

So addVolumeListener callback is called when you use setVolume.

I tested to just remove the unregisterVolumeReceiver(); and registerVolumeReceiver(); on Android, and it works like iOS. And that's the behaviour I would like to have.
But now I'm worried if there is something that I didn't see and can lead to memory leek, strange behaviour, hidden infinite loop, or performance issue?

It it's the case, it could be nice to update the iOS part and add this:

RCT_EXPORT_METHOD(setVolume:(float)val config:(NSDictionary *)config){
    __weak RCTSystemSetting *weakSelf = self;
    dispatch_sync(dispatch_get_main_queue(), ^{
        [weakSelf stopObserving];
        ...
        [weakSelf startObserving];
    });
}

Anyway, let me know if I can help and do a PR in either way. Because I think the best thing is to have the same behaviour between platforms right?

tt-driver-apps pushed a commit to tt-driver-apps/react-native-system-setting that referenced this issue Jan 27, 2020
* Fix SoundVolume addVolumeListener callback have the same behaviour on
Android and iOS
* Fix c19354837#102
* Fix c19354837/issues/102
@c19354837
Copy link
Owner

Yes, you make the right thing.

I add unregister & register to prevent the listener being called when we call the setVolume, and I must not test it in iOS.

Your code looks like great, while I cannot verify it now in my phone for some reason. I'll merge it ASAP.

@c19354837
Copy link
Owner

I've test your codes just now, and it still calls the volume listener when setVolume. You can reproduce it by my example, just slides the volume bar quickly.

Image that, We call setVolume twice. We hope the code run like:

stopObserving -> volumeChange(skip when stop) -> startObserving -> stopObserving -> volumeChange(skip when stop) -> startObserving.

While we call setVolume twice quickly. In rare case, it will be:

stopObserving -> stopObserving -> volumeChange(skip when stop) -> startObserving -> startObserving -> volumeChange(fire when start)

@c19354837 c19354837 reopened this Apr 12, 2020
@c19354837
Copy link
Owner

I have push my code to master, you can review it.

Is that what you hope?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants