-
Notifications
You must be signed in to change notification settings - Fork 289
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
Select Kernel Source Per Document Experiment #11632
Conversation
Build is currently broken, but that's on an unrelated change currently in main, will update branch when main is fixed. |
@@ -73,4 +77,9 @@ export class KernelFinder implements IKernelFinder { | |||
|
|||
return kernels; | |||
} | |||
|
|||
// Give the info for what kernel finders are currently registered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added this so that the UI could see what kernel finders are registered. I didn't just go off the controllers that we see registered since if you add a kernel source with no controllers you would still want to see it as an option in the UI so you know that you connected to the server correctly.
|
||
// Only show this command if we are in our Insiders picker type | ||
private updateVisibility() { | ||
if (this.configService.getSettings().kernelPickerType === 'Insiders') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we are not picking up the context key change in the kernel picker so after changing the setting, a window reload is required for it to work. We might need a fix in the core.
The code looks good to me. Played around with it a bit and ran into this interesting scenario:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, however please move the code to update kernelFinderInfo
into the finder itself, as the finder owns creation of the KernelConnectionMetadata
, this way we know that the KernelConnectionMeatadata
is created and populated in one place as opposed to multiple different locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry but the connections should be ready only
#11576
This PR completes shipping the basic prototype for selecting a kernel source per document. There are quite a few known hitches here still so I'll list a few.