Theme responsive notification icons

[maniman303]

Hi! This PR will make notification icons (the ones in tray) responsive to the windows theme - white icons in the dark mode and black icons in a light mode :). They will also automatically change with system theme change.

Should deal with #14, but tested only in Windows 11. Windows 10 might use different registry keys, but support can be easily added later.

[ayufan]

@maniman303

I skimmed through those changes. The whole idea and implementation seems great to me 💯

Those are my review comments:

• In general I have preference to implement a single purpose helpers that are self-containing instead of implementing an accessor class (RegistryMonitor) that is then consumed in Instance
• Would you mind adding to CommonHelpers a simple class that implements minimal set of functions as described below with this interface
• If majority of code you took are from external (like copied from StackOverflow), this module should be put into ExternalHelpers with a comment describing its origin
• This means that we would remove Instance.Initialize() calls and rather use this interface with property accessor, but allow to register on DarkModeChanged
• The Resources.Designer files are rewritten with pl_PL locale. Can you make it en_US as it was?
• There appears to be a few fully rewritten files (Instance.cs, PowerControl/Controller.cs, etc.). Project uses CRLF line encoding (as defined in omnisharp.json). Can you correct this to have minimal set of changes?

Class

namespace ExternalHelpers
{
  public class WindowsDarkMode
  {
      public static bool IsDarkModeEnabled { get; }
      public static event Action<bool> DarkModeChanged;

      static WindowsDarkMode() { /* init */ on first use }
  }
}

[maniman303]

Sure thing, thanks for review! I believe I have to switch VS to English to make comments in generated classes English, too. But RegistryMonitor should be moved as is to ExternalHelpers (it already has comments on origin, license) or should it be remade into something more suiting the convention, eg. merged into WindowsDarkMode? I would really like not to do the latter, but I will respect the project :D

[ayufan]

@maniman303

should it be remade into something more suiting the convention, eg. merged into WindowsDarkMode

That is in general my idea to make things isolated and have minimal dependencies outside. So, those components can be replaced if needed, and they are self-contained. Try it and see how much effort is that. You could even make RegistryMonitor private part of the WindowsDarkMode if this helps.

[maniman303]

Oki, I will try. Also it looks like github actually stores project files in LF, not CRLF (I have used git config --global core.autocrlf false re-cloned the repo and checked files in the unmodified main branch)

[ayufan]

@maniman303 Yes, git will drop CRLF. This is my Git Config: core.autocrlf=true.

[maniman303]

As I didn't want to break anything in RegistryMonitor I moved it to the WindowsDarkMode.cs as an internal class. Also I've added in comments proper links to the source and license

[ayufan]

@maniman303 I also wonder. This is quite heavy duty. Each app is UI app. Maybe we could actually make it way simpler without additional thread reading registry. Something like this would be equally effective and equally performant:

public static bool IsDarkModeEnabled { get; private set; }

private static System.Windows.Forms.Timer timer = new System.Windows.Forms.Timer()
{
   Interval = 1000
};

static WindowsDarkMode()
{
   ...
   IsDarkModeEnabled = FetchIsDarkModeEnabled();

   timer.Tick += delegate
   { 
      var value = FetchIsDarkModeEnabled();
      if(value == IsDarkModeEnabled)
        return;
      IsDarkModeEnabled = value;
      DarkModeChanged(value);
   });
   timer.Start();
}

private static bool FetchIsDarkModeEnabled()
{
   try
   {
      string RegistryKey = @"HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes\Personalize";
      int theme = (int?)Registry.GetValue(RegistryKey, "SystemUsesLightTheme", 1) ?? 1;
      return theme == 0;
   }
   catch
   {
      return false;
   }
}

[maniman303]

One one hand it seams a bit hacky, one the other it's now much simpler. But it works :)

[ayufan]

@maniman303 Did you missed icon for FanControl?

[ayufan]

@maniman303 This looks great 💯 My final comments for me to merge it:

• I miss .ico to FanControl
• We can remove the event as we don't seem to use
• Also, since there's nothing really new in ExternalHelpers.WindowsDarkMode now, can we move it to CommonHelpers

I use ExternalHelpers to track "externally licensed components".

[maniman303]

Fan control is missing .ico because if I'm not wrong it doesn't show up in notification tray. It's missing implementation via Controller class like others "packages" so I on purpose didn't modify it

[ayufan]

@maniman303 Take a look at this: https://github.com/ayufan/steam-deck-tools/blob/main/FanControl/FanControlForm.cs#L154, there's an access to notifyIcon. Probably the place to update is to use propertyGridUpdateTimer_Tick.

The FanControl was the first to write and later I choose better and simpler design with Controller.cs.

[maniman303]

Okay, it looks like a good place to update notifyIcon, but whenever I try to add to FanControlForm.resx a new resource I get "You are trying to edit a resource file that is a port of another project item. Editing this item could corrupt the project item..." :/

[ayufan]

@maniman303 Strange. I will handle it myself. Can you just add a ico to project?

[maniman303]

Sure, no prob