Code Quality: Added ThemedIcon #15560
Conversation
|
✅ Successfully formatted XAML files. |
mdtauk
changed the title
yaira2
force-pushed
the
mdta-ThemedIcon
branch
2 times, most recently
from
fe394de
to
9ab4575
Compare
- ThemedIcon implementations - Test UI for ThemedIcon - MenuFlyoutItemEx - All icons for ThemedIcon
…o mdta-ThemedIcon
Made "Accent" a specific IconColorType = this would be used for the Copying, Moving, Extracting actions in the Status Center - where we need outlined as well as filled icons, only displaying in the Accent Colour
Added a brush and visual state for Disabled while Toggled, using TextOnAccentDisabled brush
There was a problem hiding this comment.
@0x5bfa we're doing the final review on this PR. Can you please remove the enum to reduce confusion?
There was a problem hiding this comment.
For the future reference, we’ll make a test project and it’ll need this but we don’t want to make things complicated thus we might be going to create a source generator for it.
|
|
||
| var windowId = MainWindow.Instance.AppWindow.Id; | ||
| ThemeSettings = ThemeSettings.CreateForWindowId(windowId); | ||
| ThemeSettings.Changed += (s, e) => { IsHighContrastChanged?.Invoke(this, s.HighContrast); }; |
There was a problem hiding this comment.
Should this be split onto separate lines? I'm fine with either way but I wanted to point this out just in case.
There was a problem hiding this comment.
I’m fine either but I prefer this.
There was a problem hiding this comment.
I understand this was just added for testing, let's keep a copy but save it for the PR where we actually remove OpacityIcon and replace all instances with ThemedIcon.
There was a problem hiding this comment.
Sure.
For the future reference, this control can be also used for bitmap images, and we’ll have to create an extension control also for AppBarButton (AppBarButtonEx).
src/Files.App/UserControls/Menus/MenuFlyoutItemWithImage.xaml.cs
Outdated
Show resolved
Hide resolved
src/Files.App/UserControls/Menus/MenuFlyoutItemWithOpacityIcon.xaml.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Yair <[email protected]>
Co-authored-by: Yair <[email protected]>
Co-authored-by: Yair <[email protected]>
….xaml.cs Co-authored-by: Yair <[email protected]>
Co-authored-by: Yair <[email protected]>
| @@ -0,0 +1,4568 @@ | |||
| <!-- Copyright (c) 2024 Files Community. Licensed under the MIT License. See the LICENSE. --> | |||
There was a problem hiding this comment.
From a maintenance perspective, a XAML file with 4500 lines is not ideal 😅
There was a problem hiding this comment.
Splitting it can be done, but adding new icons would involve finding the right place to add it to.
We could use a merged dictionary for this.
There was a problem hiding this comment.
I would prefer a single file, but I'll follow your lead on this if you suggest a different approach.
There was a problem hiding this comment.
I would prefer a single file, but I'll follow your lead on this if you suggest a different approach.
I am happy to split it up, if there is a need for smaller Xaml files - but I would like you to decide on how you would like to group them. My Xaml regions are just how I see them being grouped, but as I wont always be the one adding new icons in the future, the grouping should reflect your sorting @yaira2
There was a problem hiding this comment.
Would it make sense to split out the icons into a separate file and have the base template be its own file? That ways it's easier to spot where to find icons or the actual ThemeIcon control's code.
There was a problem hiding this comment.
I can split the icons into separate Xaml, the base default control template is already separate.
| <Setter Target="PART_OutlinePath.Fill" Value="{ThemeResource ThemedIconHighContrastBrush}" /> | ||
| <Setter Target="PART_FilledPath.Fill" Value="{ThemeResource ThemedIconHighContrastBrush}" /> |
There was a problem hiding this comment.
Why are we doing this manually instead of all brushes just pointing to these two values in high contrast? That would allow us to not having to listening to the high contrast event.
There was a problem hiding this comment.
We have the option to always use High Contrast icons, for those users who don't like the use of the accent colour.
There was a problem hiding this comment.
@mdtauk Is this a use case we plan to support?
@marcelwgn does this affect performance or is there no harm in keeping it just in case?
There was a problem hiding this comment.
@mdtauk Is this a use case we plan to support?
I don't know, you will have to decide on that
@marcelwgn does this affect performance or is there no harm in keeping it just in case?
It is just a property UseContrast which works alongside the internal _isHighContrast bool which is set by the Theme Changed service
There was a problem hiding this comment.
Those brushes are set when user set Windows high contrast on AND/OR when user make a preference on using high contrast icons (if we decide to support this). If we drop the second one’s support, we should define high contrast brush only in the HighContrast XAML key.
There was a problem hiding this comment.
I could differentiate between UseContrast and _isHighContrast so the HighContrast resources kick in, and the specific High Contrast brush we use is reserved only for the optional use.
There was a problem hiding this comment.
My 0.02€: If we currently don't support the option to toggle high contrast on for Files regardless of the system settings, I would not go the length to support it now. I agree with @mdtauk that it's not much more to support, however event listeners aren't free either. And the ThemeIcon control might get rendered dozens of times so small costs might add up.
There was a problem hiding this comment.
They way I see it, HighContrast based on the system rather than the control's "UseContrast" property. Still requires an event listener, so we can replace the Layered visual state with the outline.
The UseContrast setting basically also replaces the layered icon state with the Outlined icon - however we also have a separate brush resource, which makes the icons use the same brush as the text around it, ensuring it stands out clearer. (our base brush is closer to the TextSecondary brush so they don't feel too intrusive.)
Even if we were to remove the alternate brush choice, and the UseContrast property - and decide never to support it, or custom icon colours for the custom themes in Files - the HighContrast system event is still essential.
There was a problem hiding this comment.
I am happy to remove the UseContrast additional setting, if no purpose is seen in it.
I know some people use a grey Accent colour or may want to change the icon colour via a custom theme - but if there is no plan to implement that, then so be it.
There was a problem hiding this comment.
Olay, let's remove and if the need arises, we can refer to this PR and add it back.
This comment was marked as outdated.
This comment was marked as outdated.
|
@marcelwgn as you suggested we decided to create a separate class library for controls (Files.App.Controls) and an unit test project (Files.App.UITests) for that project. To ease PR’s review work, we’ve splitter up work into three: adding template project for both, adding ThemedIcon implementation and its test and lastly replacing the old icons with this icon control. Thank you for reviewing! |
Resolved / Related Issues
This is an ongoing project to try to work on a new ThemedIcon control
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.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.