Fix for MediaElement Android Resource file not found #1537
Conversation
|
My test is not testing. I will revert unless you want me write out a multitarget test pattern and just test my item? Or i can start a discussion to introduce device specific tests. This might resolve some issues before they creep into codebase? Or expose issues that exist that have not been found. Maybe something like this: https://github.com/dotnet/maui/wiki/Testing |
|
@ne0rrmatrix thank you for the submission. On the topic of device specific tests I would say ignore it for now. @pictos has this PR #1444 aimed at providing the ability to do it. Running the test locally is enough for now. If you have the test already it might be worth opening another issue with the code so we don't miss it later |
I'm going to revert the changes to test since after examining how testing works I determined my modification to test does not work as intended. That was an error by me in my understanding of testing in general. |
src/CommunityToolkit.Maui.MediaElement/MediaSource/MediaSource.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.MediaElement/MediaSource/MediaSource.shared.cs
Outdated
Show resolved
Hide resolved
….shared.cs Co-authored-by: Pedro Jesus <[email protected]>
|
Added fix for #1538 A question for maintainers. Should I create a seperate PR for each thing I fix? Or can I lump a few fixes for related issue? |
|
@ne0rrmatrix, please create a separated PR. It's ways a good practice a PR per issue/bug, that way is easier to revert or find the PR that made the change (: |
I removed the changes to deal with the other issue. I will work on it in a separate PR. |
src/CommunityToolkit.Maui.MediaElement/Views/MediaManager.windows.cs
Outdated
Show resolved
Hide resolved
…e0rrmatrix/MauiOld into MediaElementResourceAndroidFix
|
I checked against main and the bug I was worried about exists already and I only noticed it when testing my fix. link to issue: #1545 That issue is unrelated to this so I guess this fix is good to go after review. |
|
@ne0rrmatrix I don't how to test your changes, do you mind creating a sample on our sample app to validate it? Can be a new Page |
@pictos I have been using sample app from PR to test. Just load it in android emulator and navigate to media element. Choose resources from source drop down menu. If it works then my PR works. If you load main for the same test you will see it fail to load local resources from resource folder.
|
|
FYI I'll take a look during this weekend, asap |
There was a problem hiding this comment.
Just changed the prop to be a field, since it's private it makes sense and using the .NET API.
Thanks for the fix @ne0rrmatrix
Before you submit please check that this work relates to one of the following:
Description of Change
Adds a check and modifies path for Resource if device type is Android
Linked Issues
Linked issue fixed:
#1399
#1433
Fixes
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
This adds a bool to check device type. If device type is android it modifies path to add the following prefix to path: "Assets/" This fixes a bug when selection Resource from device to playback video on android you get file not found. This fix is for android. Stock behavior is unchanged for all other devices.