Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Ability to force a theme on Windows #1666
Ability to force a theme on Windows #1666
Changes from 24 commits
486798a
c80974e
1d045f3
fb78110
b09b464
45592dd
d92a4bd
d210aa8
3e9895b
4b14626
34ee46e
885e66b
1b6436f
7ca4bb7
2fdfd80
9d347bb
6762d84
5a79a2f
e8ff688
43d3959
32ca08b
452ebec
653efc2
3984155
aa1248c
9c797ad
d27490d
33ea8f4
89c8a11
2d9e151
bcb41f8
a5dd089
1ef38f0
fe258f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation doesn't match anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a breaking change, so it should be listed in the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. Maybepick_theme
or something like that, if you have better ideas let me know.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ortry_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 thetry_
function)