-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix: Fixed issue where the 'layout' icon didn't match the select layout #17436
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: Fixed issue where the 'layout' icon didn't match the select layout #17436
Conversation
I expect it does 👍 It likely fixes an issue we were having with the Omnibar as well. |
It seems to be causing a crash, it might be more reliable to use the |
My bad, I pushed without testing. Your fix seems to work 👍 |
|
@ferrariofilippo thank you for fixing this! I didn't think about debugging this from the control side. |
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 pull request fixes an issue where the 'layout' icon in the navigation toolbar didn't update to match the selected layout. The solution implements a workaround for the fact that Style changes don't automatically invoke OnApplyTemplate in WinUI controls.
- Adds property change callback handling for the
Styleproperty inThemedIcon - Re-enables icon display in navigation bar suggestion items
- Updates template part retrieval and icon state when style changes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Files.App/UserControls/NavigationToolbar.xaml | Uncomments icon display elements and adjusts column layout for navigation bar suggestions |
| src/Files.App.Controls/ThemedIcon/ThemedIcon.cs | Registers callback for Style property changes and handles cleanup on unload |
| src/Files.App.Controls/ThemedIcon/ThemedIcon.Properties.cs | Implements Style property change handler that refreshes icon state |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Resolved / Related Issues
To prevent extra work, all changes to the Files codebase must link to an approved issue marked as
Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.Layouticon doesn't change #17347Notes
Stylechanges do not invokeOnApplyTemplate, so this solution is basically a workaround.Finalizers... should I dispose the object in a different way?Steps used to test these changes
Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.
Cloud Sync Status(I can't test this myself)