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

SwitchAudioHandler.playbackStream does not forward errors from inner audio handlers #986

Open
dropout opened this issue Jan 9, 2023 · 2 comments

Comments

@dropout
Copy link

dropout commented Jan 9, 2023

Documented behaviour

Updating the UI in response to state changes.

Actual behaviour

SwithAudioHandler.playbackState does not propagate errors. In a SwitchAudioHandler implementation the inner handlers cannot propagate errors with playbackState.addError because of the way of how SwitchAudioHandler forwards these events.

This is where the problem occures:

_playbackStateSubscription = inner.playbackState.listen(_playbackState.add);
_queueSubscription = inner.queue.listen(_queue.add);
_queueTitleSubscription = inner.queueTitle.listen(_queueTitle.add);
// XXX: This only works in one direction.
_mediaItemSubscription = inner.mediaItem.listen(_mediaItem.add);
_androidPlaybackInfoSubscription =
inner.androidPlaybackInfo.listen(_androidPlaybackInfo.add);
_ratingStyleSubscription = inner.ratingStyle.listen(_ratingStyle.add);
_customEventSubscription = inner.customEvent.listen(_customEvent.add);
_customStateSubscription = inner.customState.listen(_customState.add);

Minimal reproduction project

I thought I leave out a reproduction of the error since it's quite obvious from the code that errors of the underlying stream won't be propagated.

Reproduction steps

  1. create a basic SwitchAudioHandler implementation
  2. listen for errors on the implementation's .playbackState stream
  3. call playbackState.addError in an inner handler of SwitchAudioHandler
  4. error handlers on the implementation of SwitchAudioHandler are not triggered

Output of flutter doctor

[✓] Flutter (Channel stable, 3.3.9, on macOS 11.6.8 20G730 darwin-x64, locale en-HU)
[✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0)
[✓] Xcode - develop for iOS and macOS (Xcode 13.2.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2021.3)
[✓] IntelliJ IDEA Community Edition (version 2022.1.1)
[✓] VS Code (version 1.74.2)
[✓] Connected device (3 available)
[✓] HTTP Host Availability

• No issues found!

Devices exhibiting the bug

iOS Simulator 13.2

Suggestions to fix

  • .pipe would forward the errors too, but i'm not sure how to cancel it when inner handler is switching?
  • I'm not familiar with Rx Dart implementation, but I can look into the dart api for a resolution?
  • fix with boilerplate code?
_playbackStateSubscription = inner.playbackState.listen(_playbackState.add, onError: _playbackState.onError, onDone: _playbackState.onDone);

If you consider to fix this problem, I can create a pull request with a chosen resolution.

@ryanheise
Copy link
Owner

Yes, I can see what you mean, thanks for finding and reporting this. I would be happy to accept a pull request.

Your code snippet seems like it should work since it is the most similar to the way the current code works, since it returns the subscription object, but if you find an alternative way that satisfies the requirements, that should also be OK.

dropout added a commit to dropout/audio_service that referenced this issue Jan 10, 2023
dropout added a commit to dropout/audio_service that referenced this issue Jan 10, 2023
@dropout
Copy link
Author

dropout commented Jan 10, 2023

I've created the pull request with the resolution outlined above #987:

  • Add onError argument to listen methods
  • Does not add onDone because it's not relevant for SwitchAudioHandler's streams
  • Does not add cancelOnError boolean flag because cancel would make retries and further state propagation unusable
  • Added a unit test
  • Raised version number

I hope I did everything right - let me know if something is needed.

💎

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

No branches or pull requests

2 participants