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

feat: add durationUpdate callback #197

Merged
merged 4 commits into from
Apr 28, 2022
Merged

Conversation

BuddyLReno
Copy link
Contributor

@BuddyLReno BuddyLReno commented Nov 15, 2018

Platforms affected

  • Android
  • iOS

What does this PR do?

  • Add support for a durationUpdate callback.

This particular change helps deal with the fact that Android shuts down webapps in the background after a few minutes. You can subscribe to this callback and let the audio player report the duration to you when it's available instead of querying for it.

What testing has been done on this change?

Physical device testing on:

  • iPhone X (iOS 12)
  • iPhone 6 (iOS 11)
  • OnePlus 5T (Oreo 8.1)
  • Samsung Galaxy S8 (Nougat 7.1)

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@BuddyLReno

This comment has been minimized.

@janpio

This comment has been minimized.

@BuddyLReno

This comment has been minimized.

www/Media.js Show resolved Hide resolved
@janpio
Copy link
Member

janpio commented Nov 16, 2018

(Thanks for the updates on the text, I cleaned up the comments a bit by hiding.)

Interesting change. I understand both parts of what you wrote:

This particular change helps deal with the fact that Android shuts down webapps in the background after a few minutes.

You can subscribe to this callback and let the audio player report the duration to you when it's available instead of querying for it.

But to be honest I don't really get how those are connected. Can you elaborate maybe?

@BuddyLReno
Copy link
Contributor Author

Yes, I can elaborate.

In the newer versions of android, after about five minutes of background activity, android shuts down the web container that is running the app. So when you try to use a background task to query for the duration with a setTimeout as is described in the docs, that setTimeout never actually runs. This occurs even if you use a plugin like cordova-plugin-background-mode.

What I mean by subscribing, is basically just attaching a callback to allow the native code to run the callback when the audio player assigns duration. By doing this, you always get a real duration and don't have to worry about getting a "-1" duration as it only reports the duration after the audio player actually initializes and has the real duration. This removes the need to query for duration with a setTimeout, which prevents the situation above from happening.

Does that help?

@janpio
Copy link
Member

janpio commented Nov 16, 2018

I very much hoe so:
I understood that https://github.com/apache/cordova-plugin-media#quick-example-2 doesn't actually work that well any more and one should use this new callback param instead on Android, correct?

If so, best add this as an "Android Quirk" under the getDuration example in the README to make sure it gets noticed.

@BuddyLReno
Copy link
Contributor Author

Great! I'm getting ready to leave the office but I'll update the README later tonight after the kids go to bed. 👍

@BuddyLReno
Copy link
Contributor Author

@janpio ugh, i'm sorry this took until Monday to get to! My weekend was a lot busier than I had planned on. I've updated the README with the Android quirk requested! :)

README.md Outdated Show resolved Hide resolved
Co-Authored-By: BuddyLReno <buddy.reno@daveramsey.com>
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good 💯 💕

Would prefer a second review from someone that actually tested the functionality. Maybe a demo app on GitHub one can checkout to "demo" the functionality @BuddyLReno?

@BuddyLReno
Copy link
Contributor Author

Definitely. I'll write up a test app that utilizes this functionality and link it here.

@BuddyLReno
Copy link
Contributor Author

@janpio would it be alright if I created a test app using Ionic? I'm most familiar with that platform.

@janpio
Copy link
Member

janpio commented Nov 20, 2018

Sure, whatever you are comfortable with. Shouldn't be to hard to recreate with Cordova though later on. Maybe I can fork your app then and transform it to pure Cordova.

@BuddyLReno
Copy link
Contributor Author

@janpio here's the demo project! Let me know if you have issues getting it to install. https://github.com/BuddyLReno/cordova-plugin-media-duration-demo

@BuddyLReno
Copy link
Contributor Author

Once we get this pr merged in, I'm going to update the media dependency to use the public one.

@erisu erisu changed the title Add durationUpdate callback feat: add durationUpdate callback Apr 27, 2022
README.md Show resolved Hide resolved
@erisu erisu merged commit 177a56b into apache:master Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants