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

Support detecting inverted color scheme on Windows #7518

Merged
merged 6 commits into from Oct 11, 2016

Conversation

Projects
None yet
4 participants
@kevinsawicki
Contributor

kevinsawicki commented Oct 6, 2016

This API allows app's to determine if a high contrast theme is active on Windows in case they want to adjust their styles at startup or when the value is changed.

Chrome currently uses this API to show a popover about installing a dark theme and high contrast extension.

  • Add isInvertedColorScheme helper
  • Add inverted-color-scheme-changed event for when the theme/colors switched into and out of an inverted color scheme.

Closes #7469

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 6, 2016

Should we also listen for WM_COLORCHANGE messages and fire appropriate events?

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 6, 2016

Should we also listen for WM_COLORCHANGE messages and fire appropriate events?

Yeah, events for this would be great too, I think Chrome already has a custom listener wrapper for these, will explore.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 6, 2016

Here is the Chrome helper for these: https://cs.chromium.org/chromium/src/ui/gfx/sys_color_change_listener.h

Will update this pull request to use it as well and see how it goes.

@kevinsawicki kevinsawicki changed the title from Add systemPreferences.isInvertedColorScheme API to [WIP] Add systemPreferences.isInvertedColorScheme API Oct 6, 2016

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 6, 2016

Or, looking at what you just implemented we could use SysColorChangeObserver

https://chromium.googlesource.com/chromium/src.git/+/lkcr/ui/gfx/sys_color_change_listener.cc

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 6, 2016

Ahhaha, you beat me by like 45 seconds... 😆

@kevinsawicki kevinsawicki changed the title from [WIP] Add systemPreferences.isInvertedColorScheme API to Support detecting inverted color scheme on Windows Oct 7, 2016

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 7, 2016

Should we also listen for WM_COLORCHANGE messages and fire appropriate events?

Updated this pull request to emit an inverted-color-scheme-changed event for when the theme/colors are updated and impact that value.

@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented Oct 7, 2016

I give this PR full marks, if you want extra credit, give a way to get the High Contrast colors by asking for COLOR_WINDOWTEXT and COLOR_WINDOW colors, https://msdn.microsoft.com/en-us/library/dd318443(vs.85).aspx has some more info

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 7, 2016

give a way to get the High Contrast colors by asking for WINDOW_BACKGROUND and WINDOW_TEXT colors

Would these be the COLOR_WINDOWTEXT and COLOR_WINDOW color values?

Looks like that is what Chrome uses to detect high contrast colors: https://cs.chromium.org/chromium/src/ui/gfx/sys_color_change_listener.cc?l=25

@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented Oct 7, 2016

@kevinsawicki Yeah, fixed my typo

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 7, 2016

Yeah, fixed my typo

Awesome, yeah, that sounds good 👍

@@ -19,6 +19,21 @@ const wchar_t kSystemPreferencesWindowClass[] =
namespace api {
class SystemPreferencesColorChangeListener

This comment has been minimized.

@zcbenz

zcbenz Oct 10, 2016

Contributor

SystemPreferences can inherit gfx::SysColorChangeListener directly.

bool invertered_color_scheme_ = false;
std::unique_ptr<gfx::ScopedSysColorChangeListener> color_change_listener_;

This comment has been minimized.

@zcbenz

zcbenz Oct 10, 2016

Contributor

Since color_change_listener_ is initialized in constructor, there is no need to put gfx::ScopedSysColorChangeListener in a unique_ptr.

@@ -93,6 +100,10 @@ class SystemPreferences : public mate::EventEmitter<SystemPreferences> {
HWND window_;
std::string current_color_;
bool invertered_color_scheme_ = false;

This comment has been minimized.

@zcbenz

zcbenz Oct 10, 2016

Contributor

invertered_color_scheme_ should be initialized with IsInvertedColorScheme().

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 10, 2016

give a way to get the High Contrast colors by asking for COLOR_WINDOWTEXT and COLOR_WINDOW colors

After thinking about this a bit more, I'd like to do this in a separate PR that adds something like systemPreferences.getColor(id) and exposes access to all the colors listed on https://msdn.microsoft.com/en-us/library/windows/desktop/ms724371(v=vs.85).aspx

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Oct 11, 2016

👍

@zcbenz zcbenz merged commit 2a3bcdc into master Oct 11, 2016

8 of 9 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #4378899 succeeded in 62s
Details
electron-linux-ia32 Build #4378900 succeeded in 56s
Details
electron-linux-x64 Build #4378901 succeeded in 113s
Details
electron-mas-x64 Build #2581 succeeded in 8 min 56 sec
Details
electron-osx-x64 Build #2586 succeeded in 9 min 15 sec
Details
electron-win-ia32 Build #1675 succeeded in 8 min 0 sec
Details
electron-win-x64 Build #1650 succeeded in 8 min 6 sec
Details

@zcbenz zcbenz deleted the inverted-color-scheme branch Oct 11, 2016

@kevinsawicki kevinsawicki referenced this pull request Oct 11, 2016

Merged

Support retrieving Windows system colors #7561

4 of 4 tasks complete

@bpasero bpasero referenced this pull request Nov 10, 2016

Merged

Update to electron 1.4.x #15298

11 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment