-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Multiple Fixes for Resizetizer (esp. Android App Icons) #8020
Conversation
@@ -32,7 +32,6 @@ | |||
<ThirdParty Include="ShimSkiaSharp.dll" /> | |||
<ThirdParty Include="Fizzler.dll" /> | |||
<ThirdParty Include="Newtonsoft.Json.dll;" /> | |||
<ThirdParty Include="Svg2VectorDrawable.Net.dll" /> |
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.
Removing this svg2vector as the library is not currently used nor is it super useful if it is.
protected void AssertFileMatches(string destinationFilename, object[] args = null, [CallerMemberName] string methodName = null) => | ||
AssertFileMatchesReal(destinationFilename, args, methodName); | ||
|
||
protected void SaveImageResultFile(string destinationFilename, object[] args = null, [CallerMemberName] string methodName = null) => | ||
SaveImageResultFileReal(destinationFilename, args, methodName); |
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.
These 2 methods can be used to generate (in source folder) and then test/compare results. Typically, you would:
- use
SaveImageResultFile
locally at the end of the tests with all the files you want - ensure the image is correct
- replace with
AssertFileMatches
- push to git
CI will come along and run the tests and compare. And, if you use a smart git tool, the tool can show you diffs of git files and you can use runs locally to ensure you did not break anything.
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.
Did you check if we could use an assertion library like Verify?
https://github.com/VerifyTests/Verify
Might simplify some of the test code.
There is an older one called ApprovalTests
, but it only worked on Windows.
src/Templates/src/templates/maui-mobile/Platforms/Android/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
ccce376
to
d88716c
Compare
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.
I like all the tests!
I couldn't find any code issues to comment about, and hopefully the changes are tested well enough.
protected void AssertFileMatches(string destinationFilename, object[] args = null, [CallerMemberName] string methodName = null) => | ||
AssertFileMatchesReal(destinationFilename, args, methodName); | ||
|
||
protected void SaveImageResultFile(string destinationFilename, object[] args = null, [CallerMemberName] string methodName = null) => | ||
SaveImageResultFileReal(destinationFilename, args, methodName); |
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.
Did you check if we could use an assertion library like Verify?
https://github.com/VerifyTests/Verify
Might simplify some of the test code.
There is an older one called ApprovalTests
, but it only worked on Windows.
Ooh, VerifyTests/Verify looks nice! I'll do that in a separate PR so that this one can be merged without ant testing changes. |
Thanks. Now there are Windows issues as well with icon - aspect ratio, resizing, color, background color etc. properties. Will the fixes follow this one? |
@janseris this PR fixes all the platforms. |
This was just merged 7 minutes ago. It will be released in the next MAUI servicing release. There are multiple fixes in this PR, do you have a specific reproduction you can attach to a new issue for the problem you're referring to? |
@Redth @mattleibow great, thank you for info. |
You can try it out right now, see #7653 |
🙌 |
When are all these issues are going to be officially released? |
Same here |
Anyone care to reply please? we need to know when these changes are going to be released, this is urgent. |
They will be released with visual studio 17.4 preview 2.1 in a few days I believe. |
@mattleibow I tried these issues on VS 2022 17.4 Preview 2.1 and they are still active, MauiIcon and MauiSplashScreen looks very bad on almost all platforms, the icon is stretched and not fit also splash screen looks bad, are there any limitations on the SVG files that should be used for icon and splash screen? I can't figure this out this is so frustrating. |
This PR was more for app icons, but the issue describe was just fixed here: #10516 |
@mattleibow I didn't understand your comment, the app icons are not working as expected and so is splash screen, so can you guys provide us with time estimate on when both (app icons and splash screens) are going to be working as expected? |
This is an old PR. Please open a new issue or comment on an open issue. Also, make sure you describe what is happening when you say not working. And also, some issues may be fixed in rc1 or rc2 of net7. But the main thing is that commenting on a merged PR almost always means it goes unnoticed. |
The question is when this fix is released for MAUI users in Visual Studio, we don't pull and build maui repo after each commit/merge request. |
This was released in vs preview 2.1. I am not sure what you are seeing. Can you attach a screenshot? As far as I was aware, the app icons should be fixed. If not, then something else is gone wrong or maybe I am not considering some scenarios. Please open a new issue with screenshots of what you see and maybe the icon files in a file new template so I can make sure I get all the things tested. "not working as expected" is very hard to debug. |
@mattleibow I have reported my issues as a bug as you asked and below is the link for the new incident I opened, please take a look at it since I attached my test project along with my icon and splash screen I used and let me know your findings: |
Description of Change
This PR is a massive set of changes and test data to fix issues and ensure they stay fixed. I have listed all the issues that I know have been fixed, but I actually may have fixed a few more based on visual inspection.
I have added logic so that test runs can be cached in the working folder for git, and then CI/subsequent runs can use the reference image. This is far easier that what I was doing by picking pixels and checking colors. There is no way I could have done that with the various permutations. And, it is a much better with a visual inspection.
This PR is part of the larger effort to improve Resizetizer with many issues linked from this top-level issue: #7970
Issues Fixed
General Fixes
Splash Screen Fixes
Currently if you make any changes to the MauiSplashScreen properties (such as set a foreground tint) then it does not actually trigger a new splash to be generated.
Also, on iOS and Mac Catalyst, the generated storyboard does not actually use the new image if there was an alias/Link set.
Android Manifest Update Fixes
Moved to #8063