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

fix: [Discussion] Add try-catch to avoid IllegalStateException when calling player.isPlaying on Android media player #1365

Closed
wants to merge 1 commit into from

Conversation

luanpotter
Copy link
Member

Description

Add try-catch to avoid IllegalStateException when calling player.isPlaying on Android media player

See issues like #1150 and #1331 for some examples.

For examples of this issue. Though sometimes the exception is thrown on other methods used on the wrong time.

This is definitely a band-aid solution. The goal of WrappedPlayer is to abstract away all the insane fragility of android's default media player by doing its own state control.

This could help reduce some errors, but might as well be hinding some async/thread/racing conditions and such.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs:, chore: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
  • I have updated/added relevant examples in example.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@luanpotter
Copy link
Member Author

This could also be addressed by: #1352

Though we should probably either:

  • find the issue and fix (probably involve adding some synchronized to some methods, I believe) - definitely harder
  • accept it and ignore the error and return false (this PR)

Though better logging could be a step towards the better option

@Gustl22
Copy link
Collaborator

Gustl22 commented Jan 1, 2023

Idk if it is also solved by #1352, which addresses crashes, when an event triggers playing a source (codec) which is not supported by Android (see #1260).
So stream events inside kotlin code should be catched to avoid crashing. It cannot be catched in dart via Platform exception as it is not triggered by a synchronous call. But it can be processed further via error stream.

On the other hand, if we have errors which can be catched via Platform exception as they are thrown during a synchronous call, and we should not catch it in the kotlin code.
The best would be, if we have a reproducible example, where we could reliably reproduce this issue. But I would not recommend to try / catch it here. May should wait for better logs or reproduction sample, if possible.

@Gustl22
Copy link
Collaborator

Gustl22 commented Feb 13, 2023

@luanpotter I dived a bit deeper and found the source of this error, fixed in #1425.
I came to the conclusion, that we should not try/catch anything and let the app crash and better fix the original bug, like in this case. So I also won't try catch the asynchronous calls, like I did in #1352.

But this should be solved now, hopefully :) and may can close this, if you think so, too :)

@luanpotter
Copy link
Member Author

@Gustl22 your PR makes this completely moot now. Thanks for taking care of it :)

@luanpotter luanpotter closed this Apr 3, 2023
@luanpotter luanpotter deleted the luan.isPlaying branch April 3, 2023 01:42
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

Successfully merging this pull request may close these issues.

None yet

2 participants