-
Notifications
You must be signed in to change notification settings - Fork 473
Fix issue with tint not applying to loaded images #2077
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 issue with tint not applying to loaded images #2077
Conversation
...unityToolkit.Maui/Behaviors/PlatformBehaviors/IconTintColor/IconTintColorBehavior.windows.cs
Outdated
Show resolved
Hide resolved
|
Hi @brminnick |
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.
Thanks @myix765! Could you please update the IconTintColorBehaviorPage in the sample app to provide an example that hits every new branch in void LoadAndApplyImageTintColor(IView, WImage, Color)?
In my review, I've left a comment on the two branches in void LoadAndApplyImageTintColor(IView, WImage, Color) that I am unable to reach using the sample app.
We are working on maui winui and icons are having issue while navigate back.
Do you have any timeline to merge this PR?
We are waiting for this PR.
@akhilesh-hexagon If you're also able to help provide a sample that covers these two new branches in void LoadAndApplyImageTintColor(IView, WImage, Color), I can merge this PR as soon as you provide it!
| && image.Source is BitmapImage bitmapImage | ||
| && Uri.Compare(new Uri($"{bitmapImage.UriSource.Scheme}:///{fileImageSource.File}"), bitmapImage.UriSource, UriComponents.Path, UriFormat.Unescaped, StringComparison.OrdinalIgnoreCase) is not 0) | ||
| { | ||
| image.ImageOpened += OnImageOpened; |
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.
Could you please update the IconTintColorBehaviorPage in the sample app to include a use-case that hits this specific line of code where this specific if statement resolves to true?
if (element is IImageElement { Source: FileImageSource fileImageSource }
&& image.Source is BitmapImage bitmapImage
&& Uri.Compare(new Uri($"{bitmapImage.UriSource.Scheme}:///{fileImageSource.File}"), bitmapImage.UriSource, UriComponents.Path, UriFormat.Unescaped, StringComparison.OrdinalIgnoreCase) is not 0)I cannot reach this branch using the Sample App and am unable to test it and verify it works.
| } | ||
| else | ||
| { | ||
| ApplyTintColor(); |
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.
Could you please update the IconTintColorBehaviorPage in the sample app to include a use-case that hits this specific line of code where this specific else block resolves?
else
{
ApplyTimeColor();
}I cannot reach this branch using the Sample App and am unable to test it and verify it works.
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.
Pull Request Overview
This PR addresses an issue on Windows where tint colors were not applied to already loaded images by centralizing the TintColor change handling and adding checks for image load state.
- Extracted inline
PropertyChangedhandler intoOnIconTintColorBehaviorPropertyChanged - Switched parameters from
ViewtoIViewfor consistency - Added logic to detect
image.IsLoadedand either apply tint immediately or defer untilImageOpened
| if (element is IImageElement { Source: FileImageSource fileImageSource } | ||
| && image.Source is BitmapImage bitmapImage | ||
| && Uri.Compare(new Uri($"{bitmapImage.UriSource.Scheme}:///{fileImageSource.File}"), bitmapImage.UriSource, UriComponents.Path, UriFormat.Unescaped, StringComparison.OrdinalIgnoreCase) is not 0) | ||
| { |
Copilot
AI
May 29, 2025
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.
After tinting in the ImageOpened handler, unsubscribe this event (e.g., image.ImageOpened -= OnImageOpened;) to avoid memory leaks and duplicate invocations when the image reloads.
| { | |
| { | |
| image.ImageOpened -= OnImageOpened; // Unsubscribe first to prevent duplicate subscriptions |
| return; | ||
| } | ||
|
|
||
| if (e.PropertyName == TintColorProperty.PropertyName) |
Copilot
AI
May 29, 2025
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.
[nitpick] The logic inside this handler duplicates the tint-application flow; consider extracting it into a shared helper method to reduce code duplication and improve readability.
Description of Change
On Windows, if an image was already loaded (ie. after loading the page then navigating away and back to the page) the tint color is no longer applied because the ImageOpened event is not triggered. Adding a check for if the image is already loaded fixes this, but then causes an issue with the ToggleImageSource button. This was fixed by adding another check to see if the WImage source and the View source was the same, if not that means the toggle hasn't happened yet so we wait for the ImageOpened event to apply the tint.
This change also fixes the issue with the change tint color button not working in the sample app.
Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
OS: Windows 10
Testing the behavior is fixed: go to the IconTintColorBehavior page, click on a different page in the FlyoutMenu, then click back on Behaviors (which should still show the IconTintColorBehavior). Before these changes, when navigating back some tint colors weren't applied.