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 Media Element Position Bug for Android and IOS #1555

Merged

Conversation

ne0rrmatrix
Copy link
Contributor

  • Bug fix

Description of Change

Fixes position tracking not working when changing sources in media element on Android and IOS.

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

This change fixes the problem but events are firing multiple times like clear timers runs a bunch of times on android when I was looking at it. Is it needed for it to fire so many times? Can we handle this better? It seems the way we are handling events is slightly different depending on device.

On windows this was not an issue. But at least on Android it looks like we are doing things completely differently in handling the same things. Is there any plan to change this and maybe improve it? I am happy to work on it if someone has any idea on how to make this better.

I am inexperienced and I may be completely wrong but I have a ton of time if anyone thinks it would be a good idea to unify all of these behaviors into a single way of doing things. If that is ok I will make start a discussion and see where it goes. At this time I am looking for opinions on whether it is a good idea.

Copy link

@zabanet zabanet left a comment

Choose a reason for hiding this comment

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

Thanks ne0rrmatrix! Looks like a simple fix. Not much to review here. Everything looks fine. Can someone with write permission approve this? At this moment the MediaElement is not really usable on Android and iOS.

@pictos
Copy link
Member

pictos commented Nov 27, 2023

@ne0rrmatrix how do you plan to unify these behaviors, since they need to touch the native layer? At the end of the day, you'll have to separate the code to handle that, no?

@ne0rrmatrix
Copy link
Contributor Author

@pictos I looked into it and agree. I don't see way to unify everything without rewriting the whole thing. It is not needed anyways. I spent some time following the flow for events and noticed some of them were firing repeatedly and creating issues. It appears to be an issue with dotnet 8.x

I did not check but the flow of events was not causing issues for dotnet 7.0.x as everything still works as expected there. But an example is the flow of event triggers has resulted in weird code execution that has required updates and/or fixes to code with dotnet 8.0x.

@pictos
Copy link
Member

pictos commented Nov 27, 2023

Yeah, the best thing you can do is try to document when the event is fired... Some time could be a bug on native platform or the way it works

@ne0rrmatrix
Copy link
Contributor Author

@pictos changing the behaviors in this PR seems outside the scope of a fix for current issue. I can go work on that separately and open a discussion about updating that. Is there anything with this PR that needs changed?

@pictos
Copy link
Member

pictos commented Nov 27, 2023

Yeah, don't do this here, I believe you can open an issue about it, and if you want work on it (: You have my bless xP

@ne0rrmatrix
Copy link
Contributor Author

@pictos is this PR good to go?

@pictos
Copy link
Member

pictos commented Nov 27, 2023

Will try on my end this week (I hope)

@jfversluis jfversluis added the 📽️ MediaElement Issue/PR that has to do with MediaElement label Nov 27, 2023
@ne0rrmatrix
Copy link
Contributor Author

@pictos or @jfversluis any chance you can review this?

@jfversluis
Copy link
Member

Thank you for another great contribution! 🔥

@jfversluis jfversluis merged commit 02896d3 into CommunityToolkit:main Dec 7, 2023
8 checks passed
@ne0rrmatrix ne0rrmatrix deleted the FixMediaElementPositionBug branch February 12, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📽️ MediaElement Issue/PR that has to do with MediaElement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] PositionChanged not being fired after loading local file from C#
5 participants