Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Smarter ContextMenu items: pass contextmenu event to ContextMenu items #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

telamonian
Copy link

@telamonian telamonian commented Aug 11, 2018

Adds passDataset property to contextmenu items:

/**
 * Pass a selected node's dataset to an item's command.
 *
 * If this flag is set, when a contextmenu item is matched
 * to the target of a contextmenu event, the key-value pairs
 * in `target.dataset` (if present) will be added as
 * properties of `item.args`.
 *
 * Only meaningful for items of type `command`.
 */
 passDataset?: boolean;

The behavior of the flag is implemented via a few lines added to the matchItems function:

// Pass the target's dataset to the matched item's args, if requested.
if (item.passDataset && 'dataset' in target) {
  item.args = {...item.args, ...(target as any)['dataset']};
}

This PR doesn't change any existing behavior, it just adds some optional new behavior. See #354 for use cases/discussion.

@sccolbert
Copy link
Member

I've been wondering if we should add something like $node: Node to the command args. While not JSON, it may be the pragmatic choice. I worry that if we just pass the dataset, then people will have to start adding uuids to the dataset just to recover which node was clicked.

Another alternative would be to have a static property somewhere which is set to the node for the duration of the command execution. The relevant command could then look at that node as needed.

@blink1073
Copy link
Contributor

blink1073 commented Aug 14, 2018

I like the idea of having this static method on CommandRegistry:

currentCommandTarget(): HTMLElement | null {
   return document.body.querySelector('[data-phosphorCommandTarget]');
}

@sccolbert
Copy link
Member

Another options would be to add a third optional arg to the execute method:

execute(id: string, args: ReadonlyJSONObject = JSONExt.emptyObject, target?: HTMLElement): Promise<any>

And that would element would be set to the currentTarget property of the registry for the duration of the execute call.

@afshin
Copy link
Member

afshin commented Aug 14, 2018

https://developer.mozilla.org/en-US/docs/Web/API/Window/event

If only Firefox supported this, we'd be in the clear. In Chrome and Safari and IE, you can access window.event.target, e.g.:

  app.commands.addCommand(command, {
    label: 'Single-Document Mode',
    isToggled: () => app.shell.mode === 'single-document',
    execute: () => {
      console.log('target is', window.event && window.event.target);
      const args =
        app.shell.mode === 'multiple-document'
          ? { mode: 'single-document' }
          : { mode: 'multiple-document' };
      return app.commands.execute(CommandIDs.setMode, args);
    }
  });

results in:
window.event.target

@sccolbert
Copy link
Member

That would be nice. However that's giving you the actual label div that was clicked. Allowing us to pass in the node gives us the option of passing in the "most relevant" node, which in this case would be the entire menu item node, instead of the child label.

@telamonian
Copy link
Author

Here's another wrinkle: not just execute but also the the other callbacks in ICommandOptions may need access to the target node when they're called.

There's some relevant examples of isEnabled needing access to the target's info in JupyterLab's docmanager-extension. Basically, these isEnabled calls need to be able to determine if the target node is associated with an appropriate path.

@telamonian
Copy link
Author

I like the idea of stashing the target node in a static property, since that will make it easy to access the target within various functions (eg execute, isEnabled, etc). I also like the idea of adding an optional third target?: HTMLElement arg to execute, since that seems like it will make it easier for the average developer to figure out how to write context menu commands that make use of the target node.

I think these two things could be combined into a single nice design via some code in Menu.triggerActiveItem that would pass target to isEnabled and execute (and also some similar code in MenuItem.isEnabled, etc).

@telamonian telamonian changed the title Smarter contextmenu items: pass node's dataset to matching contextmenu item Smarter contextmenu items: pass target node to matching contextmenu item Aug 19, 2018
@telamonian
Copy link
Author

telamonian commented Aug 19, 2018

Okay, I've written/tested a new version of the PR. The passDataset flag has been completely removed. Instead, when a context menu is opened, each item will now store their target HTML nodes for the lifetime of the menu. The target is then passed to execute as a third optional target arg, as @sccolbert suggested. Here's an example implementation of a command that makes use of target:

    commands.addCommand(CommandIDs.showInFileBrowser, {
      label: () => `Show in File Browser`,
      isEnabled,
      execute: (args, target) => {
        // give 1st precedence to explicitly passed path
        let path = args['path'];
        if (!path && target) {
          // give 2nd precedence to path on the target node, if present
          path = target.dataset['path'];
        }
        if (!path) {
          // fall back to path of focused widget
          path = docManager.contextForWidget(shell.currentWidget).path;
        }
        if (!path) {
          return;
        }
  
       ...
        // do stuff with path
       ...
      }
    });

There's some more work to be done to make target available to isEnabled(), etc, but I figured this would be a good place to stop and get some feedback from the maintainers. If someone could review the code in the latest commit (maybe @sccolbert or @afshin?), that would be really helpful.

telamonian added a commit to telamonian/jupyterlab that referenced this pull request Aug 19, 2018
@afshin
Copy link
Member

afshin commented Aug 20, 2018

What if the command registry has an attached property so that the API surface area of the command functions doesn't change. Suppose you could do:

commands.addCommand(CommandIDs.showInFileBrowser, {
  label: () => `Show in File Browser`,
  isEnabled,
  execute: (args) => {
    const target = commands.targetProperty.get(args);
    // etc.
  }
});

@telamonian
Copy link
Author

I can definitely make target into a command registry property. It'll be a little tricky to still support the fact that different items in the same menu can have different targets (depending on their selector), but I think I know how it can be done.

I am a little confused by something in your example code, though @afshin. Could you please clarify what you mean by .get(args) in the line const target = commands.targetProperty.get(args);?

@afshin
Copy link
Member

afshin commented Aug 20, 2018

@telamonian I would say hold off on doing that to see what @sccolbert thinks. But the targetProperty in my code block would be a phosphor AttachedProperty instance in the CommandRegistry namespace. So that individual args being deployed to a command execution would have a target property attached to them that can be retrieved by commands.targetProperty.get(args),

@telamonian
Copy link
Author

Huh. I don't think I've ever come across a pattern quite like that of AttachedProperty. But I get it, and it does seem like a nice fit for this use case.

@telamonian
Copy link
Author

Okay, here's the 3rd stab at this PR. This version is written along the lines of the discussion I had with @sccolbert at the Jupytercon sprint. contextEvent is now a property of CommandRegistry. Some notes:

  • I had to subclass the Menu that ContextMenu wraps in order to get the timing of the cleanup of contextEvent correct. Originally I tried connecting a cleanup function to the Menu._aboutToClose signal, but then cleanup happens too early in Menu.triggerActiveItem.

  • I also added a contextEventTarget(selector: string): Element | null convenience function to CommandRegistry. It's basically just recycled code from matchItems. I put this in since it'll be useful for some of the stuff I want to do, and it seems to be non-trivial to come up with a good implementation. If contextEventTarget doesn't seem like it'll be of general enough use, though, I can always take it out

@telamonian telamonian changed the title Smarter contextmenu items: pass target node to matching contextmenu item Smarter ContextMenu items: pass contextmenu event to ContextMenu items Aug 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants