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

When debug session has just one thread, render this thread inline with the session itself #79121

Closed
isidorn opened this issue Aug 14, 2019 · 9 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Aug 14, 2019

No description provided.

@isidorn isidorn added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Aug 14, 2019
@isidorn isidorn added this to the August 2019 milestone Aug 14, 2019
@isidorn isidorn self-assigned this Aug 14, 2019
@isidorn isidorn added the verification-needed Verification of issue is requested label Aug 15, 2019
@isidorn
Copy link
Contributor Author

isidorn commented Aug 15, 2019

To verify have a simple node program to debug.
Start debugging it and hit a breakpoint.
Create a new launch config which also debugs another simple node program.
Start debugging it and hit a breakpoint.

Verify that in the call stack view you can see both debug sessions and their call stack. However the threads are not shown.

renesansz added a commit to renesansz/vscode that referenced this issue Aug 19, 2019
* Use bat on Windows

* debt: use action bar from the panel view

* Fix microsoft/vscode-remote-release/issues/1066

* fix label

* fix: event's jsdoc typo

* Move cleanRemoteAuthority function to common location

* microsoft#78168 strict init

* microsoft#78168 strict init

* microsoft#78168 strict init

* fixes microsoft#77797

* Split formatted text renderer into own file

* Split MarkdownRenderOptions from FormattedTextRenderOptions

* Rename htmlContentRenderer -> markdownRenderer

* web - do not loose state on unload

* open exter - move into opener service

* add extension kind web

* add action only in remote

* Improve microsoft#79047 fix after feedback

* use `vscode-remote`-endpoint when importing scripts

* Added preserveCase in search side bar

* test build schedule

* Revert "test build schedule"

This reverts commit 9d4ba37.

* better check for worker/web extension

* debt - IExtensionDescription#main should be relative like all other file references

* move windowsIPC => common

* very basic support to load multiple files

* microsoft#69108
Move IWorkspaceStatsService to common
Introduce a simple service for web

* Fix microsoft#69108

* tests -remove unused services

* Move getHashedRemotesFromUri to IWorkspaceStatsService

* add and use `getWorkerBootstrapUrl`, don't use default worker factory anymore

* - use strings for view zone ids
- make it unlikely that a new view instance would accept a view zone id from a previous view instance
- remove that the find widget removes a view zone id from another view
(fixes microsoft#71745)

* add `extensionHostWorker` entry point, fixes https://github.com/microsoft/vscode-internalbacklog/issues/738

* Update uglify-es (microsoft#79044)

* Revert "Work around minifier bug (microsoft#79044)"

This reverts commit 6371cad.

* expose product configuration in product service

* use product service

* Simplify terminal commands

* Improve typings, use async

* uses getSelections and simplifed the return

* Update distro

* migrate keys from legacy layout microsoft#79020

* fix tests

* Register driver

* Teardown on sigint

* Update markdown grammar

* Still show fix all actions for fix-all actions that can fix multiple errors with multple different diagnostics

* Base web user data dir off normal one

* Update distro

* Use new browser none arg

* Make sure we compare fully normalized error codes when checking for fix all actions

* Don't include closing ] in folded range

Fixes microsoft#79142

* Register Remote Explorer when there are contributions.

* Use undefined instead of null in IEditorOpeningEvent and IOpenEditorOverride

* Change openEditor to return undefined instead of null

For microsoft#70020

* Improve jsdoc section of walkthrough

Fixes microsoft#71023

As discussed in microsoft#75033

* Add installer assets for OSS (microsoft#79045)

* Revert "Update uglify-es (microsoft#79044)"

This reverts commit e677c03.

* Work around minifier bug (microsoft#79044)

* debt - avoid some StrictNullOverride

* debt - move issue service out of common since it will not be supported in the web for now

* debt - make diagnostics service only accessible from node

* update distro

* tests - add more debug for randomly failing getUntitledWorkspacesSync

* fix mispell

* 💄

* update distro

* Simplify tasks command

* docs: fix type (microsoft#79129)

* web - add todo for potentially cyclic dependency

* Multi-select in custom tree view (microsoft#78625)

Part of microsoft#76941

The first argument is now the element that the command is executed on. The second argurment is an array of the other selected items

* update distro

* web - enable feedback contribution

* debt - more tests diag

* do not look for executables in web

* Move extension tips service to web and enable extension recommendations

* use process.setImmediate

* minor polish

* fixes microsoft#79168

* fix exports trap

* debug: prevent expression.value being undefined

fixes microsoft#79169

* update distro

* web - implement credentials provider and add API

* web - workaround clipboard issue with selection type

* better exports trapping

* fix process layer-breaker

* debt - avoid process dependency in common

* rawDebugSession: do not use process

microsoft#79210

* remove proposed API `vscode.commands.onDidExecuteCommand`

* Fix microsoft#79206

* callStack view:  do not show thread when there is only one to be compact

fixes microsoft#79121

* fix compile error

* inline product configuration in produt service

* update distro

* Fix strict error

* fix tests

* Do not expand session tree node when selecting it

fixes microsoft#79184

* Fix issue with CustomExecutions not working through tasks.executeTask (microsoft#79132)

* Do not clear out the map, and track the provided execution during execute task

* Clear provided custom executions map on execution complete

* Save last custom execution

* use safeprocess env

* Remove click code from puppeteer driver, move interfaces to common

* Reduce diff

* fix typos

* web - reuse require interceptor logic

* remove FakeCommonJSSelf

* Fix dispatch keybinding when called multiple times

* Fix layer breakage

Part of microsoft#79210

* Fixes microsoft#78975: Look for autoclosing pairs in edits coming in from suggestions

* Remove smoke tests from CI for now

* Reduce diff

* Add setting to toggle new octicon style

* Update distro

* Move puppeteer to dev deps

* Run smoke tests for darwin/linux in CI

* Make display name consistent

* Fix indent

* Improve token regex

* Add connectionToken

* update distro

* Update search stop icon

* Update checkmark so they look more like a ✓

* introduce RemoteAuthorities

* remote explorer and contribution under proposed api

* Disable smoke tests on Linux

Puppeteer needs special user setup in order to launch

* Strict init

microsoft#78168

* Strict init and mark events readonly

microsoft#78168

* Remove extra null checks in coalesce

The type system should catch these now

* Add telemetry+warning for webviews that don't have a content security policy

Fixes microsoft#79248

* Fixing comment

* Update distro

* Update distro

* Don't dispose of added object in already disposed of case

Fixes microsoft#77192

See microsoft#77192 for discussion

* Mark readonly

* Use const enums

* Marking fields readonly

* Remove webview svg whitelist

This is no longer required

* Removing test for disposable store

We have disabled this behavior

* build - disable smoketest

* Enable new Octicons style by default

* fix web platform check

* better worker error logging

* make sure to prepend vs/nls

* 💄

* fix: keep the two "Copy Path" behavior consistent

When using remote, this "Copy Path" function of SearchAction will keep the remote prefix while the FileCommand will not.

Change-Id: Ide00d2da5a695d0adbe87622643c7a600dd46432

* Load Octicons through ts instead of css import

* update distro

* Fixes microsoft#79166

* web - synchronise global state changes

* explorer input black magic

fixes microsoft#78153

* fix microsoft#72417

* Fix smoke tests

* Revert "build - disable smoketest"

This reverts commit c23cacd.

* Scope new variable to a string

* Fallback to default when cwd var cannot be resolved

Fixes microsoft#79281

* fix error when dismissing snippets picker

* add fetch file system provider

* add IStaticExtensionsService, add `staticExtensions` to embedder API

* make staticExtensions optional

* Indicate web in smoke test step

* Update octicon css logic

* Fix Octicon icons on Linux

* debug: introduce data breakpoints

* Introduce and adopt asCSSUrl

* Fixes microsoft/vscode-remote-release#687:
- remove ipc message that passes over the resolved authority
- don't go through the protocol handler when hitting 127.0.0.1

* Introduce machine overridable setting

* Seti uses TS-icon for jsonc-language. Fixes microsoft#78261

* Pick up TS rc

* [email protected]

- Better align search with how monaco does it

Fixes microsoft#78281

* Format file

* Update distro

* Remove not null suppression

* Use dispoablestore

* Use closure instead of parameters

* Use type for IExtensionPointHandler and mark array readonly

* Marking arrays readonly in ExtensionPointUserDelta

* web - storage/lifecycle 💄

* Fix microsoft#79326, cleanup rendering Octicons bugs on Windows/Linux

* fix Markdown Preview scroll remains same after clicking on some other link

* Fix trivial zsh completion typo

* Improvements

* Drop Configure Trusted Domains command for now

* 💄

* Drop details & on -> from

* Address feedbacks

* Fix services

* Storage instead of setting. Add command for configuring

* Don't add star to quickpick

* Update UX

* Build errors and test failures

* 💄

* debt - fix some layer breakers

* Added buildReplaceStringWithCasePreserved generic function to be  used in both search side and the find side

* 💄 imports

* refactor: GlobalStorageDatabaseChannel should dependents in IStorageMainService (microsoft#79387)

* 💄

* build - have different worker extension host entrypoint, copy worker bootstrap file

* fix microsoft#76573

* fix path

* update distro

* Localization test fails on remote smoketest. Fixes microsoft#78412

* Fix microsoft/vscode-remote-release/issues/1027

* fix scope when falling back to original load function

* Wording update

* Fix: Markdown Preview scroll remains same after clicking on some other link microsoft#78465

Improves the behavior on how markdown preview behaves when clicking a link

* microsoft#79429

* TSLint: show a warning when accessing node.js globals in common|browser (microsoft#79222)

* trivial first cut

* document where globals are from

* improve rule detection

* fix "gulp tslint" task

* share rules

* enable more rules

* also add a rule for DOM

* Update trustedDomain default config

* tslint - disable some rules with comments (microsoft#79454)

* Drop unneeded action registration

* update distro

* Support using csharp in markdown preview to identify c# code blocks

* sync diagnostics, fixes microsoft#47292

* fixes microsoft#79280

* tests - more diagnostics for getUntitledWorkspaceSync() random failures

* debt - reduce dependencies diff to E6 branch

* update distro

* debt - menubar service should not live in common (microsoft#79181)

* distro

* Unify trusted domain language and use quick pick item id

* Update browser telemetry common properties

* microsoft#79454 Fix warnings

* Fix microsoft#79429

* Fix hygiene check
@pavelfeldman
Copy link
Member

Happy to see this moving along. A couple of comments from my side:

  • Don't render expansion control when there are no children!
    All my sessions have single thread and as a result, they are all expandable, but due to inlining, they are all empty upon expansion.

  • When there is only one top-level session, inline its children.
    As seen on the screenshot below, my launch configuration corresponds to a single 'root' session. Rendering that group "Web page 2" does not seem to be necessary.

  • When the web page navigates between the URLs, its thread name changes. I'd be great to either allow session to be renamed or to render the thread name in the tree instead. I can force thread rename via removing and re-adding it back today.

threads

@pavelfeldman
Copy link
Member

One more:

  • As I'm typing in console of a particular session, should a new session be created, it becomes active and steals the console from under me.

@pavelfeldman
Copy link
Member

Another one:

  • When my session has a child session and a thread, I still expect to see my only thread inlined. I.e. in my case session === thread. This does not seem to be the case today, session blank has a thread localhost5002 and a child session sw.js:

threads2

At the same time, my expectations are:

  • localhost:5002 (session and thread)
    • sw.js (session and thread)
  • serve.js [51030] (session and thread)

isidorn added a commit that referenced this issue Aug 21, 2019
@isidorn
Copy link
Contributor Author

isidorn commented Aug 21, 2019

@pavelfeldman thanks a lot for the great feedback. We usually prefer seperate new issues, but let's continue the discussion here and we can open new issues.

  1. Expansion control when there are no children: currently we do this everywhere across VS Code (Explorer, Variables view, Watch Expression). If we were to change this we would have to fetch children even before the element gets expanded, just to know if he has any children or not. What we currently do is that the thread should not be shown as expandble if it is not stopped here. So the question for me is why the Thread in the model is marked as stopped when it is clearly running. I would need repro steps to investigate more here.
  2. This makes sense, I have just tackled this via 72803aa Previously we would always show the session if we were once in a mulit session view (and you go back to one session). Though always saving space makes sense and I like this change. Please note that in the debug toolbar we will still show the dropdown such that the user is aware that there were multiple sessions in the past and that he is left with just one.
  3. This should already be possible. Similar issue When name of thread is changed, the UI is not updated.  #18244. Please try to send a ThreadEvent and as a reason set new. VSCode will ask you for all the threads and you just return with the updated names. And we should handle it nicely here. Maybe we shuold just add a reason "update" just to make this more clear in the DAP @weinand
  4. Similar issue here Multi-target debugging: Avoid automatic switching debug console focus #77836 Let's continue the discussion there
  5. This makes sense. I have just tackled this via b8ec1d7

Thanks again for the feedback.

fyi @weinand

isidorn added a commit that referenced this issue Aug 21, 2019
@isidorn
Copy link
Contributor Author

isidorn commented Aug 21, 2019

  1. Upon further investigation I have pushed 38aa476 which should tackle this issue in your case. We were auto focusing the newly created session even if it was not stopped which was wrong. So I changed that, we no longer switch focus when a new session is created. However our approach remains that if a breakpoint is hit we will automatically focus on the coresponding session. Please try it out and let me know how it behaves. I would be interested to hear your feedback on our auto focus behavior when there are multiple threads / sessions. Thanks

@weinand
Copy link
Contributor

weinand commented Aug 28, 2019

I've verified that this works as discussed with two instances of a simple long-running node program:

setInterval(() => {
	console.log("tick");
}, 1000);

@isidorn What I find a bit surprising is the following:

If there are no breakpoints set, starting one debug session does not show anything in the CALLSTACK view. This is only mildly surprising because VS Code always behaved like this for single-threaded programs.

Starting the second debug session results in two sessions:
2019-08-28_16-53-52

Terminating the second session makes both items disappear which gives the wrong impression that both sessions have terminated. It would be more intuitive if we would not optimise in the n-> 1 transition and continue to show a single session. This is a strategy that we are already using in other places.

@isidorn
Copy link
Contributor Author

isidorn commented Aug 28, 2019

@weinand agree that that one is a bit of a bad corner case and that users might think that both sessions are gone.
We can go back to the previous behavior (not optimising the n->1 transition).

We could also introduce a new heuristic instead: if a session is running (has no threads) show it in the call stack view always. That way we would also tackle the first surprising scenario.

@weinand @pavelfeldman let me know what you think and which approach you prefer.

@pavelfeldman
Copy link
Member

No strong preference here. I would still optimize for n -> 1 and see if there is any confusion / incoming bug reports.

My thinking here is that empty CALL STACK is not alarming since that's what most users saw when working with a single session / single thread anyways.

@vscodebot vscodebot bot locked and limited conversation to collaborators Sep 29, 2019
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 feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants