-
Notifications
You must be signed in to change notification settings - Fork 297
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
Support remote commands in the kernel picker #10602
Changes from all commits
c1bb432
9690f55
e667411
22f7599
c610482
4e05ded
4a3e97b
a4e478c
ca03019
5c5a7db
0af1629
abd9596
da88bc5
4361612
a8073e8
5ab57a7
dc68e1e
d8a013a
40ea168
ed9d88e
efcafc8
c188e9f
14f9dcc
13d7912
9094eb8
f98ba1f
12db879
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Rework kernel selection to be in either 'remote' mode or 'local' mode to avoid confusion about what kernels should be displayed. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fixes problem where clipboard permissions are required in order to enter a Jupyter server URL. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix problem of determining whether or not in 'local' or 'remote' mode for a jupyer connection. |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,30 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
import { inject, injectable, named } from 'inversify'; | ||
import { EventEmitter, Memento } from 'vscode'; | ||
import { Event, EventEmitter, Memento } from 'vscode'; | ||
import { | ||
IWorkspaceService, | ||
IEncryptedStorage, | ||
IApplicationEnvironment | ||
} from '../../../platform/common/application/types'; | ||
import { Settings } from '../../../platform/common/constants'; | ||
import { getFilePath } from '../../../platform/common/platform/fs-paths'; | ||
import { ICryptoUtils, IMemento, GLOBAL_MEMENTO } from '../../../platform/common/types'; | ||
import { IJupyterServerUriStorage } from '../types'; | ||
import { ServerConnectionType } from './serverConnectionType'; | ||
import { ICryptoUtils, IMemento, GLOBAL_MEMENTO, IsWebExtension } from '../../../platform/common/types'; | ||
import { computeServerId } from '../jupyterUtils'; | ||
import { IJupyterServerUriStorage, IServerConnectionType } from '../types'; | ||
|
||
export const mementoKeyToIndicateIfConnectingToLocalKernelsOnly = 'connectToLocalKernelsOnly'; | ||
export const currentServerHashKey = 'currentServerHash'; | ||
|
||
/** | ||
* Class for storing Jupyter Server URI values | ||
*/ | ||
@injectable() | ||
export class JupyterServerUriStorage implements IJupyterServerUriStorage { | ||
export class JupyterServerUriStorage implements IJupyterServerUriStorage, IServerConnectionType { | ||
private lastSavedList?: Promise<{ uri: string; time: number; displayName?: string | undefined }[]>; | ||
private currentUriPromise: Promise<string> | undefined; | ||
private _currentServerId: string | undefined; | ||
private _localOnly: boolean = false; | ||
private _onDidChangeUri = new EventEmitter<void>(); | ||
public get onDidChangeUri() { | ||
return this._onDidChangeUri.event; | ||
|
@@ -28,14 +33,29 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage { | |
public get onDidRemoveUris() { | ||
return this._onDidRemoveUris.event; | ||
} | ||
public get currentServerId(): string | undefined { | ||
return this._currentServerId; | ||
} | ||
public get onDidChange(): Event<void> { | ||
return this._onDidChangeUri.event; | ||
} | ||
public get isLocalLaunch(): boolean { | ||
return this._localOnly; | ||
} | ||
constructor( | ||
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, | ||
@inject(ICryptoUtils) private readonly crypto: ICryptoUtils, | ||
@inject(IEncryptedStorage) private readonly encryptedStorage: IEncryptedStorage, | ||
@inject(IApplicationEnvironment) private readonly appEnv: IApplicationEnvironment, | ||
@inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalMemento: Memento, | ||
@inject(ServerConnectionType) private readonly serverConnectionType: ServerConnectionType | ||
@inject(IsWebExtension) readonly isWebExtension: boolean | ||
) { | ||
// Remember if local only | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. THis is the code that was originally in the serverConnectionType.ts file. |
||
this._localOnly = isWebExtension | ||
? false | ||
: this.globalMemento.get<boolean>(mementoKeyToIndicateIfConnectingToLocalKernelsOnly, true); | ||
this._currentServerId = this.globalMemento.get<string | undefined>(currentServerHashKey, undefined); | ||
|
||
// Cache our current state so we don't keep asking for it from the encrypted storage | ||
this.getUri().ignoreErrors(); | ||
} | ||
|
@@ -182,33 +202,45 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage { | |
await this.setUri(Settings.JupyterServerLocalLaunch); | ||
} | ||
public async setUriToRemote(uri: string, displayName: string): Promise<void> { | ||
await this.setUri(uri); | ||
// Make sure to add to the saved list before we set the uri. Otherwise | ||
// handlers for the URI changing will use the saved list to make sure the | ||
// server id matches | ||
await this.addToUriList(uri, Date.now(), displayName); | ||
await this.setUri(uri); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like addToUriList is happening in two spots? Both here and in setUri, do we need both? |
||
} | ||
|
||
public async setUri(uri: string) { | ||
// Set the URI as our current state | ||
this.currentUriPromise = Promise.resolve(uri); | ||
if (uri === Settings.JupyterServerLocalLaunch) { | ||
await this.serverConnectionType.setIsLocalLaunch(true); | ||
} else { | ||
this._currentServerId = computeServerId(uri); | ||
this._localOnly = uri === Settings.JupyterServerLocalLaunch || uri === undefined; | ||
this._onDidChangeUri.fire(); // Needs to happen as soon as we change so that dependencies update synchronously | ||
|
||
// No update the async parts | ||
await this.globalMemento.update(mementoKeyToIndicateIfConnectingToLocalKernelsOnly, this._localOnly); | ||
await this.globalMemento.update(currentServerHashKey, this._currentServerId); | ||
|
||
if (!this._localOnly) { | ||
await this.addToUriList(uri, Date.now(), uri); | ||
await this.serverConnectionType.setIsLocalLaunch(false); | ||
|
||
// Save in the storage (unique account per workspace) | ||
const key = this.getUriAccountKey(); | ||
await this.encryptedStorage.store(Settings.JupyterServerRemoteLaunchService, key, uri); | ||
} | ||
this._onDidChangeUri.fire(); | ||
} | ||
private async getUriInternal(): Promise<string> { | ||
if (this.serverConnectionType.isLocalLaunch) { | ||
if (this.isLocalLaunch) { | ||
return Settings.JupyterServerLocalLaunch; | ||
} else { | ||
// Should be stored in encrypted storage based on the workspace | ||
const key = this.getUriAccountKey(); | ||
const storedUri = await this.encryptedStorage.retrieve(Settings.JupyterServerRemoteLaunchService, key); | ||
|
||
// Update server id if not already set | ||
if (!this._currentServerId && storedUri) { | ||
this._currentServerId = computeServerId(storedUri); | ||
} | ||
|
||
return storedUri || Settings.JupyterServerLocalLaunch; | ||
} | ||
} | ||
|
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.
@IanMatthewHuff looks like this will need to be reviewed closly as you're also working in the similar area (installing python when its missing). I.e. you might want to review the workflow & the changes.
Feels like this stuff related to no python might require its own folder/set of classes & its been worked on for months if not years (i.e. workflow keeps changing, hence better to keep it isolated & easier to manage)
Agai, I haven't reviewed anything yet, but just a thought about this (prompting to install python - getting started) part of the exetnsion
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.
@DonJayamanne I think Rich was just adding on the Command localization that I missed in my addition. The only real conflict to consider here is just the fact that we won't have only one command anymore in the new user scenario. Means that we don't get that nice "just hit the run button" launch, but we can tweak that as we move forward. Maybe prioritization for commands in the kernel picker somehow.