Make session dialogs configurable #7618
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
blink1073
force-pushed
the
session-dialogs
branch
3 times, most recently
from
3c847c8
to
51eada0
Compare
|
Unfortunately, needs rebase. |
afshin
reviewed
Dec 23, 2019
| @@ -51,7 +51,7 @@ export interface ISessionContext extends IObservableDisposable { | |||
| * #### Notes | |||
| * This includes starting up an initial kernel if needed. | |||
| */ | |||
| initialize(): Promise<void>; | |||
| initialize(): Promise<boolean>; | |||
There was a problem hiding this comment.
Should there be a docstring that says what this boolean is?
afshin
reviewed
Dec 23, 2019
| @@ -531,7 +534,8 @@ function activateNotebookHandler( | |||
| launcher: ILauncher | null, | |||
| restorer: ILayoutRestorer | null, | |||
| mainMenu: IMainMenu | null, | |||
| settingRegistry: ISettingRegistry | null | |||
| settingRegistry: ISettingRegistry | null, | |||
| sessionDialogs: ISessionContextDialogs | |||
afshin
approved these changes
Dec 23, 2019
jasongrout
reviewed
Dec 23, 2019
| @@ -732,7 +698,7 @@ export class SessionContext implements ISessionContext { | |||
| this._session.dispose(); | |||
| } | |||
| this._session = session; | |||
| this._prevKernelName = session.kernel?.name; | |||
| this._prevKernelName = session.kernel?.name || ''; | |||
jasongrout
reviewed
Dec 23, 2019
packages/apputils/src/toolbar.tsx
Outdated
| return new ToolbarButton({ | ||
| iconClassName: 'jp-RefreshIcon', | ||
| onClick: () => { | ||
| void sessionContext.restart(); | ||
| void (dialogs || sessionContextDialogs).restart(sessionContext); |
There was a problem hiding this comment.
dialogs ?? sessionContextDialogs
jasongrout
reviewed
Dec 23, 2019
packages/apputils/src/toolbar.tsx
Outdated
| <Private.KernelNameComponent sessionContext={sessionContext} /> | ||
| <Private.KernelNameComponent | ||
| sessionContext={sessionContext} | ||
| dialogs={dialogs || sessionContextDialogs} |
jasongrout
reviewed
Dec 23, 2019
| ): Promise<IConsoleTracker> { | ||
| const manager = app.serviceManager; | ||
| const { commands, shell } = app; | ||
| const category = 'Console'; | ||
| sessionDialogs = sessionDialogs || sessionContextDialogs; |
jasongrout
reviewed
Dec 23, 2019
| @@ -136,11 +138,13 @@ async function activateConsole( | |||
| rendermime: IRenderMimeRegistry, | |||
| settingRegistry: ISettingRegistry, | |||
| launcher: ILauncher | null, | |||
| status: ILabStatus | null | |||
| status: ILabStatus | null, | |||
| sessionDialogs: ISessionContextDialogs | null | |||
There was a problem hiding this comment.
you could just make this have a default value: sessionDialogs = sessionContextDialogs
There was a problem hiding this comment.
wait, no, that won't work if null is explicitly being passed in...
There was a problem hiding this comment.
If the supplied value is null I believe that wouldn't work
|
This looks pretty good to me reading through it. I had a couple of suggestions, but nothing that would block this (but apparently you've already addressed them, so yeah). The test failure looks legit. |
Closed
added 9 commits
December 23, 2019 18:18
lint update notebook example offer the option to keep no kernel integrity lint fix up merge conflicts lint update tests Update tests Select a kernel if necessary in the console
blink1073
force-pushed
the
session-dialogs
branch
from
c10e258
to
92d1dc0
Compare
|
Weird, the issues with the linux usage test went away after a rebase. |
lock
bot
added
the
status:resolved-locked
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Fixes #7616.
Code changes
ISessionContextto no longer contain any UI.ISessionContextDialogsinterface and tokenrestartandselectKerneldialogs.User-facing changes
None
Backwards-incompatible changes
ISessionContextno longer hasrestart()orselectKernel()methods.ISessionContextshould allow the dialogs to be overridden and ultimately depend on theISessionContextDialogstoken.selectKernel()dialog ifinitialize()resolves totrue.