Skip to content

No csd linux by default#68640

Merged
sbatten merged 6 commits intomicrosoft:masterfrom
sbatten:noCSDLinux
Feb 18, 2019
Merged

No csd linux by default#68640
sbatten merged 6 commits intomicrosoft:masterfrom
sbatten:noCSDLinux

Conversation

@sbatten
Copy link
Member

@sbatten sbatten commented Feb 13, 2019

No description provided.

@sbatten sbatten added this to the February 2019 milestone Feb 13, 2019
@sbatten sbatten self-assigned this Feb 13, 2019
@sbatten sbatten requested review from bpasero and gregvanl February 13, 2019 16:12
Copy link
Member

@bpasero bpasero left a 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 👍

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbatten feedback provided

@kieferrm maybe on the wording of the notification, which currently is: "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".

return 'native'; // not enabled when developing due to https://github.com/electron/electron/issues/3647
}

const accessibilityMode = configurationService.getValue('editor.accessibilitySupport');
Copy link
Member

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 ?

}

const accessibilityMode = configurationService.getValue('editor.accessibilitySupport');
if (!isMacintosh && accessibilityMode) {
Copy link
Member

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 ");
Copy link
Member

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 )?

Copy link
Member

@bpasero bpasero left a 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"?

@bpasero
Copy link
Member

bpasero commented Feb 18, 2019

@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.

@sbatten
Copy link
Member Author

sbatten commented Feb 18, 2019

@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.

@bpasero
Copy link
Member

bpasero commented Feb 18, 2019

@sbatten LGTM

@sbatten sbatten merged commit d415060 into microsoft:master Feb 18, 2019
@sbatten sbatten deleted the noCSDLinux branch February 18, 2019 17:20
sandy081 pushed a commit to vldmrkl/vscode that referenced this pull request Feb 22, 2019
Revert the default custom title bar behavior on Linux
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants