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

Reuse same DebugSessionWidget for all debug sessions. #11277

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Jun 8, 2022

What it does

Fixes #9930 by using the same DebugSessionWidget for all debug sessions. It removes a fair bit of code that assumes that we will open many DebugSessionWidgets, which could break downstream applications that exploit that assumption.

How to test

  1. Start more than one debug session (e.g. start one using the Run Mocha test configuration, and another on a basic JS file using the dynamic session)
  2. Switch between sessions.
  3. Assert that switching between sessions updates widgets appropriately (or at least as on master) and that the debug toolbar controls the currently selected session.
  4. Move some of the children of the DebugSessionWidget into other view containers, and assert that (3) is still true.
  5. After moving some of the children of the DebugSessionWidget, restart the application.
  6. Assert that the moved children remain where they were before the restart.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added debug issues that related to debug functionality potentially-breaking A change that might break some dependents conditionally labels Jun 8, 2022
@colin-grant-work colin-grant-work marked this pull request as ready for review June 9, 2022 18:34
@msujew msujew self-requested a review June 10, 2022 14:02
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

That's a lot of removed code :D

Anyway, I couldn't notice any regressions from master and the improved behavior works correctly

  • Moving a debug view part to another view container works as expected
  • Changing to another debug session (in one container) updates the call stack view (in another container) correctly
  • Moved view parts stay where they are supposed to be after reloading the app

A minor thing which also doesn't work on master currently is that switching between debug sessions doesn't update the select component (but correctly updates the debug repl):

image

This is likely out of scope for this PR, but I just wanted to write it down somewhere.

@colin-grant-work
Copy link
Contributor Author

@thegecko, I know that debugging is a topic of interest to ARM. Is the ability to open multiple sessions simultaneously in different areas important to you?

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright, looks good 👍

@thegecko
Copy link
Member

@thegecko, I know that debugging is a topic of interest to ARM. Is the ability to open multiple sessions simultaneously in different areas important to you?

Thanks for the heads up. At the moment, we don't require support for running multiple debuggers with separate UI panels.
As long as this works (as confirmed by others above) and this mimics the VS Code workflow then I'm happy to get this merged.

In fact it makes my open PR a little simpler (although it will need rewriting). I'll revisit that when this is merged:

#10748

Copy link
Member

@thegecko thegecko left a comment

Choose a reason for hiding this comment

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

Code looks good and tested the debugger still works as expected

@thegecko thegecko merged commit 106df5b into eclipse-theia:master Jun 17, 2022
@colin-grant-work colin-grant-work deleted the quality/debug-widgets branch June 24, 2022 17:07
@colin-grant-work colin-grant-work added this to the 1.27.0 milestone Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality potentially-breaking A change that might break some dependents conditionally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug Session Widget Creation Routine Prevents Drag-Drop
3 participants