-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Media onStatusUpdate maybe not running in Angular zone #1591
Comments
Yes I have to wrap code in the obstatusbupdate in zone.run otherwise no change detection as you say. Same in the onSuccess callback (and I have not tested but presume on error will have the same issue) |
|
My response from the forum:
|
onSuccess and onError only trigger once and would be better suited as promises rather than observables. onStatus could be implemented as an observable. But to be honest, the media plugin as it is now works just fine, it's simple to use just needs a note in the docs to wrap the return functions in ngzone.run() and everything is good. This might be a better long term solution but the benefits are minimal. |
I will defer to the eventual decision here, but cannot disagree more strongly with this. I see ionic-native as a way to ensure that app code never needs to explicitly deal with Angular zones. Especially if the eventual goal is to allow replacing Angular with other frameworks, it makes it all the more crucial that we do not encourage app code to ever manually interact with zones. |
Sure but I also think that stability is important here. The media plugin has had several changes and some breakages in the last few versions and I feel that keeping things stable for a while would be a good think for users. This change chould certainly be planned for the next major version and delivered with proper migration instructions and details on the breaking changes though. This would give the short term stability as well as serving the long term goal and providing a clearly documented upgrade path. |
According to this https://cordova.apache.org/docs/en/latest/reference/cordova-plugin-media/#parameters The success callback gets called when play, record or stop action has completed. I assume the error callback is called whenever an error has occurred in any of these functions. It's kinda tricky. Not sure why don't they just have callbacks for each method instead of using a shared one. I haven't used the plugin enough to figure out how these callbacks work. I'm just working with what I understand from their docs. I'll have to dig into their source code or test the plugin more to understand how things are supposed to work. |
Just looked at their source code.. Success and error callbacks are used multiple times. |
Success is only used once, it is called on completion of playback or recording. Error I know it is called if there are specific errors creating the media object or starting playback (ie network, unsupported file etc). I can't see any scenario where error would be called more than once as it's generally pretty terminal, but I could be wrong. Status is obviously called multiple times for any changes in playback or recording state. Anyway I am happy to test and give input on an updated implementation, I just feel that it's important to keep things stable and well documented. People seem to struggle a lot with file and media operations already with Cordova and they are some of the more common plugins so stability is an important thing for a happy user base :) |
From what I see in the source code, they are calling it it every time the media stops: https://github.com/apache/cordova-plugin-media/blob/master/www/Media.js#L209 I guess that's when |
@ihadeed ah you're right, I was thinking of a one time use of a media file but if someone is going to play the same media file multiple times then onSuccess will be called each time that playback/recording completes and onError could potentially be called on any play request if something goes wrong. I guess in that case an observable may make more sense. |
Please try let file = this.media.file('file.mp3');
file.onSuccess.subscribe(..);
file.onError.subscribe(...);
file.onStatusChange.subscribe(...); |
should be able to test it out tomorrow, will update you. |
@ghenry22 did you get a chance to test it? I'm releasing 4.0 soon and I would like to include this. If you haven't tested it that's fine, I'll run a test when I get a chance. |
I tried the following code but nothing is in my console. Did I do anything wrong?
|
did you call this.audio.play() at the end? You will need to call play before anything happens. Also this would be better asked in the ionic forums, this github issue is already closed so you are not going to get much response here. |
Seems like this the problem was in 4.0, in 4.1 it's working. Thanks! |
BREAKING CHANGE: the plugin's `create` method no longer takes callback functions. You must use the observables provided by the `MediaObject` instance. Refer to the updated documentation for more information. closes danielsogl#1591
[x] bug report
Current behavior:
Changes made to controller properties from within a
MediaOnStatusChange
callback are not reflected in templates.Expected behavior:
onStatusChange
handlers should be able to have their activity automatically reflected in templates.Other information:
See this forum thread for sample code illustrating the bug. Triggering change detection manually seems to work, and I suspect
zone.run()
would as well. I don't know if this is something that ionic-native shims can do for us or not.The text was updated successfully, but these errors were encountered: