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

Add checkbox to skip showing the kernel restart dialog #16265

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

NexVeridian
Copy link
Contributor

@NexVeridian NexVeridian commented Apr 29, 2024

References

closes #8021 and closes #6420

Code changes

Add checkbox to skip showing the kernel restart dialog see #8021 (comment)

User-facing changes

2024-04-29.15-21-54.mp4

Backwards-incompatible changes

none

Thing that might need to be added

#6420 (comment)

a) [x] Do not ask again in this session: restart dialogs silenced until user reloads JupyterLab
b) [x] Do not ask again for this notebook: restart dialogs silenced for this specific notebook until user reloads JupyterLab
c) [x] Do not show this dialog ever again: disables the dialog altogether, user needs to go to settings to re-enable it

this pr does option b, hiding the dialogs is per file, and is reset every session

to convert to option c, the checkbox value would need to be persisted and added to settings like Automatically Start Preferred Kernel is, I don't think I'm going to able to implement that though, this is might good enough for now

Copy link

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

@NexVeridian NexVeridian force-pushed the Skip-showing-the-kernel-restart-dialog branch from 6a8cb71 to 1c940f0 Compare April 30, 2024 03:03
@fcollonval fcollonval added this to the 4.3.0 milestone May 1, 2024
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 a lot @NexVeridian

Would you feel comfortable implementing option (c) - never display the dialog through a setting instead of per session that cannot be undone?

To achieve that you will need to extend ISessionContext.IDialogsOptions to accept the ISettingRegistry token. And then you'll be able to do something like there:

const result = await InputDialog.getText({
title: trans.__('Rename file'),
okLabel: trans.__('Rename'),
placeholder: trans.__('File name'),
text: oldName,
selectionRange: oldName.length - PathExt.extname(oldName).length,
checkbox: {
label: trans.__('Do not ask me again.'),
caption: trans.__(
'If checked, you will not be asked to rename future untitled files when saving them.'
)
}
});
if (result.button.accept) {
newName = result.value ?? oldName;
(widget as IDocumentWidget).isUntitled = false;
if (typeof result.isChecked === 'boolean') {
const currentSetting = (
await settingRegistry.get(
docManagerPluginId,
'renameUntitledFileOnSave'
)
).composite as boolean;
if (result.isChecked === currentSetting) {
settingRegistry
.set(
docManagerPluginId,
'renameUntitledFileOnSave',
!result.isChecked
)
.catch(reason => {
console.error(
`Fail to set 'renameUntitledFileOnSave:\n${reason}`
);
});
}
}

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.

Very nice @NexVeridian you are on the right path.

The dialogs plugin is defined there:

const sessionDialogs: JupyterFrontEndPlugin<ISessionContextDialogs> = {
id: '@jupyterlab/apputils-extension:sessionDialogs',
description: 'Provides the session context dialogs.',
provides: ISessionContextDialogs,
optional: [ITranslator],
autoStart: true,
activate: async (app: JupyterFrontEnd, translator: ITranslator | null) => {
return new SessionContextDialogs({
translator: translator ?? nullTranslator
});
}
};

So you will have to add ISettingRegistry as additional optional dependency. Then you'll be able to pass it to SessionContextDialogs.

This also answers the question about the settings plugin. It should be @jupyterlab/apputils-extension:sessionDialogs. That implies you need to create in packages/apputils-extension/schema directory a new file named sessionDialogs.json (naming is important). That contains

{
  "title": "Kernel dialogs",
  "description": "Kernel dialogs settings.",
  "additionalProperties": false,
  "properties": {
    "skipKernelRestartDialog": {
      "title": "...", # I let you add a short description here
      "description": "...", # I let you add a long description here
      "type": "boolean",
      "default": false
    },
  },
  "type": "object"
}

packages/apputils/src/sessioncontext.tsx Outdated Show resolved Hide resolved
@NexVeridian NexVeridian force-pushed the Skip-showing-the-kernel-restart-dialog branch from 9d477b1 to f220e29 Compare May 2, 2024 19:53
@@ -1319,6 +1322,7 @@ export namespace SessionContext {
export class SessionContextDialogs implements ISessionContext.IDialogs {
constructor(options: ISessionContext.IDialogsOptions = {}) {
this._translator = options.translator ?? nullTranslator;
this._settingRegistry = options.settingRegistry || null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the setting page works now, but checkbox dialog linked to the setting doesn't

this._settingRegistry = options.settingRegistry || null; is undefined, when the setting is unset

so I think this need to be nullSettingRegistry which I can't find

@NexVeridian NexVeridian force-pushed the Skip-showing-the-kernel-restart-dialog branch from f6ce669 to f8e1aad Compare May 2, 2024 20:45
@krassowski
Copy link
Member

@fcollonval just for awareness, from a quick straw poll on #6420 it seems that option (b) may be preferred by a number of users. I think the consensus is that we should allow for both (b) and (c), but maybe the checkbox should be connected to (b)?

@NexVeridian NexVeridian force-pushed the Skip-showing-the-kernel-restart-dialog branch from 7e0c6a6 to 90bbf2f Compare May 15, 2024 22:54
@NexVeridian
Copy link
Contributor Author

Everything should be working now, by doing this

@fcollonval just for awareness, from a quick straw poll on #6420 it seems that option (b) may be preferred by a number of users. I think the consensus is that we should allow for both (b) and (c), but maybe the checkbox should be connected to (b)?

Or if you want to checkbox to be linked to c, then you could revert the last commit and fix this

using the setting page works now, but checkbox dialog linked to the setting doesn't

this._settingRegistry = options.settingRegistry || null; is undefined, when the setting is unset

so I think this need to be nullSettingRegistry which I can't find

@NexVeridian NexVeridian force-pushed the Skip-showing-the-kernel-restart-dialog branch from 5ff4578 to 90bbf2f Compare May 17, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning message when restarting Kernel Add command to allow kernel-restart without confirmation dialog
3 participants