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

Menu refactor #3182

Merged
merged 88 commits into from Nov 29, 2017
Merged

Menu refactor #3182

merged 88 commits into from Nov 29, 2017

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Nov 2, 2017

This is a very-much in-progress reworking of the menu layout for JupyterLab (following #3103). In particular, it defines a set of top-level menus that conform more closely with common desktop applications (File, Edit, View, Kernel, Run, Help). These menus will be accessible to plugins and provide extension points for various actions.

There are still a lot of question marks, but the basic structure is:

  1. The main menu is broken into its own plugin.
  2. The main menu interface defines menus for File, Edit, Run, View, Kernel, and Help.
  3. Each of these menus may define their own application level items (e.g. Settings in the File menu).
  4. Plugins can add their own groups to the menus (e.g., the help extension adds references to the Help menu).
  5. These top-level menus may define their own "semantic extension points", i.e., commands that may be morally equivalent between different plugins, allowing them to share a menu item. A proposed example of this is the Kernel menu, which allows the Notebook and Console extensions to share the menu items for interrupting, restarting, and changing kernels.

Questions:

  • Does this structure make sense?
  • Should some items exist in more than one menu?

cc @jasongrout @ellisonbg

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 16, 2017

Before getting too mired in the weeds, I'd appreciate a bit of feedback on whether the overall delegator pattern that I've been developing here (cf. ICodeRunner, IKernelUser) makes sense, and will suit our needs.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 23, 2017

@blink1073 can you comment on the test failure here? It looks like the python -m jupyterlab.update step is failing due to an as-yet unpublished package, and I am not sure how to fix that.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 24, 2017

Yeah, that was an oversight, you can remove the test.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 24, 2017

Okay, a recap of where this stands:

Overall, we have added Edit, Run, Kernel View menus. The File, Editor, and Help menus remain. The Terminal, Notebook, and Console menus have been removed; their items have been distributed among the other menus.

The top-level menus are meant to be opinionated ways for extensions to add themselves to the menu bar. Each of these menus has an addGroup() function, which allows plugins to add any group of related commands to that menu. I have been toying with the idea of making that the only way to add menu items to these menus, but at the moment this remains an additional function on top of the normal phosphor menus.

In addition, there are a number of what we are calling "semantic extension points", that is, menu items that carry some semantic meaning which may be relevant to many different plugins. Plugins can hook into these semantic extension points and provide execute() functions for them. In some places they can also provide extensions to the labels which are interpolated, allowing contextual labeling. When these menu items are triggered, a delegator function loops over the InstanceTrackers provided by the plugin, finds the plugin that owns the active widget, and invokes the execute function for it. In this way plugins can share the same menu item and keyboard shortcut.

At the moment I have defined the following semantic extension points:

File

  • ICloseAndCleaner, a command which closes the activity and also does some cleanup work for it. This is used by both the notebook and the console for "Close and Shutdown Kernel".
  • New submenu: a submenu that plugins can use to add activity creation commands. It is currently used by the notebook, text editor, terminal, and console.

Edit

  • IUndoer, commands which do undo/redo operations for the activity. It is currently used by the text editor and the notebook for undoing text operations (but not cell operations, in the case of the notebook).
  • IClearer, a command that performs some clearing related command for the activity. For the notebook, this command clears all outputs. For the console the command clears the cells.
  • IFindReplacer, commands which do find/replace operations for the activity. Currently only used by the text editor, but may soon be used by the notebook.

Run

  • ICodeRunner, commands which allow for the execution of code. It is currently used by the notebook, console, and text editor for invoking code running.

Kernel

  • IKernelUser, common commands for interacting with kernels. It is currently used by the notebook and console.
  • IConsoleCreator, a command for creating a console for a given activity. This is currently used by the notebook and the text editor.

View

  • IEditorViewer, commands related to the view state of an inline or document-mode editor. Currently used by the text editor and the notebook.

Help

No semantic extension points.

In places where a semantic extension point also has a keyboard shortcut, some extra care must be taken. Because keyboard shortcuts are identified using CSS selectors, it is possible for the menu items to lie to the user, since the isEnabled, execute, etc functions are not necessarily consistent with the local DOM structure. In these cases I have defined an HTML data attribute which widgets may add in order to hook into those keyboard shortcuts. For instance, the ICodeRunner "Run" command defines Shift-Enter as a shortcut, and widgets can add the attribute data-jp-code-runner in order to allow invoking of that command. Other shortcuts can still override the menu item semantic extension points by providing a more specific selector.

@ian-r-rose ian-r-rose changed the title [WIP] Menu refactor Menu refactor Nov 24, 2017
@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 25, 2017

This is looking great! So far the only gotcha I've found is that I'd expect Trust and Export To to go away when the console is the active item.

image

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 25, 2017

I'd also expect the unsupported edit operations to be grayed out here:

image

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 25, 2017

I think in general the top level menu items should only be enabled if the widget is the current widget in the dock panel.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 25, 2017

Yes, you have nailed one of the outstanding issues (this also applies to the terminal/notebook View menu items). I was also discussing this with @jasongrout the other day, and I think we are all in agreement. This is one of those points of difference between the behavior of the menu items in master and the behavior of the delegators. All of the semantic extension points operate on the Lab currentWidget, but the menu items in the old menus operate on the currentWidget of each tracker, which may not be the application currentWidget.

We can make the isEnabled handlers on the relevant commands check the application currentWidget to see if it is the same as the currentWidget on their tracker. Note that this would also change the behavior of which items are enabled in the command palette. We may also want to do this for all the commands which are not exposed in the menus/palette, for consistency's sake. At that point, this becomes a change with a rather larger footprint than those few menu items. It would also be possible to just register a (mostly) duplicate command that has different isEnabled semantics for those few menu items. Thoughts?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 27, 2017

I'd say the other menu items (and subgroups) should be invisible if they are not relevant to the current widget.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 27, 2017

One of the things that was impressed upon me by the Bloomberg UX folks is that people tend to go to menus to get a feel for the things that are possible to do in the application (even if not all are enabled at that moment). For that reason I have been reticent to make the menus too dynamic, which seems like it might be more confusing than helpful.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 27, 2017

I think the fact that the submenu looks active and you don't know that all of its children are disabled until you open it is my biggest concern.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 27, 2017

I believe this may be a limitation of the Phosphor menu as is, IIRC.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Nov 27, 2017

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 27, 2017

This would frustrate me immensely, thinking that I could export to a different format and then realizing I couldn't. Also, notice that we specifically mention Trust "Untitled1.ipynb".

image

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 27, 2017

I have fixed the naming of the notebook when it is not active. I am not sure what to do about disabling the submenu at the moment. This problem also affects the submenus in the Editor menu.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 27, 2017

Perhaps we could rename the Export to... submenu Export Notebook to... to emphasize it is for notebooks only?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 27, 2017

We certainly could export a code file or the contents of a terminal to another format, so maybe that should be a semantic endpoint?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 27, 2017

Yes, that could be a sensible way forwards. It looks like submenus don't have the same level of dynamism as regular commands, so the title would be static (and enabled).

At the end of the day, most activities will probably not have many exporters, so I think we would still see the issue of most/all of the formats being grayed out.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Nov 27, 2017

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 27, 2017

Looking more into the Export option, I still don't see any way to disable/hide a phosphor submenu based on context. Any ideas @sccolbert?

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Nov 27, 2017

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 27, 2017

So far as I can tell, all of the contextual information is based calling attributes of phosphor commands (label, isVisible, isEnabled, etc). I have been using these for the meta(/delegator) commands. However, as soon as the menu item is a submenu, I lose access to those.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 28, 2017

Hah, running afoul of Typescript 2.6.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 28, 2017

@ellisonbg I agree that we should schedule another session with the Bloomberg folks. At this point, this is already a large PR, and maintaining it would be fairly painful. The menu system here is more unified than the current status, and the main issues with it are either (a) blocked by the behavior of Phosphor or (b) could use some more expert feedback.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Nov 28, 2017

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Nov 28, 2017

Perhaps we should give @jasongrout a chance to weigh in, as I think he had planned to take this for a spin.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Nov 29, 2017

Thanks. I'll be able to look at this tomorrow morning.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Nov 29, 2017

Looks good, I think we ought to merge and iterate. Excelsior!

@jasongrout jasongrout merged commit d1193ab into jupyterlab:master Nov 29, 2017
2 checks passed
@ian-r-rose ian-r-rose deleted the menu-refactor branch Nov 29, 2017
@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 29, 2017

Excellent work on this, @ian-r-rose!

@blink1073 blink1073 mentioned this pull request Dec 2, 2017
@ian-r-rose ian-r-rose mentioned this pull request Dec 8, 2017
@jasongrout jasongrout removed this from the 1.0 milestone Feb 1, 2019
@jasongrout jasongrout added this to the 0.31.0 milestone Feb 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants