notebook icon indicating copy to clipboard operation
notebook copied to clipboard

The "Close and Shut Down Notebook" menu command discards state without confirmation

Open stanwest opened this issue 1 year ago • 8 comments
trafficstars

Problem

When I have a notebook open and select "Close and Shut Down Notebook" from the "File" menu of the notebook interface, notebook (as of v. 7.3.0a0) closes the kernel unconditionally, discarding the computational state. I recommend that the interface confirm that the user wants to close and shut down, as JupyterLab does (at least in v. 4.3.0a0) when one selects its "Close and Shut Down Notebook…" menu item.

Proposed Solution

Restore the confirmation dialog and add an ellipsis to the end of the text in the menu item.

Additional context

This behavior seems to have arisen in #7384.

stanwest avatar Jun 11 '24 21:06 stanwest

Hi @stanwest . I am new to open-source and want to contribute to this issue . @stanwest I came across this code snippet for close and shut down -> execute: async () => { // Shut the kernel down, without confirmation await commands.execute('notebook:shutdown-kernel', { activate: false }); window.close(); } in the packages/notebook-extension/src/index.ts path . To this problem

const confirmation = confirm('Are you sure you want to close the notebook and shut down the kernel?'); if (confirmation) { await commands.execute('notebook:shutdown-kernel', { activate: false }); window.close(); } would this be a right approach ? thoughI have not tested it yet .

I would highly appreciate your input,

Thanks .

Anakintano avatar Jun 15 '24 12:06 Anakintano

From Triage: The reason why @JasonWeill implemented this as is was for Notebook 6 parity. If you'd like to have this behavior, it would need to be implemented as a configuration option.

RRosio avatar Jun 18 '24 16:06 RRosio

The reason why @JasonWeill implemented this as is was for Notebook 6 parity.

Thank you for triaging this and commenting. I understand and agree that parity has some weight, and yet I believe it would be much better for Notebook to ask the user if they really do want to lose state.

If you'd like to have this behavior, it would need to be implemented as a configuration option.

I don't follow that it must be that way, but if it were an option, I would recommend that the default be to ask for confirmation. Users who want no confirmation as in v. 6 could set the option accordingly.

stanwest avatar Jun 24 '24 13:06 stanwest

Hi @jtpio, i would like to contribute to this issue and plan to start working on it. Please let me know if there are any specific guidelines or things i should be aware of.

sai-chand04 avatar Jul 18 '24 10:07 sai-chand04

@sai-chand04 here are the contributing guidelines: https://github.com/jupyter/notebook/blob/main/CONTRIBUTING.md

Please feel free to open a PR when you have something, even if it's not fully finished and still a draft.

Looks like the idea in this idea would be to implement a setting to allow for showing the dialog and allow for the behavior before https://github.com/jupyter/notebook/pull/7384.

Thanks!

jtpio avatar Jul 18 '24 12:07 jtpio

Thank you for the guidance! I have started working on allowing the dialog to show and restoring the behavior before #7384. I will open a PR soon, even if it's still in draft form, for initial feedback.

Thanks!

sai-chand04 avatar Jul 19 '24 16:07 sai-chand04

Hi @jtpio I have created a PR for this issue. The PR includes the changes for the restoring the close and shutdown confirmation dialog as of now. Could you please take a look and review the changes? Your feedback would be greatly appreciated. Thanks in advance for your time and guidance!

PR link: https://github.com/jupyter/notebook/pull/7430

sai-chand04 avatar Jul 25 '24 12:07 sai-chand04

Hi, I just pulled the changes of above PR #7430 opened for this issue and tested the changes locally. I was able to validate that one part that is providing a close and shutdown confirmation dialog is implemented correctly in the changes available in the PR.

Image

The second part that is to introduce a setting to let users override the default behaviour is not there with the changes in the PR. I see that this issue is open for quite some time now, I can take this issue forward and will try to implement the missing functionality in upcoming days and will open a PR once I am good with the implementation.

rsaditya01 avatar Apr 01 '25 17:04 rsaditya01