Restore color rendering following Electron 2.0 update #17380
Conversation
The problem isn't with the colors in the CSS, but that the colors are now accurate, where before they were oversaturated due to the incorrect mapping being done before. When you are used to the incorrect, oversaturated, colors when they are suddenly now rendered correctly it usually looks "washed out".
I definitely fall into that category, you should always be rendering the correct colors 😉. |
With custom display profile configuredAtom 1.27.1Atom 1.28.0-beta1Atom built from this branchWithout custom display profile configuredAtom 1.27.1Atom 1.28.0-beta1Atom built from this branch |
Without custom display profile configured ✅Using the display's default profile, I see the following results: With custom display profile configured ❌Using a custom display profile (ACES CG Linear), I see the following results: |
Without custom display profile configured ✅Using the display's default profile, I see the following results: With custom display profile configured ✅Using a custom display profile (Adobe RGB 1998), I see the following results: |
|
@jasonrudolph Thanks for taking all the screenshots. 👌 I guess the "safe thing to do ™️" would be to merge this PR to avoid change. But the "right thing to do ™️" would probably be to use the system's color profile. On macOS and Windows the differences seem to be noticeable, but it doesn't make Atom "unusable". Just on Linux, that example with the custom profile: That's quite the drastic change and makes it hard to use. But then I wonder, people that actually use that color profile, they should be used to not a lot of contrast with dark backgrounds, so they are probably using a light theme instead? So with all that in mind, I lean slightly towards not merging this PR, even tough some people might get upset. Because you can also look at it from this angle: Atom finally respects the system's color profile that the user can tweak and doesn't force you into using |
|
Made a test similar to the one in discuss, but on macOS. The CSS value of the background color is:
So by using the Personally, I'll keep the monitor's default with the slightly corrected colors. |
The more I research this topic, the more I'm in favor of closing this pull request. Given the context in the Google doc from the Chrome dev team and the resources that the doc links to, Atom 1.28's color rendering seems to be a move in the right direction (as opposed to being a regression). #17356 (comment) describes my current understanding of the situation, and I welcome feedback. 🙇 |
|
I'm also in favor of closing this PR and keeping things as they are. Thanks for the extremely thorough investigation @jasonrudolph! 🙇♂️ |
|
Having thought about it some more.. I'm in favor of this alternative:
Add a config option: It is yet another checkbox to maintain. 🙈 But when Atom 1.28 goes to stable, we would have to tell people that experience this issue the following:
I just feel that "forcing" people to change their custom color profile is a bit too much to ask. |
|
I've updated the implementation to allow users to opt in to using the force-color-profile chromium flag. I've updated the pull request body to reflect this approach. @daviwil @maxbrunsfeld: Would you mind reviewing this pull request? I'd love to get your feedback on the design (as described in the pull request body) and on the implementation. |
This change demonstrates the configuration option implemented in atom/atom#17380.
|
I've updated the implementation to use Atom's standard config file (i.e., |
Oddly, I expect this to resolve the CI failure seen in https://travis-ci.org/atom/atom/builds/386819124#L1200.
|
Let's look at the behavior before and after these changes. Before#17356 (comment) demonstrated the behavior in Atom 1.28.0-beta1. For this test, we observed how the background color of the text editor is rendered when using Atom's One Dark syntax theme. AfterWith the changes in this pull request, Atom still respects the operating system's color profile by default, but the user can optionally force Atom to use an sRGB color profile. We see this in the demo below. When applying the Adobe RGB 1998 color profile to the operating system, by default Atom respects the color profile and renders |
| @@ -39,6 +42,12 @@ module.exports = function start (resourcePath, startTime) { | |||
| atomPaths.setUserData(app) | |||
| setupCompileCache() | |||
|
|
|||
| const config = getConfig() | |||
There was a problem hiding this comment.
It would be nice if we could reuse this Config and/or ConfigFile instance in AtomApplication, in order to avoid reading the config file twice on startup (and also to avoid duplicating the code for locating the config file). That said, I don't think it is necessarily important enough to block merging this PR.
|
Excellent work @jasonrudolph! I'm going to quickly test this out on Windows and see if it has the desired effect. |
|
I chatted with @daviwil regarding the next steps for this pull request:
Note: We acknowledge that Atom doesn't automatically relaunch when you change this new config setting, but that's something that could be addressed in a follow-up pull request. 😅 |
|
Tested this out on Windows, works great! With Color Profile set to "Use color profile configured in the operating system" the colors are washed out: Once I change Color Profile to "Use sRGB color profile" and restart Atom, I see this: Thumbs up from me! I'll get this cherry-picked over to |
There was a problem hiding this comment.
Note: We acknowledge that Atom doesn't automatically relaunch when you change this new config setting, but that's something that could be addressed in a follow-up pull request.
Yeah, would be nice to be consistent with the custom title-bar setting, but not that high priority.
|
I implemented the restart prompt in another branch. Let me know if you want me to cherry-pick the commit(s) into this branch. |
|
Thanks @thomasjo! I'll go ahead and merge this PR now and then we can merge your improvement afterward. |
Extend the color profile PR (#17380)
…-2.0 Restore color rendering following Electron 2.0 update
Extend the color profile PR (#17380)
Extend the color profile PR (#17380)
|
Thanks for this setting! I actually love the oversaturated/vibrant syntax colors of the |
|
For some reason this isn't working for me under 1.28.0 on macOS 10.13.4. The OS color profile is "Color LCD" (default). The |
Fixes #17356
Description of the Change
Background
In #17356 and electron/electron#10732, users reported an unwelcome change to Atom's color rendering following the recent update to Electron 2.0.
This doc from the Chrome team (shared in electron/electron#10732 (comment)) explains that these color differences are the result of a change in Chromium, and that the change is intentional:
However, the document goes on to acknowledge that this change has received a fair bit of pushback:
Chromium provides a flag that allows you to force a particular color profile:
Approach
This pull request allows Atom users to opt in to using the
force-color-profilechromium flag.Usage
To restore Atom's previous color behavior:
Settings View: Openin the command palette)Alternate Designs
We could always force the color profile to
srgb.However, as described in the doc from the Chrome team, this would essentially "lock all users forever into 1996-era technology," and we'd prefer to avoid that.
We could do nothing, but some users want Atom to behave like most other apps by ignoring the user's custom color profile. Pale Colors #17356 (comment), Restore color rendering following Electron 2.0 update #17380 (comment)
Benefits
Provides an option to instruct Atom to ignore the user's custom color profile, thus causing Atom 1.28's color rendering to match the rendering used in Atom 1.27 and earlier (i.e., the color rendering as it existed prior to the Electron 2.0 upgrade in #17273).
Possible Drawbacks
Adding more code to the startup process means there are more things that can go wrong in the startup process. If we encounter problems reading and/or parsing the config file, it may cause Atom to crash at startup in ways that are difficult to debug.
Verification Process
Using Ubuntu 18.04, build Atom from this branch (which uses Electron 2.0.1) and verify that:
colorProfileoption is not present in the config file, Atom launches successfully and respects the user's custom color profilecolorProfileoption is present in the config file and has a value of"srgb", Atom ignores the user's custom color profile, and Atom's color rendering matches the color rendering seen in Atom 1.27.1colorProfileoption is present in the config file and has a value of"srgb", Atom launches successfully and Atom's color rendering matches the color rendering seen in Atom 1.27.1/fyi @codebytere