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

Tree view enhancement #6588

Merged
merged 16 commits into from
Jan 12, 2023
Merged

Tree view enhancement #6588

merged 16 commits into from
Jan 12, 2023

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Oct 17, 2022

This PR improves the tree view by:

  • activating by default all newly created tabs.
  • adding a command to open the file browser tab. This is useful when another tab send the command go-to-path, now the file browser is activated after the path has changed.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch brichet/notebook/tree-view-enhancement

window.open(`${baseUrl}tree`);
}
});
if (PageConfig.getOption('notebookPage') !== 'tree') {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we need this check, or simply let folks open the file browser again if they want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact the filebrowser-extension already adds an entry in menu View:
https://github.com/jupyterlab/jupyterlab/blob/fc3f6ab716ef82944ab6c4e5d1a06418daad49dc/packages/filebrowser-extension/schema/browser.json#L24-L29
That would be a duplicated entry, one to switch to file browser tab, and one to open a new page.
This is also why I renamed the Open Files entry to File Browser, for consistency with the JupyterLab one.

Copy link
Member

@jtpio jtpio Oct 20, 2022

Choose a reason for hiding this comment

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

In that case maybe it would useful to use isVisible when defining this command? So it's available on all pages of the notebook application in case some other extensions need it, but not visible on the notebook page if desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know about that one.
I just realized that clicking on the same menu entry File Browser when the file browser is activated try to call filebrowser:hide, which is not currently defined in notebook. This raises an error in console.
Do you think we should defined the command filebrowser:hide without any action, to avoid the error ?

Copy link
Member

Choose a reason for hiding this comment

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

Does it? I just tried with this menu entry on Binder but it does not seem to be printing anything:

image

Copy link
Contributor Author

@brichet brichet Oct 24, 2022

Choose a reason for hiding this comment

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

On binder too I have this error message in the console, when the file browser is already activated before clicking on the link:

The command added by JupyterLab is filebrowser:toggle-main, which calls filebrowser:activate or filebrowser:hide-main depending on the isHidden status. Maybe it is not triggered every time for some reason.

BTW I changed the way this command is defined (using isVisible) in 375ebd3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we should defined the command filebrowser:hide without any action, to avoid the error ?

We could also make the file browser tab 'closeable' and define properly the command filebrowser:hide.
Not sure this is expected for Jupyter Notebook, that may change a bit the philosophy.

Copy link
Member

Choose a reason for hiding this comment

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

I guess users might indeed close it by mistake and won't know what to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.
It can be solved by fixing it in JupyterLab, as explain below: suppress the filebrowser:toggle-main entry from menu and create one only for filebrowser:activate.

@jtpio jtpio added this to the 7.0 milestone Oct 18, 2022
window.open(`${baseUrl}tree`);
}
});
if (!app.commands.isVisible('filebrowser:toggle-main')) {
Copy link
Member

Choose a reason for hiding this comment

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

I was more wondering whether it would be possible to define isVisible as part of the command definition below.

Copy link
Member

Choose a reason for hiding this comment

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

@brichet would you mind posting a summary of what the intended behavior should be, to make sure I understand correctly? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be confused about that, I'll try to clarify.

There are 2 different things:

1- What I was expected at first was a command to activate the file browser in tree view from another tab. For example nbgrader brings some tabs with links which open a path in file browser, using filebrowser:go-to-path command. This command already exists in tree view, it is created when importing the plugin '@jupyterlab/filebrowser-extension:browser'. But at the end, this command call the command filebrowser:activate, which is not registered in Notebook yet. The result is that the path is opened but the file browser tab is not activated, so it look like nothing happened for user.
This is fixed with the command below.

2- The plugin '@jupyterlab/filebrowser-extension:browser' also bring a command filebrowser:toggle-main, with a menu entry called Open Files, which call filebrowser:activate or filebrowser:hide-main depending on the file browser status. I thought we could use this entry already existing in tree view to open the file browser tab, instead of creating a new one which may be confusing.

Maybe the good thing to do is:

  • modifying the creation of the command filebrowser:toggle-main in JupyterLab, to register it (and create menu entry) in the plugin '@jupyterlab/filebrowser-extension:browserWidget' instead of '@jupyterlab/filebrowser-extension:browser'. this would avoid importing it in Notebook and having this menu entry by default in tree view.
  • creating a menu entry to open the file browser tab in Notebook when we create the command 'filebrowser:activate'.
  • creating the menu entry to open a new tree view page only if the previous one does not exists (as you said).

Hope it clarifies.

factory: IFileBrowserFactory
) => {
const { commands } = app;
commands.addCommand('filebrowser:activate', {
Copy link
Member

Choose a reason for hiding this comment

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

Defining a filebrowser:activate command to activate the filebrowser sounds fine though 👍

@brichet brichet force-pushed the tree-view-enhancement branch 2 times, most recently from 7db0123 to fd3e8df Compare January 10, 2023 14:28
@brichet brichet marked this pull request as ready for review January 10, 2023 14:30
execute: () => {
window.open(`${baseUrl}tree`);
if (page === 'tree') {
app.commands.execute(filebrowserCommandIDs.activate);
Copy link
Member

Choose a reason for hiding this comment

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

To avoid bringing a dependency on @jupyter-notebook/tree for @jupyter-notebook/application-extension, maybe we should just use the filebrowser:activate string here directly?

Copy link
Member

Choose a reason for hiding this comment

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

For example this is what is done in the JupyterLab docmanager-extension, using a command defined in another plugin: https://github.com/jupyterlab/jupyterlab/blob/d614e293eb4b90055307828d5e81c9661e1147c9/packages/docmanager-extension/src/index.tsx#L1125

Copy link
Contributor Author

@brichet brichet Jan 11, 2023

Choose a reason for hiding this comment

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

Sure, perhaps we should do the same in tree-extension:openFileBrowser and drop the CommandIDs namespace in tree.

EDIT: just saw the comment below.

/**
* The namespace for command IDs.
*/
export namespace CommandIDs {
Copy link
Member

Choose a reason for hiding this comment

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

With https://github.com/jupyter/notebook/pull/6588/files#r1067176298 above, these CommandIDs could be moved to the tree-extension package.

@@ -40,6 +40,7 @@
},
"dependencies": {
"@jupyter-notebook/application": "^7.0.0-alpha.10",
"@jupyter-notebook/tree": "^7.0.0-alpha.10",
Copy link
Member

Choose a reason for hiding this comment

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

Is this dependency still needed after the last commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks

@@ -242,6 +242,11 @@ const menuSpacer: JupyterFrontEndPlugin<void> = {

/**
* Add commands to open the tree and running pages.
*
* ## NOTES:
* The optional token IFileBrowserCommands is useful to ensure the corresponding
Copy link
Member

Choose a reason for hiding this comment

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

Also this comment can probably be removed now.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit a0b3a80 into jupyter:main Jan 12, 2023
@brichet
Copy link
Contributor Author

brichet commented Jan 12, 2023

Thanks a lot for reviews @jtpio, especially since this PR was pretty confusing at first.

@brichet brichet deleted the tree-view-enhancement branch January 12, 2023 14:10
@jtpio
Copy link
Member

jtpio commented Jan 19, 2023

FYI @brichet a new pre-release is now available, which includes this change: https://github.com/jupyter/notebook/releases/tag/v7.0.0a11

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants