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

debug: use only js-debug auto attach, collapse settings #106521

Merged
merged 2 commits into from
Sep 16, 2020

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented 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:

The current setting value is selected in the quickpick. If there is a
workspace setting for auto attach, the quickpick toggles 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

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
@connor4312 connor4312 added the debug Debug viewlet, configurations, breakpoints, adapter issues label Sep 11, 2020
@connor4312 connor4312 added this to the September 2020 milestone Sep 11, 2020
@connor4312 connor4312 self-assigned this Sep 11, 2020
connor4312 added a commit to microsoft/vscode-js-debug that referenced this pull request Sep 11, 2020
@isidorn
Copy link
Contributor

isidorn commented Sep 16, 2020

@connor4312 thanks for tackling this. It makes good sense to me, I like that we are getting rid of some settings, old code and simplifying the behavior a bit.

Minor feedback: should disable really hide it from the status bar? Since then it is a bit hard to disable and enable again? Hidding things from the status bar is an independent UX action which the user can do on her own -> context menu and hide

I am adding approved here, however for a code review I leave this up to @weinand since he did the original auto attach extension I believe.

Sorry for the slow review, I was on vacation for 2 days.

Copy link
Contributor

@weinand weinand left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

@weinand
Copy link
Contributor

weinand commented Sep 16, 2020

@connor4312 I like the quickpick but this raises the question whether the "JavaScript Debug Terminal" becomes obsolete (I would hope so).

If yes, then we should remove the corresponding item from the "Welcome view" (in red) and add something along the lines of "or automatically debug Node.js programs run from the command line (aka 'auto attach')" (in the green box).

2020-09-16_10-53-23

"Creating a launch.json" should move down because it becomes more like a "last resort"

@isidorn
Copy link
Contributor

isidorn commented Sep 16, 2020

Agree with @weinand comment, the dream scenario would be to merge this and the JavaScript debug terminal into one experince (I believe we have an issue for this)

@connor4312
Copy link
Member Author

connor4312 commented Sep 16, 2020

Yep, let's continue to discuss that in #105852 🙂

Minor feedback: should disable really hide it from the status bar? Since then it is a bit hard to disable and enable again? Hidding things from the status bar is an independent UX action which the user can do on her own -> context menu and hide

If we show it when disabled, that means that the item is visible in the status bar for all users by default (though we could potentially have something in the global state where we only add that after the first auto attach toggle...) My concern is that the ability to hide/show items in the status bar is not obvious or perhaps even known by most people, and it's even then it's an extra knob people need to turn off. Other extensions already tend to throw a bunch of annoying info I don't care about in the status bar, I would want to be careful that we don't join them there.

(This PR doesn't regress that behavior, so I'll proceed in merging it, but we can continue discussion)

@connor4312
Copy link
Member Author

Following some feedback in standup, polish:

  • made the state labels nicer and localized
  • title the quickpick
  • made the scope toggle a button (a folder or a globe to toggle to workspace or machine configuration)

@connor4312 connor4312 merged commit 923189c into master Sep 16, 2020
@connor4312 connor4312 deleted the connor4312/auto-attach-js-debug branch September 16, 2020 22:41
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues
Projects
None yet
3 participants