MSW: Dark mode switching (PR #26516)
Steve Cornett
A mechanism is introduced to support switching between dark and light mode according to the system-wide setting.
The new function MSWUpdateDarkMode() replaces MSWGetDarkModeSupport() for tailoring dark mode settings. MSWUpdateDarkMode() performs the actions needed upon window creation, switching from dark to light, and vice versa. MSWUpdateDarkMode() is called during window creation and also when switching modes.
MSWUpdateDarkMode() is more general than MSWGetDarkModeSupport(). It is not limited to changing the window theme and foreground color. MSWUpdateDarkMode() is defined in wxWindowMSW rather than wxControl so that any window type can be adapted to switch modes.
Because dark mode switching involves changing colors, there is a chance of overwriting background or foreground colors or other attributes that were set by the user. The new function wxMSWDarkMode::IsChanging() is used to avoid making settings that are needed only for switching modes and not needed for window creation. Therefore, the dark mode behavior is intended to be the same as it was before when no mode switching occurs. In any case, the end user can override MSWUpdateDarkMode() to circumvent unwanted side effects.
The function wxMSWDarkMode::AllowForWindow() now sends WM_THEMECHANGED to the window. I am not able to reproduce some problems described in comments and I attribute the improvement to this change. For example, wxRadioButton and wxStaticText now need no special customization. They just work.
For some window types there may be dark mode settings that cannot feasibly be switched after creation. Some applications may not be able to fully switch modes. But some applications will.
This stage adds switching to the windows that defined MSWGetDarkModeSupport(), plus a couple others. Later, additional window types can be adapted by implementing MSWUpdateDarkMode().
I tested primarily on Windows 11 25H2 with some limited testing on Windows 10 H2.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26516
Commit Summary
- 1a15761 Darkmode switching
File Changes
(28 files)
- M include/wx/msw/checkbox.h (3)
- M include/wx/msw/choice.h (3)
- M include/wx/msw/control.h (24)
- M include/wx/msw/listctrl.h (3)
- M include/wx/msw/notebook.h (3)
- M include/wx/msw/private/darkmode.h (8)
- M include/wx/msw/radiobut.h (2)
- M include/wx/msw/statbox.h (3)
- M include/wx/msw/stattext.h (3)
- M include/wx/msw/statusbar.h (3)
- M include/wx/msw/textctrl.h (3)
- M include/wx/msw/toolbar.h (3)
- M include/wx/msw/window.h (7)
- M src/msw/checkbox.cpp (15)
- M src/msw/choice.cpp (12)
- M src/msw/control.cpp (32)
- M src/msw/darkmode.cpp (71)
- M src/msw/headerctrl.cpp (23)
- M src/msw/listctrl.cpp (56)
- M src/msw/notebook.cpp (35)
- M src/msw/radiobut.cpp (13)
- M src/msw/statbox.cpp (9)
- M src/msw/stattext.cpp (9)
- M src/msw/statusbar.cpp (7)
- M src/msw/textctrl.cpp (78)
- M src/msw/toolbar.cpp (43)
- M src/msw/toplevel.cpp (3)
- M src/msw/window.cpp (30)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26516.patch
- https://github.com/wxWidgets/wxWidgets/pull/26516.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 1 commit.
- 86d2a43 Merge branch 'master' into darkmode-switching
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this is definitely going to be useful, but I have a couple of questions and wanted to ask them already, even though I didn't look at all the changes yet.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
I am partial, of course, but I think that the semantics of the old function, with it being a callback defined in derived classes filling in MSWDarkModeSupport struct, was more natural and avoided many classes overriding this function and completely or partially ignoring its parameters.
I also think that using MSWDarkModeSupport was preferable, its removal means we have to set foreground colour explicitly in several places instead of just setting the flag.
Are there any good reasons to change it like you did? And, if not, would it be possible to revert to the old semantics?
> @@ -61,8 +61,6 @@ class WXDLLIMPEXP_CORE wxRadioButton : public wxMSWOwnerDrawnButton<wxRadioButto
virtual wxBorder GetDefaultBorder() const override { return wxBORDER_NONE; }
virtual wxSize DoGetBestSize() const override;
- virtual bool MSWGetDarkModeSupport(MSWDarkModeSupport& support) const override;
Has this been removed (and not replaced with overriding the new function) intentionally?
> - return true; + // Native control does not show correct text color. Enable custom drawing.
Shouldn't this be done only when switching to dark mode but not from it? Or did I misunderstand and this function is not called in the latter case?
> // Static boxes don't seem to have any dark mode support, so just set the
// foreground colour contrasting with the dark background for them.
- support.setForeground = true;
-
- return true;
+ SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_BTNTEXT));
This probably was the case before, but it seems to be wrong to set the foreground colour for the same reason it was wrong to do it for wxFrame, i.e. I think it should be enough to return the correct colour from GetDefaultAttributes(), shouldn't it?
> + // For top level window, update non-client area + if ( IsTopLevel() ) + wxMSWDarkMode::ConfigureTLW(GetHwnd());
It would be cleaner to do this in wxNonOwnedWindow::MSWUpdateDarkMode(). And probably more correct too as some windows (TDI child frames?) return true from IsTopLevel() without being actual TLWs at Windows level.
> @@ -214,6 +214,15 @@ wxStaticText::MSWHandleMessage(WXLRESULT *result,
return false;
}
+void wxStaticText::MSWUpdateDarkMode(const wchar_t* themeName,
+ const wchar_t* themeId)
+{
+ wxStaticTextBase::MSWUpdateDarkMode(themeName, themeId);
+ // Text color does not respond when switching to light mode.
Sorry, what is the meaning of this comment? Maybe it's just me, but I simply can't figure it out.
> @@ -2948,6 +2927,63 @@ void wxTextCtrl::MSWSetRichZoom()
::SendMessage(GetHWND(), EM_SETZOOM, (WPARAM)num, (LPARAM)denom);
}
+void wxTextCtrl::MSWUpdateDarkMode(const wchar_t* themeName,
+ const wchar_t* themeId)
+{
+ wxTextCtrlBase::MSWUpdateDarkMode(themeName, themeId);
+
+#if wxUSE_RICHEDIT
+ if ( IsRich() )
+ {
+ // We need to set the colours explicitly for rich text controls.
+ SetBackgroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOW));
+ SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT));
+
+ // For single-line style, background does not respond when switching
+ // to light mode. Reset the font background.
Won't this lose any custom background for ranges of text?
> + if (wxMSWDarkMode::IsActive()) {
+ style |= WS_BORDER;
+ } else {
+ style &= ~WS_BORDER;
+ }
Minor style nitpick:
⬇️ Suggested change- if (wxMSWDarkMode::IsActive()) {
- style |= WS_BORDER;
- } else {
- style &= ~WS_BORDER;
- }
+ if (wxMSWDarkMode::IsActive())
+ {
+ style |= WS_BORDER;
+ }
+ else
+ {
+ style &= ~WS_BORDER;
+ }
I also wonder if we could use ToggleWindowStyle() here with some border style?
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive;
I'd prefer to initialize it explicitly, even if it's not needed:
⬇️ Suggested change-bool gs_isActive; +bool gs_isActive = false;
> }
void AllowForWindow(HWND hwnd, const wchar_t* themeName, const wchar_t* themeId)
{
- if ( !wxMSWImpl::ShouldUseDarkMode() )
It looks like removing this check results in WM_THEMECHANGED being always sent now.
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive; + +// The return value for IsChanging(). We never bother resetting this variable
I understand the explanation/rationale, but it seems wrong to call this IsChanging, it should be something like DarkModeHasBeenEnabled or something like this.
> @@ -753,6 +796,12 @@ HandleMenuMessage(WXLRESULT* result,
return false;
}
+void HandleSysColorChange()
I would call this NotifySysColorChange() because it doesn't really handle anything.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
I didn't quite follow your concern. But the MSWDarkModeSupport struct is just very restrictive - only allowing modifying three aspects of a window. Some controls need much more modification. For example, wxTextControl needs a whole page of code that is very specific to that control. For another example, in src/msw/choice.cpp at the comment "// It is slightly improper to do this in a const function" you can see that control needs special modification that is not addressed by the MSWDarkModeSupport. Basically the new mechanism allows any control to do whatever it needs without having to conform to a template.
Regarding needing to set the foreground color in several places, that is true. However, some windows may only need to set the foreground color if the mode is changing, not always. Some need to also set the background color. Some need to set one color one way and another the other. The variations are endless.
This new method seems more natural to me. Generally when a class needs to allow derived classes to do something special, it defines a virtual function and the derived classes override it. They do the special thing they need, and call their base class to do the common stuff.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -61,8 +61,6 @@ class WXDLLIMPEXP_CORE wxRadioButton : public wxMSWOwnerDrawnButton<wxRadioButto
virtual wxBorder GetDefaultBorder() const override { return wxBORDER_NONE; }
virtual wxSize DoGetBestSize() const override;
- virtual bool MSWGetDarkModeSupport(MSWDarkModeSupport& support) const override;
Yes this is removed intentionally. AllowForWindow() now sends WM_THEMECHANGED to the window. For some controls like this one, that improved their responsiveness to changing the dark/light mode. In this case, no special modification is needed. The control updates properly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> - return true; + // Native control does not show correct text color. Enable custom drawing.
In general the same actions need to be done whether switching from light to dark or dark to light except for
wxMSWImpl::AllowDarkModeForWindow(). That really only needs to be called switching to dark. We could add an if-statement around it, but we would see no change in behavior. Calling it twice doesn't hurt.
But there are other changes, for example SetWindowTheme() that should be done going both ways. Some controls need to be kicked more than others and it's difficult to predict which need what. The underlying Windows controls are inconsistent and stubborn. Some wxWidgets controls are complicated. I kept it simple by generally doing the same actions going both ways. But we certainly could do experiments and add if-statements so we only do what is absolutely necessary.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
What bothers me is that many classes ignore the parameters. Maybe it would be fine if it were called without them, but upcalling a virtual function with some parameters which then calls the base class with different parameters seems to be really confusing to me and I'd very much like to avoid this, one way or the other.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> - return true; + // Native control does not show correct text color. Enable custom drawing.
Sorry if I was unclear, but the point was that we should avoid using custom draw when not using dark mode because relying on the native behaviour when possible is always better, e.g. because it could gain some additional functionality which wouldn't be provided by our custom drawing function in future Windows versions (or, more prosaically, because our version could have bugs). We don't have any choice about this in dark mode, but in light mode we shouldn't use it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> // Static boxes don't seem to have any dark mode support, so just set the
// foreground colour contrasting with the dark background for them.
- support.setForeground = true;
-
- return true;
+ SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_BTNTEXT));
Anywhere there was support.setForeground=true, that resulted in a SetForegroundColour(). So that's what I did. I'll look through all the color settings to see if there is a better way. But this is no worse that it was before, so I don't think that should be a requirement for now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> + // For top level window, update non-client area + if ( IsTopLevel() ) + wxMSWDarkMode::ConfigureTLW(GetHwnd());
Will do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> // Static boxes don't seem to have any dark mode support, so just set the
// foreground colour contrasting with the dark background for them.
- support.setForeground = true;
-
- return true;
+ SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_BTNTEXT));
No, it isn't, as I said, this must have already been the problem before. The only potential new thing is if this now gets done even in the light mode, but I'm still not quite sure if this is going to be executed for it if it's the starting state.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> + if (wxMSWDarkMode::IsActive()) {
+ style |= WS_BORDER;
+ } else {
+ style &= ~WS_BORDER;
+ }
Will fix. I don't see how ToggleWindowStyle() will help since we don't want the previous state to affect the result. We just want to turn this style on or off.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive; + +// The return value for IsChanging(). We never bother resetting this variable
This will be invoked with the wxMSWDarkMode:: prefix, so maybe shorter and "Active" rather than "Enabled" for consistency with IsActive(). So wxMSWDarkMode::HasBeenActive()?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
> @@ -753,6 +796,12 @@ HandleMenuMessage(WXLRESULT* result,
return false;
}
+void HandleSysColorChange()
Will do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive; + +// The return value for IsChanging(). We never bother resetting this variable
Yes, this is perfectly fine, thanks. Anything goes as long as it doesn't imply that it's being changed right now, as the current name does.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> - return true; + // Native control does not show correct text color. Enable custom drawing.
Unfortunately the Windows native header control is one of the worst with dark mode. The text color always stays as it was created. We have no choice but to use the custom drawing both for dark mode, and for switching out of dark mode.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> - return true; + // Native control does not show correct text color. Enable custom drawing.
Ah, I see, thanks. In this case we just need to explain this in the comment.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> // Static boxes don't seem to have any dark mode support, so just set the
// foreground colour contrasting with the dark background for them.
- support.setForeground = true;
-
- return true;
+ SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_BTNTEXT));
This SetForegroundColour() does not occur if the app starts in light mode and stays in light mode. The MSWUpdateDarkMode() function only gets called when the app starts in dark mode, or switches modes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive;
Will do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
> }
void AllowForWindow(HWND hwnd, const wchar_t* themeName, const wchar_t* themeId)
{
- if ( !wxMSWImpl::ShouldUseDarkMode() )
Yes, WM_THEMECHANGED needs to be sent when switching modes, both ways.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
So we keep the struct parameter containing the SetWindowTheme parameters but without the setForeground member. I propose renaming the struct from MSWDarkModeSupport to MSWDarkModeTheme.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -214,6 +214,15 @@ wxStaticText::MSWHandleMessage(WXLRESULT *result,
return false;
}
+void wxStaticText::MSWUpdateDarkMode(const wchar_t* themeName,
+ const wchar_t* themeId)
+{
+ wxStaticTextBase::MSWUpdateDarkMode(themeName, themeId);
+ // Text color does not respond when switching to light mode.
How about this:
// When switching to light mode, the text color remains the same. We must
// explicitly update the color.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
I think we might want 2 functions:
- Virtual
MSWEnableDarkMode(bool)which will be overridden in the derived classes to do whatever they need to do when switching modes. - Non-virtual
MSWDoSetDarkMode(const MSWDarkModeSupport&)which these derived functions could call to perform the common actions (including changing foreground).
Details can be changed, of course, e.g. I wonder if we shouldn't use some (scoped) enum instead of bool just in case some other mode is added in the future, but I believe things would be more clear with 2 functions rather than one.
What do you think?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
I've been working on a new idea and it seems good so far.
The MSWUpdateDarkMode() function should take no parameters. This removes the confusion you mentioned.
I don't know what the bool parameter you suggested would be for.
I don't think it helps to have a separate MSWDoSetDarkMode() function to do common changes because there really are no changes that are common to all windows at all levels. The actions really need to be highly customized in every window. Some do need to set the foreground color, but sometimes it needs to be done only when switching modes. And some need to set the background color, etc.
Instead of the confusing parameters I had before, we have two virtual overrides that specify the SetWindowTheme() arguments. This is pretty simple and uncluttered since most windows do not need to specify a special theme name, and only one needs to specify the ID.
virtual const wchar_t* MSWGetDarkModeThemeName() const
{
return L"Explorer";
}
virtual const wchar_t* MSWGetDarkModeThemeId() const
{
return nullptr;
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -618,6 +618,13 @@ class WXDLLIMPEXP_CORE wxWindowMSW : public wxWindowBase
{
}
+ // Update the window for dark mode for window creation or for switching
+ // between dark and light modes. The parameters are for SetWindowTheme().
+ // The general purpose theme name "Explorer" should be specified unless
+ // a specific theme is required.
+ virtual void MSWUpdateDarkMode(const wchar_t* themeName,
The MSWUpdateDarkMode() function should take no parameters. This removes the confusion you mentioned.
Yes, this is good, thank you.
I don't know what the bool parameter you suggested would be for.
I thought we wanted to call it when switching (back) to light mode too and this parameter was supposed to indicate the direction of the switch.
Instead of the confusing parameters I had before, we have two virtual overrides that specify the SetWindowTheme() arguments.
This is basically just MSWGetDarkModeSupport() replacement, of course, just with 2 functions instead of 1. I am not sure whether 2 functions or 1 is better, but considering we already have this one, I'd keep it and just remove setForeground from it if it's unnecessary now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive; + +// The return value for IsChanging(). We never bother resetting this variable
I renamed this function HasChanged(). We could also go with HasSwitched(). The key issue is this function tells whether the mode has changed or not. Some controls need to take extra actions to switch from light mode to dark mode compared with just creating the window in dark mode. This function helps us do only what is needed, to avoid overwriting any user-set attributes like colors.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -151,6 +151,19 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive; + +// The return value for IsChanging(). We never bother resetting this variable
Could you please copy the contents of your reply into a comment somewhere? This is very useful and not at all obvious from just reading the code. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> @@ -2948,6 +2927,63 @@ void wxTextCtrl::MSWSetRichZoom()
::SendMessage(GetHWND(), EM_SETZOOM, (WPARAM)num, (LPARAM)denom);
}
+void wxTextCtrl::MSWUpdateDarkMode(const wchar_t* themeName,
+ const wchar_t* themeId)
+{
+ wxTextCtrlBase::MSWUpdateDarkMode(themeName, themeId);
+
+#if wxUSE_RICHEDIT
+ if ( IsRich() )
+ {
+ // We need to set the colours explicitly for rich text controls.
+ SetBackgroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOW));
+ SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT));
+
+ // For single-line style, background does not respond when switching
+ // to light mode. Reset the font background.
I tried for hours but could not find a way to preserve customized colors. Basically the issue is that if we preserve customized colors, uncustomized text disappears. I think the only way to preserve everything properly is to destroy and recreate the control, which I do not think is realistic.
I will modify this to use native messages rather than the SetBackgroundColour/SetForegroundColour to reduce interference with the color related members.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 1 commit.
- 7513ce1 Remove MSWUpdateDarkMode() parameters and restore MSWGetDarkModeSupport()
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 1 commit.
- 74676b5 Avoid overwriting wxTextCtrl colors
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this looks good to me globally but I'd really like to rename the function because I find "Update" confusing, for me this implies that it's called on a control already in dark mode to tweak it or something like that, not that it is the function which puts the control into dark mode.
Also, seeing how wxMSWDarkMode::IsChanged(), I think it would make sense to give this function a parameter like this:
enum class SetMode { // Set dark mode for a newly created window. Initial, // Set dark mode for an existing window after switch to light mode. AfterChange }; virtual void MSWSetDarkMode(SetMode setmode);
as checking for mode == SetMode::AfterChange inside the function body would be more clear than current checks for HasChanged().
(for completeness, I also thought about having MSWSetDarkMode() and additional MSWSwitchToDarkMode() which would be only called, after MSWSetDarkMode(), when switching, but I think it's not worth splitting this into multiple functions).
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must
Shouldn't this say:
⬇️ Suggested change- // When switching to light mode, the text color remains the same. We must + // When switching from light mode, the text color remains the same. We must
?
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must + // explicitly update the color. + if ( wxMSWDarkMode::HasChanged() )
Maybe also test for !UseForegroundColour()? The idea being that if the application code changes colours (in light mode), then it's also responsible for updating them when the application switches to the dark mode.
Also, I'm not sure if this executes before or after the user-defined wxEVT_SYS_COLOUR_CHANGED handler. If it's done before, changing the colour before the application changes it to something else is simply useless, but if it's done after, it would be really bad because it would override the colour set by the application.
> + // Struct used for MSWGetDarkModeSupport() below.
+ // This specifies the arguments to the SetWindowTheme API for dark mode.
+ struct MSWDarkModeSupport
+ {
+ // The name of the theme to use (also called "app name").
+ const wchar_t* themeName = nullptr;
+
+ // The theme IDs to use. If neither this field nor the theme name is
+ // set, no theme is applied to the window.
+ const wchar_t* themeId = nullptr;
+ };
+
+ virtual void MSWGetDarkModeSupport(MSWDarkModeSupport& support) const;
+
+ // Update the window for dark mode window creation or for switching
+ // between dark and light modes.
Sorry, but I still have a lot of trouble understanding the comment and the function name: why does the former speak about switching between dark and light modes when this function is only called when enabling dark mode? I.e. "switching to dark mode".
And, similarly, why is it called "Update"? It doesn't seem to update anything, I'd call it just MSWEnableDarkMode().
> @@ -151,6 +151,12 @@ enum PreferredAppMode PreferredAppMode gs_appMode = AppMode_Default; +// The initial dark mode state. +bool gs_isActive = false;
Please bear me but this variable needs to be renamed as well, it's very confusing to call "isActive" something that does not correspond to the value returned by IsActive().
-bool gs_isActive = false; +bool gs_wasActiveOnStartup = false;
(or wasInitiallyActive or whatever, just not isActive).
>
- return true;
+void wxMSWHeaderCtrl::MSWUpdateDarkMode()
+{
+ wxControl::MSWUpdateDarkMode();
+
+ // The Windows native header control does not respond to changes in the
+ // dark/light mode. The text color always stays as it was created. We have
+ // no choice but to use the custom drawing both for dark mode.
Either "both" is superfluous or there is something lacking in this sentence.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> + // Struct used for MSWGetDarkModeSupport() below.
+ // This specifies the arguments to the SetWindowTheme API for dark mode.
+ struct MSWDarkModeSupport
+ {
+ // The name of the theme to use (also called "app name").
+ const wchar_t* themeName = nullptr;
+
+ // The theme IDs to use. If neither this field nor the theme name is
+ // set, no theme is applied to the window.
+ const wchar_t* themeId = nullptr;
+ };
+
+ virtual void MSWGetDarkModeSupport(MSWDarkModeSupport& support) const;
+
+ // Update the window for dark mode window creation or for switching
+ // between dark and light modes.
This function modifies the window with whatever is needed going both into and out of dark mode. So it handles both enabling and disabling dark mode. I didn't want to name it like "EnableOrDisable." I'm open to suggestions. I will add more comments and make the name MSWChangeDarkMode().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must
The comment is correct. I'll add more explanation.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + // Struct used for MSWGetDarkModeSupport() below.
+ // This specifies the arguments to the SetWindowTheme API for dark mode.
+ struct MSWDarkModeSupport
+ {
+ // The name of the theme to use (also called "app name").
+ const wchar_t* themeName = nullptr;
+
+ // The theme IDs to use. If neither this field nor the theme name is
+ // set, no theme is applied to the window.
+ const wchar_t* themeId = nullptr;
+ };
+
+ virtual void MSWGetDarkModeSupport(MSWDarkModeSupport& support) const;
+
+ // Update the window for dark mode window creation or for switching
+ // between dark and light modes.
Oh, sorry for misunderstanding when it is called.
The name makes more sense if it's called for the switch to light mode too, but in this case I'd rather call it MSWUpdateMode() or MSWSwitchMode(), i.e. without "dark". Or if we want to have "dark" in the name, then have "light" too, e.g. MSWSetLightOrDarkMode().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must + // explicitly update the color. + if ( wxMSWDarkMode::HasChanged() )
If we add the !UseForegroundColour() condition, then the code will only properly handle switching modes one time because this function sets the foreground color. If the theme changes and, then changes back again this function will find that the foreground color is always set, and will do nothing - leaving the text color in the wrong state. To handle this properly requires a way to know whether the foreground color was set by the user or set internally. Unfortunately, we don't have that as the SetForegroundColour() and SetOwnForegroundColour() both set m_hasFgCol. That is an issue for another day.
This is an excellent example of why dark mode switching will not be perfect for all apps. This is a trade-off. Apps that set custom colors will not look quite right upon a theme switch (but perhaps overall better than they do now). Apps that do not use custom colors will look good.
Of course, the end user can always provide an override of this function to customize the colors.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must + // explicitly update the color. + if ( wxMSWDarkMode::HasChanged() )
Oops, of course you're right about UseFgCol() not working if we use SetFgCol(), sorry.
But I still think we can make this work correctly, we just shouldn't call SetFgCol(): probably just setting m_foregroundColour directly would work, wouldn't it (if do it only if !UseFgCol())?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must + // explicitly update the color. + if ( wxMSWDarkMode::HasChanged() )
Great idea but I find that the foreground color was already set by wxWindowBase::InheritAttributes() and so the test !UseForegroundColour() is always false here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 1 commit.
- d0b7600 Rename MSWUpdateDarkMode() to MSWSwitchMode()
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 2 commits.
- 009fec2 Merge remote-tracking branch 'upstream/master' into darkmode-switching
- f8d517d fix wxListBox::MSWGetDarkModeSupport() return type
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> - support.setForeground = true; - - return true; + // When switching to light mode, the text color remains the same. We must + // explicitly update the color. + if ( wxMSWDarkMode::HasChanged() )
Sorry for coming up with so many great ideas none of which is working...
This definitely won't be part of this PR but I think we need to change InheritAttributes() to avoid overwriting m_hasFgCol. And remove the check for the latter from wxWindowBase::GetForegroundColour() (it's funny that the comment there says that the logic in this function is the same as in GetBackgroundColour() but in fact this extra check makes it different).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 1 commit.
- 77c0dd2 Rename MSWSwitchMode() to MSWSetDarkOrLightMode() and add SetMode parameter
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor commented on this pull request.
> +
+ // The theme IDs to use. If neither this field nor the theme name is
+ // set, no theme is applied to the window.
+ const wchar_t* themeId = nullptr;
+ };
+
+ virtual void MSWGetDarkModeSupport(MSWDarkModeSupport& support) const;
+
+ // The reason for calling MSWSetDarkOrLightMode below.
+ enum class SetMode
+ {
+ // Set dark mode for a newly created window.
+ Initial,
+
+ // Set dark mode or light mode for an existing window.
+ Change
I named this value "Change" rather than "ChangeAfter" because the word "after" implies timing that is not necessarily true. There are actually two WM_SETTINGCHANGE "ImmersiveColorSet" messages, one that occurs before the switch and one after. When the mode is changing, MSWSetDarkOrLightMode() can get called once or twice depending on whether the mode ever changed before. Those details are complicated but irrelevant. Upon "Change", the function should just update the dark/light mode.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this looks good to me except for one last question below.
> @@ -4103,14 +4103,7 @@ bool wxWindowMSW::MSWCreate(const wxChar *wclass,
}
if ( wxMSWDarkMode::IsActive() )
- {
- // We currently allow customizing the theme at wxControl level as some
- // native controls require using a different theme, but for plain
- // windows it looks like the default ("Explorer") should always be used
- // and its only (but important) effect is to make their scrollbars
- // dark, if they're used.
- wxMSWDarkMode::AllowForWindow(m_hWnd);
- }
+ MSWSetDarkOrLightMode(SetMode::Change);
Is Change used intentionally here? I think it should be Initial, but if this is wrong, could you please add a comment explaining why?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Steve Cornett
@stevecor pushed 1 commit.
- 6020bca Fix MSWSetDarkOrLightMode() call in MSWCreate()
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.