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

Add an app API method to fetch the system ColorizationColor #7173

Merged
merged 6 commits into from Sep 20, 2016

Conversation

Projects
None yet
7 participants
@MarshallOfSound
Member

MarshallOfSound commented Sep 11, 2016

This is useful for automatic app theming

  • Fetches the color from the registry
  • Returns an empty string if the fetch fails (a falsey value)
@@ -852,6 +852,19 @@ correctly.
**Note:** This will not affect `process.argv`.
### `app.getColorizationColor()` _Windows_
Returns the users current system wide color preference in the form of an ARGB

This comment has been minimized.

@paulcbetts

paulcbetts Sep 11, 2016

Contributor

Maybe include a screenshot of where this color is used?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 11, 2016

Member

Inside the docs?

Perhaps linking to something like this http://www.windowscentral.com/how-custom-accent-color-windows-10 explains what the color is.

On Windows 7 and 8 it is your Aero theme color. On Windows 10 it's your "theme color".

This comment has been minimized.

@paulcbetts

paulcbetts Sep 11, 2016

Contributor

Ya, that works too - I'm just not sure that people will know what this is. I'd even go so far as to rename this getAccentColor

@ficristo

This comment has been minimized.

ficristo commented Sep 12, 2016

@MarshallOfSound could you explain more "This is useful for automatic app theming" ?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 12, 2016

@ficristo On Windows 7+ users can specify a theme color. Some apps offer the ability to either use a custom window frame (using the frameless window) or customize the theme of their app.

It would be nice to utilize the users system color preference as the custom frame or theme hence this method

### `app.getSystemAccentColor()` _Windows_
Returns the users current system wide color preference in the form of an ARGB
hexadecimal string.

This comment has been minimized.

@sindresorhus

sindresorhus Sep 12, 2016

Contributor

I would return it as RGBA hex to match browsers: http://stackoverflow.com/a/27802062/64949

@miniak

This comment has been minimized.

Contributor

miniak commented Sep 13, 2016

@MarshallOfSound Why don't you use DwmGetColorizationColor?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 13, 2016

@miniak Reading from the registry seemed like a better plan than using an undocumented API 👍

@miniak

This comment has been minimized.

Contributor

miniak commented Sep 13, 2016

@miniak

This comment has been minimized.

Contributor

miniak commented Sep 13, 2016

Btw there should also be a notification triggered by WM_DWMCOLORIZATIONCOLORCHANGED

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 13, 2016

@miniak Wut, I could have sworn those methods were undocumented 😢 Maybe I'm finally going mad 😆

@@ -167,6 +167,13 @@ class Browser : public WindowListObserver {
// one from app's name.
// The returned string managed by Browser, and should not be modified.
PCWSTR GetAppUserModelID();
typedef HRESULT (STDAPICALLTYPE *DwmGetColorizationColor)(DWORD *, BOOL *);

This comment has been minimized.

@miniak

miniak Sep 13, 2016

Contributor

@MarshallOfSound this is unnecessary, Electron only supports Windows 7+

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 13, 2016

Member

Compile failed without it so I had to add it in. I didn't think it was necessary but ninja disagreed 😆

This comment has been minimized.

@miniak

miniak Sep 13, 2016

Contributor

What about fixing the build instead of adding complexity to the code? :) what was it complaining about?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 13, 2016

Member

Basically a whole bunch of unresolved identifiers, it didn't know what DwmGetColorizationColor was. I'll try harder to work around it but adding one typedef didn't seem to bad at the time 👍

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 13, 2016

@miniak Switched to use the DWMAPI dll. Also added the system-accent-color-changed event 👍

@@ -844,6 +848,8 @@ void App::BuildPrototype(
.SetMethod("setUserTasks", base::Bind(&Browser::SetUserTasks, browser))
.SetMethod("getJumpListSettings", &App::GetJumpListSettings)
.SetMethod("setJumpList", &App::SetJumpList)
.SetMethod("getSystemAccentColor",

This comment has been minimized.

@miniak

miniak Sep 13, 2016

Contributor

@MarshallOfSound I think this belongs to the new systemPreferences module

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 13, 2016

Member

Moving the method to systemPreferences is relatively easy. The issue is that the event WM_DWMCOLORIZATIONCOLORCHANGED is emitted at the window level, and I don't think we should be coupling the NativeWindow class with systemPreferences.

@@ -90,6 +90,15 @@ bool NativeWindowViews::PreHandleMSG(
// mode if it isn't already, always say we didn't handle the message
// because we still want Chromium to handle returning the actual
// accessibility object.
case WM_DWMCOLORIZATIONCOLORCHANGED: {

This comment has been minimized.

@zcbenz

zcbenz Sep 16, 2016

Contributor

Putting this in NativeWindowViews makes the event get emitted for multiple times when there are multiple windows. The common way is to create message window to receive the messages, and there is a helper class in Chromium to do it: https://cs.chromium.org/chromium/src/base/win/message_window.h?sq=package:chromium.

By using it we can also move the API to systemPreferences.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 16, 2016

Member

@zcbenz I'm unfamiliar with that helper class so I'm probable doing something silly. But while trying to do this

CreateNamed(base::Bind(&SystemPreferences::HandleMessage, this), L"test");

I get this glorious error

vendor\brightray\vendor\download\libchromiumcontent\src\base\memory\ref_counted.h(407): error C2039: 'Release': is not a member of 'atom::api::SystemPreferences'

Is there another helper that provides the required Release and AddRef methods that making a pointer to a non static member function requires?

This comment has been minimized.

@zcbenz

zcbenz Sep 16, 2016

Contributor

Usually you can do this to pass raw pointers:

base::Bind(&SystemPreferences::HandleMessage, base::Unretained(this))

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 16, 2016

Member

@zcbenz That works, thanks.

However, after some more exploration it looks like a message only window won't do the job as

does not receive broadcast messages

https://msdn.microsoft.com/en-us/library/windows/desktop/ms632599.aspx#message_only

This comment has been minimized.

@zcbenz

zcbenz Sep 16, 2016

Contributor

I'm not sure whether WM_DWMCOLORIZATIONCOLORCHANGED belongs to broadcast message, most system notifications do not.

If it is we have to use a hidden window to deal with it.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 16, 2016

Member

@zcbenz Updated to create a hidden window inside the systemPreferences module

namespace {
const wchar_t kSystemPreferencestWindowClass[] =

This comment has been minimized.

@levrik

levrik Sep 17, 2016

Contributor

Is Preferencest a typo?

@zcbenz

zcbenz approved these changes Sep 19, 2016

MarshallOfSound added some commits Sep 16, 2016

Add a systemPreferences API method to fetch the system ColorizationColor
This is useful for automatical app theming

* Fetches the color from the registry
* Returns an empty string if the fetch fails (a falsey value)
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Sep 20, 2016

👍

@zcbenz zcbenz merged commit 77fdc67 into master Sep 20, 2016

5 of 6 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #2412 succeeded in 8 min 14 sec
Details
electron-osx-x64 Build #2418 succeeded in 8 min 47 sec
Details
electron-win-ia32 Build #1484 succeeded in 7 min 57 sec
Details
electron-win-x64 Build #1462 succeeded in 8 min 2 sec
Details
@paulcbetts

This comment has been minimized.

Contributor

paulcbetts commented Sep 22, 2016

Thanks @MarshallOfSound! This is great

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