[POC] Decide default theme on Windows based on the system theme

[n0toose]

wxWidgets provides an easy way to read registry values, apparently.
Instead of forcing the user to switch their theme so that it corresponds
to the system theme, it'd be a better idea to just do that for them.

What this change does not do: If the user changes the system theme after
Tenacity is initially configured, the theme won't change.

Additional Notes / Feedback Needed

• The change has been submitted on a branch so that other contributors with commit access can freely work on this. Some elements really make dark mode look ugly on Windows, so it may be worth pouring in some additional effort on this.
• The change was also authored with a potential expansion to GTK/Qt platforms in mind.
• The change was tested on a Windows 10 machine that had dark mode enabled for applications. Works as intended.
• I prepared a Visual Studio 2022 environment in a virtual machine and developed from it in order to get this to work. It was painful and took me several hours, but I hope it's worth it.

Feedback Needed

This probably marks the "first substantial" patch I've sent in that concerns the code, so please bear with me, I'm trying my best.

• Although I used the Theme class just to get things working, I really doubt that using it and exposing it everywhere is a good thing and sensible thing to do design-wise. I don't have any better alternatives in mind (other than creating a new class) and need some further feedback.
• I am not sure if my preprocessor hacks are actually normal or super cursed.
• Should we just put the Windows-specific stuff in a separate source file or something?
• Is me initializing the booleans in the header file super cursed?
• I used #ifdef _WIN32 instead of #if defined(_WIN32) like in other places. It made more sense syntax-wise, especially because I am not using a second condition over there. Need confirmation that this is okay.
• The header defines functions that are never initialized in other platforms other than Win32. What now?

Checklist

• I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
• I made sure the code compiles on my machine
• I made sure there are no unnecessary changes in the code*
• I made sure the title of the PR reflects the core meaning of the issue you are solving*
• I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

[n0toose]

Just to reiterate: This is a proof of concept and even if I've spent a lot of time polishing this change, it isn't as good as I'd like it to be.

[n0toose]

Force-pushed to replace _WIN32 with __WXMSW__ as pointed out by @leio.

[leio]

I think how this is often done is to have a "System Theme" theme option, which then uses the system theme. But in our case, I suppose it's really just about picking a different default custom theme instead, but I guess that could still work like that, but with a bit different naming than "System Theme"? "System Theme" would be also fine if we took colors and such from the current system theme for what we draw custom, instead of having predefined ones.
If we have a "theme" that is about following system theme, it also would avoid it working only on initial configuration. Though ideally it would even runtime switch then when the system theme changes while tenacity is running, this is somewhat common for people in e.g. GNOME where the whole system theme might be set up to switch from Adwaita to Adwaita-dark once the sun goes down.

Just some initial thoughts.

[n0toose]

Yeah, runtime switching is a whole different beast of a subject though. Which events would we listen to? Can we allow the user to manually override this? We should check whether the user is using another theme other then Light/Dark!

The GNOME feature you brought up only exists unofficially on Windows (https://justgetflux.com/news/pages/v4/readme/), so I figured that it'd be a good idea to start from there as a very early step.

[leio]

Yeah, runtime switching is a whole different beast of a subject though. Which events would we listen to?

GTK will get appropriate style change events, etc. I don't know if wxGTK exposes them to us properly or not these days, might need some #ifdef __WXGTK__ code.

Can we allow the user to manually override this? We should check whether the user is using another theme other then Light/Dark!

That would be the idea - the user would choose "System Theme", "Light Theme" or "Dark Theme" or whatever else we have. System Theme would honor system theme as much as possible, the rest are what our theme says to have. Initially "System Theme" would switch between light and dark variants appropriately, following the system theme, and maybe in the future follow it even more (e.g. look more like Yaru in colors on Ubuntu, because it would take the base colors out of the theme).

The GNOME feature you brought up only exists unofficially on Windows (https://justgetflux.com/news/pages/v4/readme/), so I figured that it'd be a good idea to start from there as a very early step.

I'm not sure what that f.lux link shows, maybe you had a copy-paste mistake

[n0toose]

version 4.118 and 4.119 (Feb 2021)

• Try this: “Effects…” “Use Dark mode at sunset” to turn Windows Dark mode on at night and off during the day

[n0toose]

I think that this change should be merged, as including it, even if it isn't in a 100% ready state, is most likely better than nothing.

[generic-pers0n]

Hello! I've been recently thinking about this pull request as I'm looking for stuff to merge into Saucedacity's codebase, based on the discussion in #733.

I have found interest in maybe testing this out and even seeing how I can make this a preference. However, I've been looking at other ways on how to implement this, such as inverting theme colours without changing preference, etc. That works well for light themes but not so much for dark themes.

I was looking through wxWidgets' 3.2.0 documentation, and I found wxSysColourChangedEvent. However, I don't exactly know how we're going to handle this event, but it is one to look at.

---

UPDATE: A couple of things about wxSysColourChangedEvent:

• Building against wxWidgets 3.0, this might only work on Windows given the 3.0 documentation. This is contrast to wxWidgets 3.2.0 documentation. I might (and hope to) be wrong.
• This works on Linux, so rather than making this Windows only, we can make this a cross-platform implementation.
• We would handle a wxSysColourChangedEvent in ProjectWindow.
• This can work beyond initial configuration of Tenacity. Here, we can introduce a new preference.
• I would use your code in handling this.

I was able to trip a wxWidgets assertion by implementing this event handler just to test out if this works on Linux. While it works on Pop!_OS 22.04 (and I'd imagine this would be the case with GNOME 40+ or so), results may vary. On Windows, it should hopefully work.

Overall, I propose this POC that builds on top of yours, but it uses wxSysColourChangedEvent instead of reading a registry key. It is cross-platform and is not limited to initial configuration. However, is likely better because you have intended to write this with the possibility of switching to a different toolkit (e.g. Qt). I will need to thoroughly look at your code to improve this POC here.

---

UPDATE 2: Actually, it might not be a simple as I thought, but if the target is Windows only, then a modified POC still works because we can check that registry key each time the theme changes. This would be beyond initial configuration and can still benefit this.

[generic-pers0n]

So, after a little more research, my final POC: for wxWidgets 3.1.3 or later, we use wxSystemAppearance to tell if the system's appearance is dark or not (wxSystemAppearance::IsDark()). For wxWidgets 3.0, we limit support to Windows only, relying on parts of this PR's code here to do this. This is unless we use some workarounds with wxColour and wxSystemSettings, but I don't know how this could be done. Either way, I will try to write a patch using Saucedacity's code base to demonstrate a POC.

[generic-pers0n]

Okay. Here is the POC below. I have created two POCs, the first with Tenacity and the second with Saucedacity (as I initially wrote the code using Saucedacity's code base). Note that this requires wxWidgets >= 3.1.3 in order for either forks to properly build (not accounting for other issues).

Of course, this is merely the theme switching part of the overall concept. No new preferences are introduced, but nevertheless, you can see where we're going with the theme switching code.

The code was tested on Pop!_OS 22.04. Both work pretty well, actually. The main window updates automatically, but other windows that we theme don't automatically update (e.g. the About dialog box as that's what I tested with).

Tenacity POC Patch:

diff --git a/src/ProjectWindow.cpp b/src/ProjectWindow.cpp
index 1796c3428..cfef300c5 100644
--- a/src/ProjectWindow.cpp
+++ b/src/ProjectWindow.cpp
@@ -695,8 +695,25 @@ BEGIN_EVENT_TABLE(ProjectWindow, wxFrame)
    EVT_UPDATE_UI(1, ProjectWindow::OnUpdateUI)
    EVT_COMMAND(wxID_ANY, EVT_TOOLBAR_UPDATED, ProjectWindow::OnToolBarUpdate)
    //mchinen:multithreaded calls - may not be threadsafe with CommandEvent: may have to change.
+
+   EVT_SYS_COLOUR_CHANGED(ProjectWindow::OnSysColourChanged)
 END_EVENT_TABLE()
 
+void ProjectWindow::OnSysColourChanged(wxSysColourChangedEvent&)
+{
+   auto sysAppearance = wxSystemSettings::GetAppearance();
+
+   if (sysAppearance.IsDark())
+   {
+      theTheme.LoadTheme(themeDark);
+   } else
+   {
+      theTheme.LoadTheme(themeLight);
+   }
+
+   ThemePrefs::ApplyUpdatedImages();
+}
+
 void ProjectWindow::ApplyUpdatedTheme()
 {
    auto &project = mProject;
diff --git a/src/ProjectWindow.h b/src/ProjectWindow.h
index 1d759c8e9..2c9cce311 100644
--- a/src/ProjectWindow.h
+++ b/src/ProjectWindow.h
@@ -155,6 +155,7 @@ public:
 
    void OnMenu(wxCommandEvent & event);
    void OnUpdateUI(wxUpdateUIEvent & event);
+   void OnSysColourChanged(wxSysColourChangedEvent& event);
 
    void MacShowUndockedToolbars(bool show);
    void OnActivate(wxActivateEvent & event);

Saucedacity POC Patch:

diff --git a/src/ProjectWindow.cpp b/src/ProjectWindow.cpp
index b0a50edab..d07600b50 100644
--- a/src/ProjectWindow.cpp
+++ b/src/ProjectWindow.cpp
@@ -676,8 +676,25 @@ BEGIN_EVENT_TABLE(ProjectWindow, wxFrame)
    EVT_UPDATE_UI(1, ProjectWindow::OnUpdateUI)
    EVT_COMMAND(wxID_ANY, EVT_TOOLBAR_UPDATED, ProjectWindow::OnToolBarUpdate)
    //mchinen:multithreaded calls - may not be threadsafe with CommandEvent: may have to change.
+
+   EVT_SYS_COLOUR_CHANGED(ProjectWindow::OnSysColourChanged)
 END_EVENT_TABLE()
 
+void ProjectWindow::OnSysColourChanged(wxSysColourChangedEvent&)
+{
+   auto sysAppearance = wxSystemSettings::GetAppearance();
+
+   if (sysAppearance.IsDark())
+   {
+      theTheme.LoadTheme(themeDark);
+   } else
+   {
+      theTheme.LoadTheme(themeDefault);
+   }
+
+   ThemePrefs::ApplyUpdatedImages();
+}
+
 void ProjectWindow::ApplyUpdatedTheme()
 {
    auto &project = mProject;
diff --git a/src/ProjectWindow.h b/src/ProjectWindow.h
index 01f74a03e..c793f6f71 100644
--- a/src/ProjectWindow.h
+++ b/src/ProjectWindow.h
@@ -155,6 +155,7 @@ public:
 
    void OnMenu(wxCommandEvent & event);
    void OnUpdateUI(wxUpdateUIEvent & event);
+   void OnSysColourChanged(wxSysColourChangedEvent& event);
 
    void MacShowUndockedToolbars(bool show);
    void OnActivate(wxActivateEvent & event);

I think this works well for the theme switching mechanism. I personally am satisfied with this part of the code, but it is clear that we need to add a new preference for this.

[leio]

Looks like IsDark is only properly implemented on wxMSW and elsewhere it falls back to guessing based on background color. Ideally for GTK, it would check the light/dark preference over the modern dbus API first. Then again, this PR was at least initially only for wxMSW anyhow :)

@generic-pers0n if you're on IRC/Matrix, can you reach out about this or other things with me? I'm leio there too.

[n0toose]

So, after a little more research, my final POC: for wxWidgets 3.1.3 or later, we use wxSystemAppearance to tell if the system's appearance is dark or not

Today in "classes I wish I had known about from the beginning".

[n0toose]

Nevermind, I looked at my past notes, I only worked on Windows at first because of the 3.0 compatibility thing that distros like Debian or Void Linux still depend on. My focus was to get it to work first and polish it up later.

You could borrow #if wxCHECK_VERSION(3, 1, 1) from here: https://github.com/tenacityteam/tenacity/pull/300

[leio]

Meanwhile wxWidgets-3.2 finally came out, so it might be OKish to depend on it. Might be a blocker for older distro versions, but they wouldn't package a new application anyways either.
Out of #300, the split out #569 bits are useful on their own as long as it's still wxWidgets, because it moves from a deprecated widget to the modern one - getting it work with the checkbox feature in 3.0 too from that is just a bonus.

[generic-pers0n]

Looks like IsDark is only properly implemented on wxMSW and elsewhere it falls back to guessing based on background color. Ideally for GTK, it would check the light/dark preference over the modern dbus API first. Then again, this PR was at least initially only for wxMSW anyhow :)

@generic-pers0n if you're on IRC/Matrix, can you reach out about this or other things with me? I'm leio there too.

Indeed I'm willing to talk about this! But what do you know? It turns out we rely on having to check the background color on other platforms. This was what I was thinking as a fallback for wxWidgets 3.0 initially.

A little mini rant about our case and wxWidgets compatibility

I mean, wxSystemAppearance::IsDark would be an ideal API in theory, and the fallback code for wxWidgets 3.0 would be checking against the background color. But...on wxGTK, that sounds redundant. Either you check the background color in wx3.0 or do practically the same thing in wx3.{1, 2}. I mean, it could work for cross-platform compatibility, but how well it would work on wxGTK, it might be fairly limited. What if the system's theme is neither light or dark? What if it's orange or purple? (Okay, I guess I'm overreacting here, but maybe there are some good points to get across. Or maybe not, that's up for you to decide).

On an unrelated note, I think this acts as another reason as to why I wanted to take a look at an alternative toolkit (namely Qt), which I have plans for. But let's not talk about that here. That's for another issue.

[generic-pers0n]

Meanwhile wxWidgets-3.2 finally came out, so it might be OKish to depend on it. Might be a blocker for older distro versions, but they wouldn't package a new application anyways either.

You have a point. I would be using #ifdefs to maintain compatibility with wx3.0 as @n0toose has shown.

Edit: My Matrix username is @generic-person:matrix.org in case anyone wants to contact me through there.

[generic-pers0n]

So a new proposal based upon some talks with @leio:

• On Linux, we can use DBus to get a preference (org.freedesktop.appearance) in place of using wxSystemAppearance::IsDark() since this appears to be a better method. This can be contributed to wxWidgets, making it an enhancement for upstream wxWidgets.

  • This might be better than wxWidget's currently implementation. We can possibly upstream this for wxWidgets 3.2.

• Others: use wxSystemAppearance::IsDark() for wxWidgets >= 3.1.3 in OnSysColourChanged (wxSystemAppearance only exists in these versions). Otherwise, we use @n0toose's code for Windows, etc.

For DBus, we probably might need to add a new dependency for using DBus, maybe GDBus given we use wxGTK on Linux. Additionally, I still have to learn DBus, so that's out of my skillset for now (unless @leio is willing to take up the challenge, but I don't know). Everything else I am capable of doing, and we already have the Windows code for wxWidgets <= 3.1.3.

Also note: I'm currently looking at this for Saucedacity, so keep that in mind. We could always merge this into Saucedacity in the future, but that might be for a later date.

[leio]

I don't think you should worry at all about 3.0 for non-wxGTK, because on Mac and Windows we would be building and distributing a final binary ourselves, and that on top of 3.2. It's just Linux with wxGTK where there's some value in keeping compatibility with 3.0 due to wxWidgets distribution packages for some time.
I think at this point it would be fine to require 3.2 at minimum too, now that a stable release is out, but I don't consider that up to me (plus I did do all that work already afterall, back when 3.2 release was still completely up in the air from wxWidgets upstream). However, I do suggest requiring 3.2 as soon as it's just too much work to keep it going - so far it really wasn't, as the hard part was something that was better to be ported to something 3.0 had too in any case (the wxDataViewCtrl instead of deprecated wxListCtrl that only got checkbox support in 3.2, but wxData* had it in 3.0 already).

Anyhow, got a bit into rambling territory. tl;dr: I think it's very fine to require wx3.2 for Mac and Windows, and somewhat fine to require for linux/bsd (wxGTK) - users of distros without 3.2 packaged can use from the flatpak.

[generic-pers0n]

I don't think you should worry at all about 3.0 for non-wxGTK, because on Mac and Windows we would be building and distributing a final binary ourselves, and that on top of 3.2. It's just Linux with wxGTK where there's some value in keeping compatibility with 3.0 due to wxWidgets distribution packages for some time. I think at this point it would be fine to require 3.2 at minimum too, now that a stable release is out, but I don't consider that up to me (plus I did do all that work already afterall, back when 3.2 release was still completely up in the air from wxWidgets upstream). However, I do suggest requiring 3.2 as soon as it's just too much work to keep it going - so far it really wasn't, as the hard part was something that was better to be ported to something 3.0 had too in any case (the wxDataViewCtrl instead of deprecated wxListCtrl that only got checkbox support in 3.2, but wxData* had it in 3.0 already).

Anyhow, got a bit into rambling territory. tl;dr: I think it's very fine to require wx3.2 for Mac and Windows, and somewhat fine to require for linux/bsd (wxGTK) - users of distros without 3.2 packaged can use from the flatpak.

TL;DR: I agree with raising the minimum version of wxWidgets to 3.2. I believe it provides many things where 3.0 lacked before (especially wxSystemAppearance in our case). Linux/BSD are a different discussion due to the uneven wxWidgets distribution, but

Technically, for Saucedacity, I did already announce that the minimum required version for wxWidgets would be wxWidgets 3.1(.3) going forward on all platforms (including wxGTK). I had planned that Saucedacity would later support the latest development version of wxWidgets (3.3.0) also, and this was because of the uneven distribution of wxWidgets on Linux distros (we would use the appropriate version of wxWidgets for Windows and macOS, depending on what wxWidgets recommends themselves, e.g., they encourage 3.3.0 in production use like they did for 3.1.0). Therefore, I agree with raising the minimum required version of wxWidgets to 3.2.0. EDIT: Just another note to throw in there: Saucedacity will not build against wxWidgets 3.0 due to breaking changes between the two. I tested this once and Saucedacity failed to build. At this point, for Saucedacity, it would be safe to proceed with moving forward with wxWidgets 3.2.

I actually believe in raising the minimum version of wxWidgets to the latest stable. I believe that 3.2 is a much needed updated to wxWidgets and while there are clearly still it's shortcomings, it is better than using wxWidgets 3.0.