Skip to content
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

[Run] Sync accent color #4107

Closed
dsrivastavv opened this issue Jun 5, 2020 · 29 comments
Closed

[Run] Sync accent color #4107

dsrivastavv opened this issue Jun 5, 2020 · 29 comments
Assignees
Labels
Area-User Interface things that regard UX for PowerToys Idea-Enhancement New feature or request on an existing product Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@dsrivastavv
Copy link
Contributor

Powertoys run accent color should change on changing windows accent color

@dsrivastavv
Copy link
Contributor Author

dsrivastavv commented Jun 5, 2020

@AnuthaDev One thing I am worried about the solution in #2988 is that we would be unnecessarily checking for accent changes each time launcher is visible, impacting startup time. That's why we want to stick strictly to a callback-based solution. Your solution does give a neat way to find accent colors and it would be helpful if you could integrate this into our ThemeManager.

@AnuthaDev
Copy link

@somil55 Yeah, I was thinking about it too, I will work on it tomorrow

@AnuthaDev
Copy link

Hey @somil55 I found this wouldn't it be better if we add theming support this way instead of adding another dependency, its roughly the same work...

@dsrivastavv
Copy link
Contributor Author

@AnuthaDev I have handled high contrast modes in the same way and used controlzex to receive a callback on dark/light windows theme change. One thing you can do is to look at Controlzex syncAll mode here. This should fire up a callback on accent change. Hopefully, this would get you started. Also, I feel it would be better to stick with controlzex rather than implementing a registry listener as controlzex is actively maintained and updated and I found their implementation to be quite efficient.

@dsrivastavv dsrivastavv added Product-PowerToys Run Improved app launch PT Run (Win+R) Window Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Jun 5, 2020
@AnuthaDev
Copy link

@somil55 If we use this, we don't need to callback from the registry

@AnuthaDev
Copy link

The uicolortype background color is black in dark mode and white in light mode

@AnuthaDev
Copy link

@somil55 If we use this, we don't need to callback from the registry

The same event handler can process theme change as well as accent color change

@dsrivastavv
Copy link
Contributor Author

dsrivastavv commented Jun 5, 2020

@AnuthaDev I just tried to use UISettings and the callbacks don't fire for me. UISetting is probably only for UWP apps. Have you tried using it?

Another issue is that if we use UISetting we would have to manually handle changing resource dictionary at runtime. Currently, we have all of it nicely packed in controlzex library.

@AnuthaDev
Copy link

Have you tried using it?

I haven't tested the callbacks yet... But accessing the theme via uicolortype.background works for me...

@AnuthaDev
Copy link

AnuthaDev commented Jun 5, 2020

Another issue is that if we use UISetting we would have to manually handle changing resource dictionary at runtime. Currently, we have all of it nicely packed in controlzex library.

Yeah, fair point I guess. I'll see if any better approach can be used

@AnuthaDev
Copy link

Apparently, windows has a bajilion ways of doing the same thing

@dsrivastavv
Copy link
Contributor Author

Another issue is that if we use UISetting we would have to manually handle changing resource dictionary at runtime. Currently, we have all of it nicely packed in controlzex library.

Yeah, fair point I guess. I'll see if any better approach can be used

I would currently suggest you look at SyncAll mode in controlzex. I think that would be the easiest way to incorporate accent in the current framework.

@AnuthaDev
Copy link

AnuthaDev commented Jun 6, 2020

@somil55 said:

I found their implementation to be quite efficient.

So, I copied and enhanced it in #4134😁

Also,

Another issue is that if we use UISetting we would have to manually handle changing resource dictionary at runtime. Currently, we have all of it nicely packed in controlzex library.

Bet they can't beat this sneaky boi😁

@dsrivastavv
Copy link
Contributor Author

So, I copied and enhanced it in #4134😁

I might be missing something but what is that you are doing differently from their implementation?

Bet they can't beat this sneaky boi😁

You would need to handle a generic case for location of your resource.

@AnuthaDev
Copy link

I might be missing something but what is that you are doing differently from their implementation?

Not much really, but Its less code than adding a dependency

You would need to handle a generic case for location of your resource.

Please elaborate

@dsrivastavv
Copy link
Contributor Author

Bet they can't beat this sneaky boi😁

You would need to handle a generic case for location of your resource.

Currently, you are using App.Resources.MergedDictionaries[0].Source = new Uri($"/Themes/{currentTheme}.xaml", UriKind.Relative) which assumes you will be changing theme resource at index 0. But there could be other resource files related to font, language etc. added in App.xaml. For example :

<ResourceDictionary.MergedDictionaries>
                <ResourceDictionary Source="pack://application:,,,/Languages/en.xaml" />
                <ResourceDictionary Source="pack://application:,,,/Themes/Dark.xaml" />
</ResourceDictionary.MergedDictionaries>

In the above example a language resource has been added <ResourceDictionary Source="pack://application:,,,/Languages/en.xaml" /> at index 0 and you would essentially be overwriting it.

So you would need to figure out a way to identify at which index in Merged dictionary is your resource located (if any) and then overwrite it.

@dsrivastavv
Copy link
Contributor Author

dsrivastavv commented Jun 6, 2020

I might be missing something but what is that you are doing differently from their implementation?

Not much really, but Its less code than adding a dependency

I wouldn't be against adding a dependency. I personally find it better to leverage existing work rather than reinventing the wheel.

@AnuthaDev
Copy link

Bet they can't beat this sneaky boi😁

You would need to handle a generic case for location of your resource.

Currently, you are using App.Resources.MergedDictionaries[0].Source = new Uri($"/Themes/{currentTheme}.xaml", UriKind.Relative) which assumes you will be changing theme resource at index 0. But there could be other resource files related to font, language etc. added in App.xaml. For example :

<ResourceDictionary.MergedDictionaries>
                <ResourceDictionary Source="pack://application:,,,/Languages/en.xaml" />
                <ResourceDictionary Source="pack://application:,,,/Themes/Dark.xaml" />
</ResourceDictionary.MergedDictionaries>

In the above example a language resource has been added <ResourceDictionary Source="pack://application:,,,/Languages/en.xaml" /> at index 0 and you would essentially be overwriting it.

So you would need to figure out a way to identify at which index in Merged dictionary is your resource located (if any) and then overwrite it.

I believe I solved it

@AnuthaDev
Copy link

I might be missing something but what is that you are doing differently from their implementation?

Not much really, but Its less code than adding a dependency

I wouldn't be against adding a dependency. I personally find it better to leverage existing work rather than reinventing the wheel.

Yeah, well this dependency also has a lot of bloat that is not required, also the "reinventing the wheel" part of #4134 is still smaller than using this library...

@crutkas
Copy link
Member

crutkas commented Jun 6, 2020

  1. saying "bloat" is a big statement. be metric / data driven please. What is the added memory / start time ramifications.
    • we actually just helped reduce startup time drastically
  2. MahApps is pretty well liked and used. It is open source and has support in the community too.
  3. FancyZones already uses MahApps in the editor :)
  4. We have to be Dark mode and light mode compliant as well as high contrast. We will support RS4 min version as well (your PR currently bumped the versions)
  5. Most importantly: Before we do a any shift, lets discuss how it actually will work and impact stuff. Doing POCs for how is fine but discussion needs to happen so we have a strong handshake.

For accent color for PowerToys may be something we want at a global level, not just PT Run. Which then implies that would be a C++ library as FancyZones and other apps may want it.

@AnuthaDev
Copy link

@crutkas Thanks for replying:

  1. Okay, I will try to run benchmarks :)
  2. I am not denying this, I am just wondering if adding a full fledged UI toolkit for only theme change is a better option than handcrafting the code
  3. Yeah, I just saw that. Does it mean there will be no changes in binary size if we include mahapps here?

We have to be Dark mode and light mode compliant as well as high contrast

My PR supports all modes :)

We will support RS4 min version as well (your PR currently bumped the versions)

Yeah, I was getting build errors, that is why I did it. The PR is still in draft though and I will revert the changes :)

  1. Okay, understood 👍. Actually I am pretty new here and still in the process of learning. Sorry for causing any inconvenience😔

For accent color for PowerToys may be something we want at a global level, not just PT Run. Which then implies that would be a C++ library as FancyZones and other apps may want it.

Yeah, that makes sense. I say, lets keep it localized to PT Run for a release and then work on making it a c++ library.

@niels9001 niels9001 added the Area-User Interface things that regard UX for PowerToys label Aug 6, 2020
@crutkas crutkas added Idea-Enhancement New feature or request on an existing product and removed Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Sep 28, 2020
@crutkas
Copy link
Member

crutkas commented Sep 28, 2020

@somil55 shouldn't this be easy now we've migrated to ControlzEx?

@crutkas crutkas self-assigned this Sep 28, 2020
@crutkas crutkas added this to the v1.0 Release milestone Sep 28, 2020
@dsrivastavv
Copy link
Contributor Author

@crutkas Controlzex has added functionality to invoke a callback on accent change. So I am guessing this should be a straightforward feature to implement.

@niels9001
Copy link
Contributor

@somil55 @crutkas

For #6899 I used ModernWPF.. It has a {DynamicResource SystemAccentColor} that updates automatically when the Windows accent color is changed. Works really well. It also has darker/light accent brushes (like UWP) that might be useful for e.g. the focus color. Might be something for Run?

@dsrivastavv
Copy link
Contributor Author

dsrivastavv commented Oct 6, 2020

@niels9001 This seems like a clean fix for adding accent colors. Though one catch here might be dealing with accent colors in high contrast modes.

@crutkas Any thoughts on this?

@niels9001
Copy link
Contributor

@somil55 I think Windows defaults back to back or white for the Accent color, right? I guess this is to not conflict with actual high contrast colors. For the focus color, we could could use the primary high contrast color (e.g. purple, green, turquoise?

@dsrivastavv
Copy link
Contributor Author

@somil55 I think Windows defaults back to back or white for the Accent color, right? I guess this is to not conflict with actual high contrast colors. For the focus color, we could could use the primary high contrast color (e.g. purple, green, turquoise?

@niels9001 On a quick pass in high contrast mode, it seems like accent color is being set to black/white. And your idea for using primary high contrast color sounds good.

@crutkas crutkas added the Help Wanted We encourage anyone to jump in on these and submit a PR. label Oct 23, 2020
@crutkas
Copy link
Member

crutkas commented Oct 23, 2020

Would love if a community member could help on this.

@niels9001
Copy link
Contributor

@crutkas Can we use ModernWPF? It's MIT licensed and allows for seamless accent color syncing.

Additionally we can use those styles instead of the ones we've made (e.g. for the scrollbar). Using it for ColorPicker as well.

@niels9001 niels9001 self-assigned this Dec 2, 2020
@crutkas crutkas removed this from the 2020 Stability Release milestone Dec 14, 2020
@niels9001 niels9001 mentioned this issue Dec 20, 2020
5 tasks
@niels9001 niels9001 added the Status-In progress This issue or work-item is under development label Dec 24, 2020
@enricogior enricogior added Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Help Wanted We encourage anyone to jump in on these and submit a PR. Status-In progress This issue or work-item is under development labels Jan 6, 2021
@enricogior enricogior changed the title Sync accent color in powertoys run [Run] Sync accent color Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface things that regard UX for PowerToys Idea-Enhancement New feature or request on an existing product Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

No branches or pull requests

5 participants