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

Respect included property on preferences #11588

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

msujew
Copy link
Member

@msujew msujew commented Aug 19, 2022

What it does

Closes #11587. Preferences that should not be included (i.e. included: false) are ignored when building the preference schema.

How to test

  1. Start the application on Mac (or set one of the preferences included properties to false)
  2. Assert that the preference is not rendered/does not appear in the search of the preference widget
  3. Assert that the preference cannot be accessed using the settings text editor

Review checklist

Reminder for reviewers

@msujew msujew added the preferences issues related to preferences label Aug 19, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@msujew I confirmed that the preference is hidden from the UI but I believe we need more.
The preference still exists as a suggestion in the settings.json and setting the preference still takes effect which can cause unwanted side effects.

In my example I also set workbench.statusBar.visible to include: false and it still shows up, and takes effect:

Screen Shot 2022-08-19 at 9 42 44 AM

@msujew
Copy link
Member Author

msujew commented Aug 19, 2022

@vince-fugnitto Right, I'll look into that 👍

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This is the correct approach - preventing the preference from ever being registered with the schema at all. In testing, this prevents undesired / inapplicable preferences from appearing in any context. 👍

@msujew msujew merged commit 17c525e into master Aug 23, 2022
@msujew msujew deleted the msujew/respect-included-preference branch August 23, 2022 13:46
@github-actions github-actions bot added this to the 1.29.0 milestone Aug 23, 2022
@vince-fugnitto
Copy link
Member

@msujew should we still go ahead with #11584 after your changes?

@vince-fugnitto
Copy link
Member

The changes unfortunately include a regression in the browser application on macOS where the top-level disappears after the application is loaded. I'll need to see why that happens:

Screen Shot 2022-08-24 at 11 09 51 AM

@colin-grant-work
Copy link
Contributor

        'window.menuBarVisibility': {
            type: 'string',
            enum: ['classic', 'visible', 'hidden', 'compact'],
            markdownEnumDescriptions: [
                nls.localizeByDefault('Menu is only hidden in full screen mode.'),
                nls.localizeByDefault('Menu is always visible even in full screen mode.'),
                nls.localizeByDefault('Menu is always hidden.'),
                nls.localizeByDefault('Menu is displayed as a compact button in the sidebar. This value is ignored when `#window.titleBarStyle#` is `native`.')
            ],
            default: 'classic',
            scope: 'application',
            // eslint-disable-next-line max-len
            markdownDescription: nls.localizeByDefault("Control the visibility of the menu bar. A setting of 'toggle' means that the menu bar is hidden and a single press of the Alt key will show it. By default, the menu bar will be visible, unless the window is full screen."),
            included: !isOSX
        },

The included should check whether we're in Electron or not. If we're in the browser, the preference should be present, even on MacOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preference widget does not respect included
3 participants