-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[REVERTED] [MediaElement] Removed line that pauses player before setting it to null. #9531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, the Pause does now seem redundant
39339d5
to
599294f
Compare
This is my first pull request, so I've almost certainly done something wrong on my UITest. Seeing as it is a simple fix on a new control, maybe best if the test is removed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just need to fix the test.
Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue9525.cs
Show resolved
Hide resolved
new MediaElement | ||
{ | ||
AutomationId = "Issue9525MediaElement", | ||
Source="https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a video that won't disappear on us. Something uploaded to the source, perhaps.
#9525 (comment) |
Hey @StevenGranados can you fix the flag and upload a video so we can use on the repo? thanks |
I don't know how to put files in the source. I think there might have to be a new type of MediaSource to allow for it. At the moment there is Uri and File (I assume from the device file system). |
#11417 uses a Uri in the Source parameter. I assume you can control the availability of this? |
@StevenGranados I think @rmarinho just meant a video online in a location which won't change. Personally I've always found this to be an issue, even when testing against Channel 9 videos e.g. https://sec.ch9.ms/ch9/80a3/6563611f-6a39-44fa-a768-1a58bdd080a3/HotRestart.mp4 So I'd suggest something like that which should hopefully be stable for months if not longer. |
@rmarinho I've made some changes |
I think the pause is there to stop background playback of the mediaelement, but since the next line also seems to stop it, I removed the line.
This reverts commit 7a8d7d8.
Description of Change
Removed line that pauses player before setting it to null.
I think the pause is there to stop background playback of the media element, but since the next line also seems to stop it, I removed the line.
I appreciate there is probably a lot more going on here though @peterfoot .
I'm not sure how to handle the video source, there has to be one for the exception to occur. It's currently taking a hard-coded URL source.
Issues Resolved
API Changes
None
Platforms Affected
Behavioral/Visual Changes
Will now not give an exception on Dispose.
Before/After Screenshots
Not applicable
Testing Procedure
Have a page in iOS with a media element. Change page so that the media element is disposed.
PR Checklist