Ability to force a theme on Windows #1666

VZout · 2020-08-20T14:24:21Z

Added the WindowBuilderExtWindows::with_theme function to allow forcing a theme. In case you always want to have a dark theme no matter the system settings.

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
~~Created or updated an example program if it would help users understand this functionality~~
Updated feature matrix, if new features were added or implemented

…ferences

…eme_v1

repi · 2020-10-01T21:52:21Z

Any feedback about this or merging it in?

chrisduerr · 2020-10-01T21:55:41Z

I don't think there's any Windows maintainer capable of reviewing this.

filnet · 2020-10-02T08:14:04Z

src/platform_impl/windows/window_state.rs

@@ -32,6 +32,7 @@ pub struct WindowState {
    pub modifiers_state: ModifiersState,
    pub fullscreen: Option<Fullscreen>,
    pub is_dark_mode: bool,
+    pub forced_theme: Option<Theme>,


would it make sense to get rid of is_dark_mode and use only theme ?
probably a breaking change.

is_dark is purely to check OS preference.

An approach could be extending the theme enum to be something like:

enum Theme { OS_DETERMINED, FORCED_DARK, FORCED_LIGHT, }

and OS_DETERMINED would be the default.

However this does not get rid of is_dark because using dark style can fail and it is used to check the OS preference.

msiglreith

Tested on win10:

None: Correctly shows the theme selected in the OS
Some(theme): Correctly overrides the theme set by the user
None + theme switch: Correctly switches to OS theme
Some(theme) + theme swtich: Correctly keeps the forced theme

Good to go imo if there are no further code related concerns

kchibisov

I don't have windows machine to test it, but I'd do some source level review, since I have some style questions.

CHANGELOG.md

FEATURES.md

kchibisov · 2020-10-10T17:46:07Z

src/platform/windows.rs

@@ -125,6 +125,9 @@ pub trait WindowBuilderExtWindows {
    /// If you need COM API with `COINIT_MULTITHREADED` you must initialize it before calling any winit functions.
    /// See https://docs.microsoft.com/en-us/windows/win32/api/objbase/nf-objbase-coinitialize#remarks for more information.
    fn with_drag_and_drop(self, flag: bool) -> WindowBuilder;
+
+    /// Forces either Dark or Light theme. By default the system settings are used.


Just by looking on that function, I feel like it'll make sense to accept Option<Theme>, meaning that providing None will use system's preferred Theme. That seems more ergonomic and can interact better with downstream configurations, since otherwise they should avoid calling that method, what do you think?

Good feedback. I've decided to replace Option<> with a Theme::System to be a bit more explicit.
Let me know if you disagree :)

src/platform_impl/windows/mod.rs

…eme_v1

src/platform_impl/windows/dark_mode.rs

src/window.rs

src/platform_impl/windows/event_loop.rs

…force_theme_v1

filnet · 2020-10-15T16:15:25Z

src/platform_impl/windows/dark_mode.rs

    if *DARK_MODE_SUPPORTED {
-        let is_dark_mode = should_use_dark_mode();
+        let is_dark_mode: bool = match preferred_theme {


do you really need to specify the bool type ?

src/platform_impl/windows/event_loop.rs

src/platform_impl/windows/window.rs

kchibisov

That looks better and I feel like we're getting closer! Thanks for your work so far.

src/platform/windows.rs

kchibisov · 2020-10-17T16:50:43Z

src/platform_impl/windows/window_state.rs

@@ -32,6 +32,7 @@ pub struct WindowState {
    pub modifiers_state: ModifiersState,
    pub fullscreen: Option<Fullscreen>,
    pub is_dark_mode: bool,


I feel like for the more code simplicity we can use Theme instead of bool here, so try_dark_mode will also return a Theme, and then you'd have less mapping to bool -> Theme , issuing a new ThemeChanged when current_theme != new_theme?

Yeah I like this changed. Implemented it!

Co-authored-by: Kirill Chibisov <contact@kchibisov.com>

kchibisov

You should address compilation warnings, besides that it should be good to go.

src/platform_impl/windows/dark_mode.rs

kchibisov · 2020-11-21T19:21:31Z

src/platform_impl/windows/dark_mode.rs

@@ -72,9 +74,12 @@ lazy_static! {

 /// Attempt to set dark mode on a window, if necessary.
 /// Returns true if dark mode was set, false if not.
-pub fn try_dark_mode(hwnd: HWND) -> bool {
+pub fn try_dark_mode(hwnd: HWND, preferred_theme: Option<Theme>) -> Theme {


I'd actually rename this function, since it it's not about dark mode anymore, it's just about trying to get a theme, however when I see try, I'm expecting Result, so I'm not sure if this is a good name. Maybe pick_theme or something like that, if you have better ideas let me know.

if your preferred theme is dark it could theoretically fail on old windows versions. In that case it would return Theme::Light.
So I think try should be included in the name. try_select_theme, try_pick_theme or try_theme 🤷

I don't think it should necessarily return a result. I could change it to a result but it would make the calling code less nice IMO. (It would return a result, if the result is ok the current theme is the preferred theme if the result is a error than the theme is light. Which works but the caller of try_dark_mode needs to understand that the standard theme is light so I prefer to hide it away into the try_ function)

CHANGELOG.md

Co-authored-by: Kirill Chibisov <contact@kchibisov.com>

kchibisov

Needs rebase and should be good to go.

msiglreith · 2020-11-28T16:38:07Z

src/platform/windows.rs

@@ -82,7 +82,7 @@ pub trait WindowExtWindows {
    fn set_taskbar_icon(&self, taskbar_icon: Option<Icon>);

    /// Whether the system theme is currently Windows 10's "Dark Mode".


Documentation doesn't match anymore

Changed the comment

msiglreith · 2020-11-28T16:38:44Z

src/platform/windows.rs

@@ -82,7 +82,7 @@ pub trait WindowExtWindows {
    fn set_taskbar_icon(&self, taskbar_icon: Option<Icon>);

    /// Whether the system theme is currently Windows 10's "Dark Mode".
-    fn is_dark_mode(&self) -> bool;
+    fn theme(&self) -> Theme;


This seems like a breaking change, so it should be listed in the changelog

Changed the changelog

CHANGELOG.md

Co-authored-by: msiglreith <m.siglreith@gmail.com>

VZout added 2 commits August 20, 2020 15:53

You can now force either dark or light theme no matter the system pre…

486798a

…ferences

Clean up and fixed a freeze when switching at runtime

c80974e

VZout mentioned this pull request Aug 20, 2020

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

VZout and others added 5 commits August 20, 2020 16:28

Update FEATURES.md

1d045f3

Update windows.rs

fb78110

Update CHANGELOG.md

b09b464

cargo fmt

45592dd

Merge branch 'force_theme_v1' of github.com:VZout/winit into force_th…

d92a4bd

…eme_v1

VZout marked this pull request as ready for review August 20, 2020 14:34

Update CHANGELOG.md

d210aa8

chrisduerr requested a review from Osspial October 1, 2020 21:55

filnet reviewed Oct 2, 2020

View reviewed changes

msiglreith approved these changes Oct 10, 2020

View reviewed changes

kchibisov requested changes Oct 10, 2020

View reviewed changes

msiglreith added DS - windows C - waiting on author Waiting for a response or another PR labels Oct 13, 2020

VZout added 6 commits October 13, 2020 15:42

Merge branch 'master' into force_theme_v1

3e9895b

Incorporated feedback

4b14626

fixed a comment

34ee46e

Merge branch 'force_theme_v1' of github.com:VZout/winit into force_th…

885e66b

…eme_v1

Changelog update

1b6436f

fmt

7ca4bb7

VZout requested a review from kchibisov October 13, 2020 14:29

kchibisov requested changes Oct 14, 2020

View reviewed changes

src/platform_impl/windows/dark_mode.rs Outdated Show resolved Hide resolved

src/window.rs Outdated Show resolved Hide resolved

src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved

VZout and others added 4 commits October 14, 2020 15:37

Merge branch 'master' into force_theme_v1

2fdfd80

Incorporated feedback

9d347bb

Merge branch 'master' into force_theme_v1

6762d84

Inlined the changed param

5a79a2f

Merge branch 'force_theme_v1' of https://github.com/VZout/winit into …

e8ff688

…force_theme_v1

VZout requested a review from kchibisov October 15, 2020 14:22

filnet reviewed Oct 15, 2020

View reviewed changes

src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved

filnet reviewed Oct 15, 2020

View reviewed changes

src/platform_impl/windows/window.rs Outdated Show resolved Hide resolved

kchibisov requested changes Oct 17, 2020

View reviewed changes

VZout and others added 4 commits November 16, 2020 11:15

Update src/platform/windows.rs

43d3959

Co-authored-by: Kirill Chibisov <contact@kchibisov.com>

feedback

32ca08b

merged

452ebec

kchibisov's suggestion

653efc2

VZout requested a review from kchibisov November 16, 2020 10:29

Merge branch 'master' into force_theme_v1

3984155

kchibisov requested changes Nov 21, 2020

View reviewed changes

VZout and others added 4 commits November 23, 2020 15:19

Update CHANGELOG.md

aa1248c

Co-authored-by: Kirill Chibisov <contact@kchibisov.com>

reordered changelog

9c797ad

Update dark_mode.rs

d27490d

renamed try_dark_mode to try_theme and fixed a unused import

33ea8f4

VZout requested a review from kchibisov November 26, 2020 10:54

kchibisov approved these changes Nov 27, 2020

View reviewed changes

VZout added 3 commits November 27, 2020 11:27

Merge branch 'master' into force_theme_v1

89c8a11

fmt

2d9e151

fixed function order

bcb41f8

msiglreith reviewed Nov 28, 2020

View reviewed changes

VZout added 2 commits November 30, 2020 17:07

Update windows.rs

a5dd089

Merge branch 'master' into force_theme_v1

1ef38f0

msiglreith reviewed Nov 30, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

fe258f6

Co-authored-by: msiglreith <m.siglreith@gmail.com>

msiglreith merged commit 6ddee9a into rust-windowing:master Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to force a theme on Windows #1666

Ability to force a theme on Windows #1666

VZout commented Aug 20, 2020 •

edited

repi commented Oct 1, 2020

chrisduerr commented Oct 1, 2020

filnet Oct 2, 2020

VZout Oct 5, 2020 •

edited

msiglreith left a comment

kchibisov left a comment

kchibisov Oct 10, 2020

VZout Oct 13, 2020

filnet Oct 15, 2020

kchibisov left a comment

kchibisov Oct 17, 2020

VZout Nov 16, 2020

kchibisov left a comment

kchibisov Nov 21, 2020

VZout Nov 23, 2020

kchibisov left a comment

msiglreith Nov 28, 2020

VZout Nov 30, 2020

msiglreith Nov 28, 2020

VZout Nov 30, 2020 •

edited

		@@ -82,7 +82,7 @@ pub trait WindowExtWindows {
		fn set_taskbar_icon(&self, taskbar_icon: Option<Icon>);

		/// Whether the system theme is currently Windows 10's "Dark Mode".

Ability to force a theme on Windows #1666

Ability to force a theme on Windows #1666

Conversation

VZout commented Aug 20, 2020 • edited

repi commented Oct 1, 2020

chrisduerr commented Oct 1, 2020

Choose a reason for hiding this comment

VZout Oct 5, 2020 • edited

Choose a reason for hiding this comment

msiglreith left a comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VZout Nov 30, 2020 • edited

Choose a reason for hiding this comment

VZout commented Aug 20, 2020 •

edited

VZout Oct 5, 2020 •

edited

VZout Nov 30, 2020 •

edited