Change Window Theme with Pulsar Theme #545
Conversation
|
Made the requested changes on this PR. While it doesn't resolve all the issues related, do we think we should go ahead and close this one? Or still worth it to implement? |
|
Provided this PR still works in newer versions, and doesn't cause problems - we should probably implement this, no? It doesn't fix everything with the weird theming, but it does fix some, right? |
|
It might be worth putting behind a config setting, I suppose? I can see how it makes sense to infer a light-mode or dark-mode decision based on the UI theme the user has chosen… but I can also see how some people might just want all their window title bars to match each other. |
|
That's an easy agreement from me - especially since it would technically be a user facing change, and something that would be a divergence from their expected norm |
|
Thanks for taking another look at this @Spiker985 & @savetheclocktower I'll go ahead and put the behavior behind a config file and get this updated as soon as I can |
|
Alright, @Spiker985 & @savetheclocktower I've gone ahead and made sure this one is configurable. If anyone would like to review it, it'd be awesome to get this one merged |
There was a problem hiding this comment.
Seems legit. 👍
Maybe someday we can add something to auto-select a light or dark theme based on the OS theme. (Perhaps with a drop-down for system dark and another dropdown for system light, to designate the specific theme when OS theme is light vs when OS theme is dark.)
| this.disposable.add( | ||
| ipcHelpers.on(ipcMain, 'setWindowTheme', (event, options) => { | ||
| if (options && typeof options === 'string') { | ||
| nativeTheme.themeSource = options; |
There was a problem hiding this comment.
I had to look this up, but seems legit!
https://www.electronjs.org/docs/latest/api/native-theme#nativethemethemesource
| let luminosity = 0.2126 * bgColor.red + 0.7152 * bgColor.green + 0.0722 * bgColor.blue; | ||
| // ^^ Luminosity per ITU-R BT.709 |
There was a problem hiding this comment.
Source: Maybe from here?
https://stackoverflow.com/questions/596216/formula-to-determine-perceived-brightness-of-rgb-color --> https://en.wikipedia.org/wiki/Relative_luminance#Relative_luminance_and_%22gamma_encoded%22_colorspaces --> https://www.itu.int/dms_pubrec/itu-r/rec/bt/R-REC-BT.709-6-201506-I!!PDF-E.pdf (??)
Just for reference sake.
|
Thanks for the review @DeeDeeG! As for syncing this setting with the OS theme, that should be the default behavior once we bump Electron versions (iirc) so hopefully that's a non-issue in the future. |
This PR adds a feature that's likely long overdue.
When a user changes the theme within Pulsar, the brightness of the background color will be computed, and if light or dark, will change the Pulsar Window theme to respect that. During initial boot the theme will still follow the system theme as it does, but otherwise once themes are loaded it will then be changed if needed.
How is this done
When the
theme-managerenables a new theme, it will use thecolor.jsclass to get the RGB values of theHTMLelement'sbackgroundColor, with that it then uses the formula defined byITU-R BT.709to determine if that colour is brighter or darker.Depending on that answer it will then call
application-delegate.js(which is now provided to the Theme Manager).setWindowTheme()which uses the IPC Bus to trigger thenativeThememodule from Electron to change the theme.Issues
According to this as far as I can tell this module does not respect the theme for Windows on the title bar, but does so for the menu bar. So this may only half way fix the issue in Windows until we could either bump Electron versions, or someone that understands that discussion better can implement it. But I'm hoping this does work properly for users on other platforms.
Examples
Again remember, that an Electron issue prevents this working on the Title bar in Windows, which is where I took these pictures. So remembering that here's the results I was able to acheive.
When set to a light theme:
When set to a dark theme:
Resolves #422