-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
No csd linux by default #68640
No csd linux by default #68640
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten LGTM. But please open a test plan item and assign me for it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -265,6 +265,11 @@ export function getTitleBarStyle(configurationService: IConfigurationService, en | |||
return 'native'; // not enabled when developing due to https://github.com/electron/electron/issues/3647 | |||
} | |||
|
|||
const accessibilityMode = configurationService.getValue('editor.accessibilitySupport'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten get this setting only when !isMacintosh
?
@@ -265,6 +265,11 @@ export function getTitleBarStyle(configurationService: IConfigurationService, en | |||
return 'native'; // not enabled when developing due to https://github.com/electron/electron/issues/3647 | |||
} | |||
|
|||
const accessibilityMode = configurationService.getValue('editor.accessibilitySupport'); | |||
if (!isMacintosh && accessibilityMode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten the value of editor.accessibilitySupport
is a string, so how does your check here actually work?
run: () => { | ||
this.storageService.store('menubar/electronFixRecommended', true, StorageScope.GLOBAL); | ||
} | ||
const message = nls.localize('menubar.linuxTitlebarRevertNotification', "We have updated the default title bar on Linux to use the native setting. If you prefer, you can go back to the custom setting "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten why is there a trailing whitespace at the end of the sentence (you can go back to the custom setting
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten looks like you changed to show separate notifications now. LGTM with some polish:
- "Go to Setting" => "Open Settings"
- it looks like you always store the "Do not show again" state after showing the notification, shouldn't that be a decision that is made only by the user when clicking "Don't Show Again"?
@sbatten also, I would inline the "More Info" to be a link within the notification and not have it as extra button to reduce clutter. You can use markdown style styntax for that. |
@bpasero, updated to use inline link as suggested and switched back to Open Settings for consistency. I kept the behavior to not keep showing this message even without the user's input so as to not bug the user. The notifications are not critical unlike the previous notification to address an electron bug. |
@sbatten LGTM |
Revert the default custom title bar behavior on Linux
No description provided.