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

The less settings the better #732

Closed
isidorn opened this issue Sep 1, 2020 · 6 comments · Fixed by microsoft/vscode#106521
Closed

The less settings the better #732

isidorn opened this issue Sep 1, 2020 · 6 comments · Fixed by microsoft/vscode#106521
Assignees
Labels

Comments

@isidorn
Copy link

isidorn commented Sep 1, 2020

Refs: microsoft/vscode#105771

I noticed that we introduced quite some settings for debug.javascript while most of this make perfect sense I just wanted to share my thoughts that:

  • The less settings the better
  • No need to intrdouce a setting only if a couple of users ask for it. Nobody will always be happy.
  • Less settings, less complexity
  • We depracte the "use preview" settings once things go out of preview

I did not really review all of them but there are some quite obscure ones, like for example
"debug.javascript.warnOnLongPrediction"

Just my 2 cents.

@isidorn
Copy link
Author

isidorn commented Sep 1, 2020

On top of this there is a lot of settins overlap how autoAttach can be enabled / disabled.
"debug.node.autoAttach": "off",
"debug.javascript.usePreviewAutoAttach": false,
"debug.javascript.autoAttachFilter": "explicit",

For example one of the options for autoAttachFilter could be never

@isidorn
Copy link
Author

isidorn commented Sep 1, 2020

Also "debug.node.autoAttach": "disabled" puts it to off and hides it. This is not proper, since this setting should not control the UI. Users can expiclitly hide any item in the status bar.

@joaomoreno
Copy link
Member

Also "debug.node.autoAttach": "disabled" puts it to off and hides it. This is not proper, since this setting should not control the UI. Users can expiclitly hide any item in the status bar.

Absolutely right! The disabled vs off difference isn't obvious. Once I grokked it, it's just a matter of controlling the visibility of a status bar action, for which we already have core functionality.

@connor4312
Copy link
Member

The preponderance of auto attach settings specifically is something brought up in standup as well. debug.node.autoAttach is the 'old' setting and debug.javascript.usePreviewAutoAttach is toggling with the old behavior. Next month, provided things go well, I want to remove both these settings and add a never setting to debug.javascript.autoAttachFilter which would be the same as 'disabled'.

debug.javascript.warnOnLongPrediction -- good suggestion, I added this way back when I started working on VS Code, this should be an item in the workspace state, not a setting.

Absolutely right! The disabled vs off difference isn't obvious. Once I grokked it, it's just a matter of controlling the visibility of a status bar action

This was existing functionality, cc @weinand. I wonder if we need a button at all once js-debug auto attach is implemented, since at that point auto attach will be fairly passive. Thoughts?

@weinand
Copy link

weinand commented Sep 1, 2020

The only reason why debug.node.autoAttach has a "disabled" state is that it was introduced before we were able to control the visibility of status bar actions via core functionality. Since not every user is a node.js user, and we are shipping with a builtin node debugger, we needed a way to hide this node.js specific status bar item for Python, Java, etc. users.

The "on" and "off" states exist because the old "auto attach" feature is based on polling which has a cost.
Since the new "auto attach" no longer uses polling, we can remove the "on" and "off" settings since we have the "autoAttachFilter" for controlling how "aggressive" ;-) auto attach is.

@isidorn
Copy link
Author

isidorn commented Sep 1, 2020

Great, so it seems like we are all aligned.
Apart from the above I suggest we review the other debug settings and figure out if they are really needed / or if their description could be improved.

connor4312 added a commit that referenced this issue Sep 8, 2020
connor4312 added a commit to microsoft/vscode that referenced this issue Sep 11, 2020
This PR removes the hook in node-debug's auto attach, and uses only
js-debug auto attach. As referenced in the linked issues, this involves
removing `debug.javascript.usePreviewAutoAttach` and collapsing
`debug.node.autoAttach` into `debug.javascript.autoAttachFilter`. The
latter option gains a new state: `disabled`. Since there's no runtime
cost to having auto attach around, there is now no distinct off versus
disabled state.

The status bar item and the `Debug: Toggle Auto Attach` command now
open a quickpick, which looks like this:

![](https://memes.peet.io/img/20-09-9d2b6c0a-8b3f-4481-b2df-0753c54ee02b.png)

The current setting value is selected in the quickpick. If there is a
workspace setting for auto attach, the quickpick toggle the setting
there by default. Otherwise (as in the image) it will target the user
settings. The targeting is more explicit and defaults to the user
instead of the workspace, which should help reduce confusion (#97087).
Selecting the "scope change" item will reopen the quickpick in that
location.

Aside from the extra options for the `disabled` state in js-debug's
contributions, there's no changes required to it or its interaction
with debug-auto-launch.

Side note: I really wanted a separator between the states and the
scope change item, but this is not possible from an extension #74967.

Fixes #105883
Fixes microsoft/vscode-js-debug#732 (the rest of it)
Fixes #105963
Fixes #97087
sandy081 pushed a commit to microsoft/vscode that referenced this issue Sep 16, 2020
This PR removes the hook in node-debug's auto attach, and uses only
js-debug auto attach. As referenced in the linked issues, this involves
removing `debug.javascript.usePreviewAutoAttach` and collapsing
`debug.node.autoAttach` into `debug.javascript.autoAttachFilter`. The
latter option gains a new state: `disabled`. Since there's no runtime
cost to having auto attach around, there is now no distinct off versus
disabled state.

The status bar item and the `Debug: Toggle Auto Attach` command now
open a quickpick, which looks like this:

![](https://memes.peet.io/img/20-09-9d2b6c0a-8b3f-4481-b2df-0753c54ee02b.png)

The current setting value is selected in the quickpick. If there is a
workspace setting for auto attach, the quickpick toggle the setting
there by default. Otherwise (as in the image) it will target the user
settings. The targeting is more explicit and defaults to the user
instead of the workspace, which should help reduce confusion (#97087).
Selecting the "scope change" item will reopen the quickpick in that
location.

Aside from the extra options for the `disabled` state in js-debug's
contributions, there's no changes required to it or its interaction
with debug-auto-launch.

Side note: I really wanted a separator between the states and the
scope change item, but this is not possible from an extension #74967.

Fixes #105883
Fixes microsoft/vscode-js-debug#732 (the rest of it)
Fixes #105963
Fixes #97087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants