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 statusbar + toolbar item for selecting jupyter #4424

Merged
merged 7 commits into from
Jan 21, 2021

Conversation

DonJayamanne
Copy link
Contributor

For #254

  • Adding an item in the statusbar and toolbar for native notebooks
  • Changing existing command so we can track how the UI is displayed (whether from command, toolbar, statusbar).

@DonJayamanne DonJayamanne marked this pull request as ready for review January 20, 2021 00:10
@DonJayamanne DonJayamanne requested a review from a team as a code owner January 20, 2021 00:10
package.nls.json Outdated Show resolved Hide resolved
@@ -149,4 +148,7 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu
[DSCommands.SubmitGitHubIssue]: [];
[DSCommands.ShowDataViewer]: [IShowDataViewerFromVariablePanel];
[DSCommands.ClearSavedJupyterUris]: [];
[DSCommands.SelectJupyterURI]: [undefined, 'toolbar' | undefined];
[DSCommands.SelectNativeJupyterUriFromStatusBar]: [];
[DSCommands.SelectNativeJupyterUriFromToolBar]: [];
Copy link
Member

Choose a reason for hiding this comment

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

We seem to have a set of commands with arguments and a set without. Seems like these two new ones don't have arguments. But...does this matter? Not sure if I see a reason for the distinction.

Copy link
Member

@IanMatthewHuff IanMatthewHuff Jan 20, 2021

Choose a reason for hiding this comment

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

Also, why | undefined? Isn't 'commandPalette' or the other sources options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because SelectJupyterUri command can only be invoked from the command palette in which case the argument is undefiend.
The only other place its used is Interactive & WebView notebooks where we pass the argument toolbar to indicate its been invoked from the interactive/webview notebook toolbar.

Other than that there are no places where this command is invoked from.

We have similar commands that do the exact same thing, launch the UI. However we have separate commands so we can uniquely identify how and where those commands are launched. Pity we cannot use the same command with arguments (in VSC toolbar and statusbars).
I know in treeviews we can use commands with args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope that makes sense. The complication comes from the fact that we want to identify the differnt places the same command is launched, hence duplicate commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, just found we can use args in the statusbar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by removing one of the duplicate commands.

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

There are a couple of small cleanup issues (spelling and command arguments) but code looks legit so approving with cleanup.

this.statusBarItem.show();
const uri = await this.serverUriStorage.getUri();
const label =
uri === Settings.JupyterServerLocalLaunch
Copy link
Contributor

Choose a reason for hiding this comment

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

For remote we're not showing the URI? It's just going to say 'jupyter server: remote'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, else we end up taking a whole lot of space in the status bar.
Ooops, I'm meant to display the Uri in the tooltip.

@codecov-io
Copy link

codecov-io commented Jan 21, 2021

Codecov Report

Merging #4424 (71361d4) into main (27cfffe) will increase coverage by 0%.
The diff coverage is 93%.

@@          Coverage Diff           @@
##            main   #4424    +/-   ##
======================================
  Coverage     75%     75%            
======================================
  Files        392     397     +5     
  Lines      25746   25924   +178     
  Branches    3680    3709    +29     
======================================
+ Hits       19407   19615   +208     
+ Misses      4826    4799    -27     
+ Partials    1513    1510     -3     
Impacted Files Coverage Δ
.../datascience/interactive-common/interactiveBase.ts 71% <ø> (ø)
src/client/datascience/types.ts 100% <ø> (ø)
src/client/telemetry/index.ts 80% <ø> (ø)
src/client/datascience/notebook/remoteSwitcher.ts 91% <91%> (ø)
src/client/common/utils/localize.ts 96% <100%> (+<1%) ⬆️
src/client/datascience/commands/serverSelector.ts 86% <100%> (+<1%) ⬆️
src/client/datascience/constants.ts 99% <100%> (+<1%) ⬆️
src/client/datascience/jupyter/serverSelector.ts 73% <100%> (+<1%) ⬆️
src/client/datascience/jupyter/serverUriStorage.ts 82% <100%> (+<1%) ⬆️
src/client/datascience/notebook/serviceRegistry.ts 100% <100%> (ø)
... and 59 more

@DonJayamanne DonJayamanne merged commit 88f11c2 into main Jan 21, 2021
@DonJayamanne DonJayamanne deleted the remoteStatusAndIcon branch January 21, 2021 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants