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

More context in context menu #4529

Closed
ian-r-rose opened this issue May 7, 2018 · 3 comments · Fixed by #4590 or #5409
Closed

More context in context menu #4529

ian-r-rose opened this issue May 7, 2018 · 3 comments · Fixed by #4590 or #5409
Labels
status:Needs Discussion status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Lumino
Milestone

Comments

@ian-r-rose
Copy link
Member

There are a number of places in the UI where the phosphor context menu system is more limited than we would want, and it is causing trouble. The context menu is based upon CSS selectors, but if there is more than one node with the same (or intersecting) selectors, we currently have no way of distinguishing which one was clicked on.

This, for instance, is a blocker for moving the file browser over to the application context menu, since we need to know more about which file browser item was clicked upon, and how to get that information in the Open With submenu.

It is also a problem with the context menu items in the tabs of the main area (as noted in #4500). If the user right-clicks on a tab that is not the currently active one, it shows the right context menus, but the commands operate on the wrong widget. I am not sure the best way to fix this, but it would be nice to be able to get more information to the commands invoked in the context menu about which node was clicked.

@telamonian
Copy link
Member

telamonian commented May 16, 2018

As a starting point for discussion on this issue, I've submitted a pull request (#4590) with a cleaned-up/more complete version of the focus fix I came up with when I was fiddling around with #4500.

The fix works by having the main app (ie the JupyterLab object) capture the most recent contextmenu event in a ._contextEvent property. The context menu constructor code can then inspect this property in order to get any information it needs about the menu-initiating event that isn't exposed by the current framework.

@telamonian
Copy link
Member

telamonian commented May 16, 2018

I also added a lambda to the addCommands function in docmanager-extension/src/index.ts that exposes the widget specific to the document that was right-clicked on to initiate the contextmenu event:

// fetches the doc widget associated with the most recent contextmenu event
const contextMenuWidget = (): Widget => {
  let path = app.contextMenuTargetPath;
  if (!path) {
    // fall back to active doc widget if path cannot be obtained from event
    return app.shell.currentWidget;
  }

  return docManager.findWidget(path);
};

I then made the relevant subs in the code for the commands themselves (ie I replaced app.shell.currentWidget with contextMenuWidget() where the tab-specific document widget was needed).

In sum, this makes use of the new app._contextEvent property to fix the tab-focus issue brought up in #4500. rename... and the other commands now operate on the tab you actually right-click on, instead of just always operating on the active document (which is the behavior in the current master branch code).

@telamonian
Copy link
Member

telamonian commented May 16, 2018

If you all (mostly @ian-r-rose, I suppose) consider the basic premise of the fix to be sound, I'm sure it can be expanded to cover the information needed by the file browser context menu in the Open With command and elsewhere.

On the other hand, I'm not at all familiar with TS design patterns/best practices. I figure there's a decent chance that my approach breaks desired encapsulation and/or exposes too much information to certain scopes. If so, what would be the appropriate way to expose the needed info?

telamonian added a commit to telamonian/jupyterlab that referenced this issue May 16, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this issue May 16, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this issue May 17, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this issue Jul 27, 2018
@ian-r-rose ian-r-rose reopened this Aug 10, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this issue Sep 5, 2018
… contextmenu

still needs a working version of the "Open With..." item
@blink1073 blink1073 added this to the Future milestone Sep 9, 2018
mgeier pushed a commit to mgeier/jupyterlab that referenced this issue Sep 18, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this issue Sep 29, 2018
… contextmenu

still needs a working version of the "Open With..." item
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 8, 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.
Labels
status:Needs Discussion status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Lumino
Projects
None yet
3 participants