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

Collapse debugger panel when disabling debugger #13088

Merged
merged 5 commits into from Sep 30, 2022

Conversation

yanmulin
Copy link
Contributor

References

Addressing the issue: #13018

Code changes

listen to Debugger terminated event and collapse the debugger panel if it is visible.

User-facing changes

Before
Recording 2022-09-13 at 19 22 11

After
Recording 2022-09-13 at 19 20 12

Backwards-incompatible changes

None

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@welcome
Copy link

welcome bot commented Sep 14, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR @yanmulin

Would you mind not closing the tab if multiple debug sessions are active (see screencast) or place this behind a setting?

multiple-debug-sessions

@fcollonval fcollonval modified the milestones: 3.4.x, 4.0.0 Sep 14, 2022
@fcollonval
Copy link
Member

I don't think we can figure out if there are other opened widgets with an active debug session. @afshin is that correct?

@krassowski
Copy link
Member

krassowski commented Sep 14, 2022

Performance and UX wise:

  • closing the panel will cause layout shift and freeze apps if there are too many nodes (not that big of a problem with windowed rendering, but if someone has huge SVG on screen it still might happen)
  • for the same reason it will shift the content of all open editors/widgets, possibly leaving the user scrolling around to find where they were.

So ideally I would see the behaviour as switching to most recently used non-debugger sidebar widget on that side first and only if debugger is the only tab collapsing the right sidebar.

I'm not sure how feasible it is though.

@yanmulin
Copy link
Contributor Author

yanmulin commented Sep 20, 2022

@fcollonval A quick solution is to add a global variable to track the number of active debugger sessions. It increments when a session is started and decrements when a session is stopped. But not sure if it is a good idea.

@yanmulin
Copy link
Contributor Author

yanmulin commented Sep 20, 2022

@krassowski Couldn't trigger the layout shift on my side with a random big SVG found online. The cells just expand horizontally. Is that because the SVG not big enough?

Recording 2022-09-20 at 01 55 34

@fcollonval
Copy link
Member

A quick solution is to add a global variable to track the number of active debugger sessions. It increments when a session is started and decrements when a session is stopped. But not sure if it is a good idea.

I agree this is not an elegant solution - let's add a settings for this new feature. Do you need pointer for that?

Couldn't trigger the layout shift on my side with a random big SVG found online. The cells just expand horizontally. Is that because the SVG not big enough?

I think @krassowski was making a general comment that changing the width of a notebook will trigger style recalculation (and layout shift as the content distribution will change). This can freeze the UI if the notebook is large and/or contains highly complex views.

@yanmulin
Copy link
Contributor Author

yanmulin commented Sep 21, 2022

@fcollonval added a setting option.
Recording 2022-09-20 at 22 44 53 (1)

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Sorry for the late feedback @yanmulin.

Would you mind accepting the suggestion to make the settings follow the format name?

packages/debugger-extension/schema/main.json Outdated Show resolved Hide resolved
packages/debugger-extension/src/index.ts Outdated Show resolved Hide resolved
@krassowski
Copy link
Member

@yanmulin here the layout shift has two meanings:

  1. browser's need to re-compute the layout after a DOM/CSS change that could have shifted the layout
  2. user-perceivable movement of elements

I mentioned both together, but you do not the (2) to have the (1). The performance consequences of layout shift in the sense of (1) are more severe when there are many nodes; SVG visualisations can have thousands or even millions of nodes, but this is not a precondition of layout shift occurring.

Here is an example of (1) - in visible in the debugger and (2) - visible because the print output with expanded/collapsed side panel takes either one or two lines:

layout_shift

Even if we remove the print() call, the layout shift in the sense of (1) still occurs:

layout_shift_2

yanmulin and others added 2 commits September 29, 2022 20:17
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @yanmulin

@fcollonval fcollonval merged commit af65430 into jupyterlab:master Sep 30, 2022
@welcome
Copy link

welcome bot commented Sep 30, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@fcollonval fcollonval mentioned this pull request Sep 30, 2022
8 tasks
@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.5.x

fcollonval pushed a commit that referenced this pull request Oct 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants