Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Add life-cycle button in toolbar of notebook and console panel. #239

Merged
merged 7 commits into from
Dec 16, 2019
Merged

Add life-cycle button in toolbar of notebook and console panel. #239

merged 7 commits into from
Dec 16, 2019

Conversation

KsavinN
Copy link
Collaborator

@KsavinN KsavinN commented Nov 29, 2019 •
edited
Loading

related to #214 and #154

Now:

  • toggle should stop/start session.

  • toggle should appear only for the supported kernel.

  • user cant run debugging/ set breakpoints it toggle is off

  • if user set breakpoints and reload page client will be automatically turn on the toggle

@KsavinN KsavinN added the ui label Nov 29, 2019
@KsavinN KsavinN added this to the 0.1.0 milestone Nov 29, 2019
@KsavinN KsavinN self-assigned this Nov 29, 2019
@KsavinN
Copy link
Collaborator Author

KsavinN commented Nov 29, 2019 •
edited
Loading

onlyUI

Implement simple button switch in the toolbar of notebook and insert toolbar with button switch to console panel

jtpio
jtpio reviewed Nov 29, 2019
View reviewed changes
src/handlers/notebook.ts Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Dec 2, 2019

Thanks for posting a screencast, this looks promising!

jtpio
jtpio reviewed Dec 2, 2019
View reviewed changes
src/handlers/console.ts Outdated Show resolved Hide resolved
jtpio
jtpio reviewed Dec 2, 2019
View reviewed changes
src/handlers/notebook.ts Outdated Show resolved Hide resolved
@KsavinN KsavinN force-pushed the toggleLifeCycle branch 4 times, most recently from a1cd1cb to a1040cd Compare December 2, 2019 15:05
afshin
afshin reviewed Dec 4, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
afshin
afshin reviewed Dec 4, 2019
View reviewed changes
src/service.ts Outdated Show resolved Hide resolved
@KsavinN KsavinN force-pushed the toggleLifeCycle branch 2 times, most recently from 290e200 to 1e1ab77 Compare December 5, 2019 14:08
@jtpio jtpio mentioned this pull request Dec 6, 2019
Merged
jtpio
jtpio reviewed Dec 9, 2019
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Dec 10, 2019

It would be nice to rebase this PR to grab all the latest changes from master (including the single mode debugger).

@jtpio
Copy link
Member

jtpio commented Dec 10, 2019

Also @KsavinN let me know if you want to share the work on this. Otherwise I'm also happy to pick this up :)

@KsavinN
Copy link
Collaborator Author

KsavinN commented Dec 10, 2019

Also @KsavinN let me know if you want to share the work on this. Otherwise I'm also happy to pick this up :)

I start rebasing. And i think i will need today spend some time for reading all your changes which you made :)

@jtpio jtpio mentioned this pull request Dec 10, 2019
Closed
afshin
afshin reviewed Dec 10, 2019
View reviewed changes
src/handlers/console.ts Outdated Show resolved Hide resolved
@KsavinN KsavinN added the bug Something isn't working label Dec 13, 2019
@jtpio
Copy link
Member

jtpio commented Dec 13, 2019 •
edited
Loading

If I have nbA and nbB, both with debugging turned on, and I want to look at nbA while seeing the debug window for nbB, I turn off debugging for nbA. Does that mess up the debugging session for nbA?

The simpler option here would be to focus on nbB to look at the debug window for nbB, and focus back on nbA to look at the debug window for nbA?

Switching off debugging (via the button in this PR) will stop the debugger, and the state of the debug session will be lost.

Here is a screencast from a while back to illustrate the flow:

multi-notebook-debugging

Do you think there is a use case for looking at the debug state for a notebook that doesn't have the focus?

@SylvainCorlay
Copy link
Member

Do you think there is a use case for looking at the debug state for a notebook that doesn't have the focus?

I think that it would confuse the user experience.

@SylvainCorlay
Copy link
Member

That makes me realize it may be important to indicate which kernel is being debugged in the debugger pane.

@afshin
Copy link
Member

afshin commented Dec 13, 2019

@jtpio said: Do you think there is a use case for looking at the debug state for a notebook that doesn't have the focus?

@SylvainCorlay said: I think that it would confuse the user experience.

This came out of interviews I did with some potential users who specifically said they would like to have a view on the variable state of Notebook A potentially one in a breakpoint-stopped state while they were playing around with ideas in Notebook B. The toggle was to allow for both default-on (i.e., all notebooks always have debugging) and also for occasionally turning one off so you can look at a previous notebook's state.

@jtpio
Copy link
Member

jtpio commented Dec 13, 2019

That makes me realize it may be important to indicate which kernel is being debugged in the debugger pane.

Yes, we discussed that last time with @afshin and @tgeorgeux and it sounds like a good idea (but let's track that in a different issue).

@SylvainCorlay
Copy link
Member

@jtpio said: Do you think there is a use case for looking at the debug state for a notebook that doesn't have the focus?

@SylvainCorlay said: I think that it would confuse the user experience.

@afshin This came out of interviews I did with some potential users who specifically said they would like to have a view on the variable state of Notebook A potentially one in a breakpoint-stopped state while they were playing around with ideas in Notebook B. The toggle was to allow for both default-on (i.e., all notebooks always have debugging) and also for occasionally turning one off so you can look at a previous notebook's state.

Do you think we could do this after 0.1?

@jtpio Yes, we discussed that last time with @afshin and @tgeorgeux and it sounds like a good idea (but let's track that in a different issue).

Sounds good

@KsavinN KsavinN removed the bug Something isn't working label Dec 13, 2019
@afshin
Copy link
Member

afshin commented Dec 13, 2019

Do you think we could do this after 0.1?

The toggle behavior in this PR already supports that. But we should review workflows more carefully after 0.1, yes.

jtpio
jtpio reviewed Dec 13, 2019
View reviewed changes
src/index.ts Outdated
Comment on lines 158 to 159
if (this.handlers[widget.id]) {
if (!debug.session.debuggedClients.has(debug.session.client.path)) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the two if could be combined then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, definitely. Sorry need make break :)

@SylvainCorlay
Copy link
Member

The toggle behavior in this PR already supports that. But we should review workflows more carefully after 0.1, yes.

I thought it would simply make it behave like a non-debugging kernel?

@afshin
Copy link
Member

afshin commented Dec 13, 2019

@SylvainCorlay What I mean is that I'm inside non-debugging Notebook B and I just want the debugger to still show me the debugging Notebook A state. This PR supports that sort of use case. But it's not more sophisticated in terms of switching kernels in the debugger (which is an idea that came out of when Tim and Jeremy and I spoke and should wait until after 0.1).

@jtpio
Copy link
Member

jtpio commented Dec 16, 2019

Since this PR has been open for a quite a while and need to be rebased regularly, I would be fine merging it as is and address the remaining points in follow-up changes.

@afshin what do you think?

afshin
afshin approved these changes Dec 16, 2019
View reviewed changes
Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

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

Thanks @KsavinN!

@afshin
Copy link
Member

afshin commented Dec 16, 2019

@jtpio Yeah, let's merge and iterate, that's better.

@afshin afshin merged commit 28d4ab7 into jupyterlab:master Dec 16, 2019
This was referenced Dec 16, 2019
Closed
Closed
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jtpio jtpio jtpio left review comments

@afshin afshin afshin approved these changes

Assignees

@KsavinN KsavinN

Labels
ui
Projects
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@KsavinN @jtpio @jasongrout @afshin @SylvainCorlay