Skip to content
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

Extend the color profile PR (#17380) #17464

Merged
merged 2 commits into from Jun 4, 2018

Conversation

Projects
None yet
3 participants
@thomasjo
Copy link
Member

thomasjo commented Jun 4, 2018

Description of the Change

Extends #17380 by wiring up the dialog prompt for restarting Atom whenever the core.colorProfile setting is changed. Since this removes the need for the explicit description remark about restarting Atom, that sentence is also removed.

The third (and optional) change alters the bound functions used when wiring up the change event listeners to arrow functions. We can drop that commit if it's deemed risky, not necessary, or some such.

Alternate Designs

A more elaborate opt-in mechanism for "prompt for relaunch" was considered, but rejected after taking into consideration the concept of YAGNI™.

Why Should This Be In Core?

For obvious reasons.

Benefits

Users do not have to manually restart Atom. Some users will likely not read the description and realize they need to manually restart Atom, which would most likely lead to issues being submitted, etc.

Possible Drawbacks

More code, and complexity?

Verification Process

Manually tested on Ubuntu 16.04. Works as expected.

@@ -357,7 +357,7 @@ const configSchema = {
description: 'Experimental: Use the new Tree-sitter parsing system for supported languages.'
},
colorProfile: {
description: "Specify whether Atom should use the operating system's color profile (recommended) or an alternative color profile.<br>Changing this setting will require a relaunch of Atom to take effect.",
description: "Specify whether Atom should use the operating system's color profile (recommended) or an alternative color profile.",

This comment has been minimized.

@50Wliu

50Wliu Jun 4, 2018

Member

The relaunch line is included in https://github.com/atom/atom/pull/17464/files#diff-8c99efd21425e5773e52ba4fbb820bf0L574, the other setting that requires a restart. If this one is removed, then the other should be as well.

This comment has been minimized.

@thomasjo

thomasjo Jun 4, 2018

Author Member

Thought I checked that one, but guess I didn't scroll far enough. Good catch, @50Wliu 🥇

Personally I vote for removing the sentence in both places since it feels a bit superfluous since we have the prompt. What do other folks think? /cc @atom/feedback

This comment has been minimized.

@daviwil

daviwil Jun 4, 2018

Member

I think it's good to inform the user that a restart will be required even if we're going to prompt them. This will make sure they understand why they're being prompted for a restart. I'd say leave the message in for the sake of clarity

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jun 4, 2018

This change is simple enough that I don't see any problem merging this PR right after I merge the color profile PR. I think I'd prefer leaving the "restart" language in the setting description though, just for the sake of clarity.

Thanks again for putting this together!

@thomasjo thomasjo force-pushed the extended-color-profile-bits branch from c7935c5 to 4a2b585 Jun 4, 2018

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jun 4, 2018

That was ninja-fast ;)

@daviwil

daviwil approved these changes Jun 4, 2018

Copy link
Member

daviwil left a comment

🙇

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jun 4, 2018

Just waiting on CI to finish before merging this. Super slow today!

@daviwil daviwil merged commit a2fe4c3 into fix-color-problems-using-electron-2.0 Jun 4, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@daviwil daviwil deleted the extended-color-profile-bits branch Jun 4, 2018

daviwil added a commit that referenced this pull request Jun 4, 2018

@daviwil daviwil restored the extended-color-profile-bits branch Jun 4, 2018

@daviwil daviwil deleted the extended-color-profile-bits branch Jun 4, 2018

daviwil added a commit that referenced this pull request Jun 4, 2018

@bittin bittin referenced this pull request Jul 25, 2018

Closed

1.28 releases #17733

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.