Fix Media Element Windows CTD #1921
Conversation
…esktop. Updated the `Content` property of the `fullScreenButton` object from `fullScreenIcon` to a new `FontIcon` object. The `Glyph` property is now set to "\uE740" and the `FontFamily` property is set to "Segoe Fluent Icons". This change modifies the visual representation of the `fullScreenButton` in the UI.
|
Thanks @ne0rrmatrix for the PR. I believe changing the icon to a font icon makes sense. However, is Segoe Fluent Icons a system windows font? if yes, what will happen if that font does not exist in the OS? 🤔 |
src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.windows.cs
Outdated
Show resolved
Hide resolved
This is a windows specific fix and the font icon is standard across current windows versions that we support. |
Removed the `fullScreenIcon` static readonly object from the `MauiMediaElement` class in the `MauiMediaElement.windows.cs` file. Now, a new `FontIcon` object is directly created in the `OnFullScreenButtonClick` method. This change simplifies the code and makes the `OnFullScreenButtonClick` method more self-contained. This removes reduntant static readonly object that was causing issues. The fontIcon is now generated dynamically to prevent CTD.
|
Since we were having issues with a static readonly object I have removed it and it is now generated dynamically. Not sure if the second call would be impacted but I can't guess every use case. The only question now is do we want to that in case somehow we end up with two fullscreen media elements on the same page which would also generate the same bug? Not sure how that would happen but if it does it will crash. If we do I can remove Edit: Was able to verify when exiting app after enter and exiting full screen that the app can crash while exiting. Fix has been applied. |
Refactored the `MauiMediaElement` class in `MauiMediaElement.windows.cs` file. Removed the `exitFullScreenIcon` static readonly variable and instead created a new `FontIcon` directly where needed. Updated the `OnFullScreenButtonClick` method to set the `fullScreenButton.Content` to new `FontIcon` instances with different glyphs (`"\uE740"` and `"\uE73F"`), representing different icons in the "Segoe Fluent Icons" font family. These changes affect the icons used for the full screen button in the media player. This fixes a small UI issue where the incorrect icon was applied starting with full screen then reversion of Full screen status. So icons got reversed starting when selecting full screen. Fixed now. Removed the static readonly object as I was able to verify it does in fact have a bug with it.
|
I too am a bit nervous hard coding a font here. The smells like a potential for a future bug. @ne0rrmatrix - could you add the font as a resource to the Windows platform in |
Updated the `UseMauiCommunityToolkitMediaElement` method in the `AppBuilderExtensions` class to include a configuration for the "Segoe Fluent Icons" font from the "SegMDL2.ttf" file specifically for the Windows platform. Also, added a new `ItemGroup` in the `CommunityToolkit.Maui.MediaElement.csproj` file to pack the "SegMDL2.ttf" font file into the application when the `TargetFramework` contains '-windows'.
…rmatrix/MauiOld into FixWindowsMediaElementCrash
Updated as requested. Font added as resource for windows. |
I've tried to look but have failed to find the licensing rules around the specific font. Are we allowed to distribute the font? |
|
License can be found by downloading Mac download link here. It is in zip file. Scroll down page and look for mac assets. https://learn.microsoft.com/en-us/windows/apps/design/style/segoe-ui-symbol-font Edit: Copy of text of included license "You may use the Segoe MDL2 Assets and Segoe UI fonts or glyphs included in this file (“Software”) solely to design, develop and test your programs that run on a Microsoft Platform, a Microsoft Platform includes but is not limited to any hardware or software product or service branded by trademark, trade dress, copyright or some other recognized means, as a product or service of Microsoft. This license does not grant you the right to distribute or sublicense all or part of the Software to any third party. By using the Software you agree to these terms. If you do not agree to these terms, do not use the Software." Looks like we cannot distribute them. I will revert the changes. I will mention this font is built into windows and I believe is used directly by the OS in many built in applications like explorer, Settings, etc. Not 100 percent sure but it is a core OS font as far as I know. |
|
I just double checked and google first result is that "Segoe UI" is a windows system font. The MDL2 icons I reference are system icons that are a core part of the OS. I'm not sure how we move forward if this is an issue. We can of course close this PR and go back and revert the previous one that added this use of fonts and look for an alternative method of fixing the previous issue? Can we not rely on system resources to just be available? |
2. remove `static readonly` from `FontIcons` 3. Fix what font icon is applied while switching full screen status
There was a problem hiding this comment.
Thanks James! I owe you an apology - I see now that we were already using Segoe Fluent Icons in MediaElement and that this PR does not introduce that change. (A lazy PR review on my part - sorry!)
I'm cool moving forward with this. I have a couple comments (below). After that, I'm cool approving/merginfg this 💯
src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.windows.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.windows.cs
Show resolved
Hide resolved
src/CommunityToolkit.Maui.MediaElement/Views/MauiMediaElement.windows.cs
Show resolved
Hide resolved
Head branch was pushed to by a user without write access
Description of Change
-Update font icon behavior to prevent CTD.
Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
Fixes crash where if you have multiple media elements on a page in windows the app will immediately crash to desktop. This was an issues with font icons and has been resolved with this PR.