Enable packages to control order of context menu items by captbaritone · Pull Request #16661 · atom/atom

captbaritone · 2018-02-01T00:38:35Z

Goals:

Allow packages to share context menu “groups” (sets of context menu items delineated by separators). For example, a package should be able to provide a “Copy Full Path” context menu item which lives in the same group as the core “Copy” item.
Allow packages to control the ordering of context menu items.
Allow packages to control the order of context menu groups.

Constraints:

Packages should be able to position their items alongside other package's items without requiring the other package to make any changes.
The current sorting behavior should be maintained for context menus which do not contain any items which use this new option.
Packages may make ordering requests which are in contradictory. In this case, some ordering requests must be ignored in a deterministic way.

Prior work:

There has been at least one prior proposal to (discussion here) support ordering in Atom context menus. It got as far as merging support into Electron. Unfortunately, the Electron implementation has some problems, and the Atom support was never implemented.
The prior proposal seems to be based upon the the API that Eclipse has.

-- Extract from the Eclipse documentation.

Proposed Solution:

Add four new optional properties to the context menu items accepted by atom.contextMenu.add() each accepting as its value either an array of atom commands. The four properties are:
- before
- after
- beforeGroupContaining
- afterGroupContaining
before/after Provide a means for a single context menu item to declare their placement relative to another context menu item. These also imply that menu item in question should be placed in the same “group” as the item
beforeGroupContaining/afterGroupContaining Provide a means for a single context menu to declare the placement of their containing group, relative to the containing group of the specified item. Ideally these could be declared on some entity representing the notion of a “group”, but no such entity exists at this time, and I don't believe it's worth introducing a new construct within this API.

Edge case handling:

If no menu item with the given command exists, the relationship is ignored. This allows package authors to declare positioning relative to packages which the user may or may not have installed.
When a menu item attempts to position itself relative to another item, the two groups are joined. Note that this can happen recursively.
If two sets of menu items are provided by two consecutive plugins, and they don't delineate their items with separators, the items at the boundaries will be merged together into a group. If any item in that group specifies its position, it would cause all items in that new group to be relocated.

Possible challenges:

This approach assumes that context menus do not contain multiple entries that dispatch the same command. I can't think of any rational case in which this would happen, but it could be that there is some case I have not thought of. Another option would be for menu items to declare their position relative to other items via the other item's label. This would have the advantage that Atom is already enforcing label uniqueness, but it does not seem as stable for me. Localization would be one reason to avoid using labels as an item's unique identifier.

maxbrunsfeld

This seems very well designed. Nice work!

Does sorting the menu items ever have a noticeable performance impact when right clicking? It seems like the number of context menu items is usually small, so it's probably not an issue, but it'd be good to look at a profile just in case.

maxbrunsfeld · 2018-02-21T20:30:42Z

spec/menu-sort-helpers-spec.js

+      expect(sortMenuItems(items)).toEqual(items)
+    })
+  })
+  describe('dedupes separators', () => {


Could you add blank lines between these it and describe calls?

captbaritone · 2018-02-21T21:16:25Z

@maxbrunsfeld Thanks for taking the time to review. I'll try to construct some reasonable worst-case scenarios and profile them.

captbaritone · 2018-02-22T00:58:46Z

For a rather complex context menu of >30 elements containing 20 relationships of various types it takes under 10 milliseconds to sort them.

maxbrunsfeld · 2018-02-22T01:15:30Z

👍 In a lot of code paths in Atom, ten milliseconds would be pretty significant, but since this only occurs on a fairly course-grained basis (once per right click) it doesn't seem like a big deal here.

stevenzeck · 2018-04-06T18:02:02Z

Do packages that use a .cson file to build the context menu need to make any changes to use? Or does this not have anything to do with them?

captbaritone · 2018-04-06T18:34:47Z

This should work from either CSON or via atom.contextMenu.add().

lierdakil · 2018-04-19T00:39:10Z

There's no documentation on this feature. Is this intended to be left undocumented, or is this an oversight?

To be clear, I believe this JSDoc should've been updated:

atom/src/context-menu-manager.coffee

Lines 84 to 110 in 42e3a50

    
           # ## Arguments 
        
           # 
        
           # * `itemsBySelector` An {Object} whose keys are CSS selectors and whose 
        
           #   values are {Array}s of item {Object}s containing the following keys: 
        
           #   * `label` (optional) A {String} containing the menu item's label. 
        
           #   * `command` (optional) A {String} containing the command to invoke on the 
        
           #     target of the right click that invoked the context menu. 
        
           #   * `enabled` (optional) A {Boolean} indicating whether the menu item 
        
           #     should be clickable. Disabled menu items typically appear grayed out. 
        
           #     Defaults to `true`. 
        
           #   * `submenu` (optional) An {Array} of additional items. 
        
           #   * `type` (optional) If you want to create a separator, provide an item 
        
           #      with `type: 'separator'` and no other keys. 
        
           #   * `visible` (optional) A {Boolean} indicating whether the menu item 
        
           #     should appear in the menu. Defaults to `true`. 
        
           #   * `created` (optional) A {Function} that is called on the item each time a 
        
           #     context menu is created via a right click. You can assign properties to 
        
           #    `this` to dynamically compute the command, label, etc. This method is 
        
           #    actually called on a clone of the original item template to prevent state 
        
           #    from leaking across context menu deployments. Called with the following 
        
           #    argument: 
        
           #     * `event` The click event that deployed the context menu. 
        
           #   * `shouldDisplay` (optional) A {Function} that is called to determine 
        
           #     whether to display this item on a given context menu deployment. Called 
        
           #     with the following argument: 
        
           #     * `event` The click event that deployed the context menu. 
        
           #

captbaritone · 2018-04-19T17:13:55Z

@lierdakil Yeah, this should probably get documented now that it's been accepted. I'll add it to my list.

lierdakil · 2018-04-19T17:27:12Z

now that it's been accepted

Uh... to my understanding, it's not only "accepted", but also released with v1.26.0... so yeah, the feature is released, but documentation isn't there. I'm aware that's how it usually is, but it doesn't mean that's a good thing. I would argue the docstring should've been updated as a part of this PR.

This is especially not fun considering Atom had a policy "undocumented means unsupported" since the early days. So this feature is in a kind of an awkward spot right now.

To be clear, I'm not trying to start anything, just sharing my point of view. Please don't take it personally.

captbaritone · 2018-04-19T17:35:11Z

@lierdakil Not sure about the official stance, but I'm okay with this being an "unsupported" API until it gets documented. In fact, it might be good to get some people trying it out and proving that it doesn't have any unforeseen problems before it's fully blessed. That said, I don't know exactly how these things generally get rolled out.

The Nuclide team will be doing some work to adopt this in the near future and we will probably get some additional signal there to see how this works at a larger scale than my initial tests.

Summary: Back in February I [added support](atom/atom#16661) in Atom for controling the order of context menu items. The API is not particularly pretty, but it works. Responding to alobbFB's report in T26643515, I've added our first use of this API. You'll notice that in this diff, `tree-view:search-in-directory` does not declare its own position. Instead, other entries position themselves relative to it. This is because in some cases (I'm not sure if this is intentional or not) `atom/find-and-replace` provides a context menu entry by the same name. To handle this, "Show in Finder" and "Set to Current Working Root" define themselves relative to both `project-find:show-in-current-directory` and `tree-view:search-in-directory`. This way, whichever entry wins, the ordering is stable. {F134321737} Reviewed By: matthewwithanm Differential Revision: D9070332 fbshipit-source-id: 76dd805ae739e78b8e8275475f1b9e3356b83817

Allow packages to control the order of context menu items

7a60c99

captbaritone requested a review from matthewwithanm February 1, 2018 00:39

captbaritone changed the title ~~Enable packages to control ordre of context menu items~~ Enable packages to control ordrer of context menu items Feb 1, 2018

captbaritone changed the title ~~Enable packages to control ordrer of context menu items~~ Enable packages to control order of context menu items Feb 1, 2018

captbaritone mentioned this pull request Feb 1, 2018

Allow a location option when adding menu items #6270

Closed

captbaritone added 2 commits February 5, 2018 11:04

Fix comment typo

ee73183

Fix accidental quadratic

a1d9671

maxbrunsfeld approved these changes Feb 21, 2018

View reviewed changes

Improve spec spacing

42e3a50

captbaritone merged commit fa9c8d1 into atom:master Feb 22, 2018

MoritzKn mentioned this pull request Mar 9, 2018

right-click menus are inconsistent atom/tree-view#535

Open

codebytere mentioned this pull request Mar 20, 2018

Add menu item order control electron/electron#12362

Merged

2 tasks

lierdakil mentioned this pull request Apr 19, 2018

[atom] Support version v1.26. DefinitelyTyped/DefinitelyTyped#25111

Merged

9 tasks

stevenzeck mentioned this pull request Aug 13, 2018

please replace from first place in context menu Glavin001/atom-beautify#2163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable packages to control order of context menu items#16661

Enable packages to control order of context menu items#16661
captbaritone merged 4 commits intoatom:masterfrom
captbaritone:context-menu-order-master

captbaritone commented Feb 1, 2018 •

edited

Loading

Uh oh!

maxbrunsfeld left a comment

Uh oh!

maxbrunsfeld Feb 21, 2018

Uh oh!

captbaritone commented Feb 21, 2018

Uh oh!

captbaritone commented Feb 22, 2018

Uh oh!

maxbrunsfeld commented Feb 22, 2018 •

edited

Loading

Uh oh!

stevenzeck commented Apr 6, 2018

Uh oh!

captbaritone commented Apr 6, 2018 •

edited

Loading

Uh oh!

lierdakil commented Apr 19, 2018 •

edited

Loading

Uh oh!

captbaritone commented Apr 19, 2018

Uh oh!

lierdakil commented Apr 19, 2018

Uh oh!

captbaritone commented Apr 19, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

captbaritone commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goals:

Constraints:

Prior work:

Proposed Solution:

Uh oh!

maxbrunsfeld left a comment

Choose a reason for hiding this comment

Uh oh!

maxbrunsfeld Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

captbaritone commented Feb 21, 2018

Uh oh!

captbaritone commented Feb 22, 2018

Uh oh!

maxbrunsfeld commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenzeck commented Apr 6, 2018

Uh oh!

captbaritone commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lierdakil commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captbaritone commented Apr 19, 2018

Uh oh!

lierdakil commented Apr 19, 2018

Uh oh!

captbaritone commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

captbaritone commented Feb 1, 2018 •

edited

Loading

maxbrunsfeld commented Feb 22, 2018 •

edited

Loading

captbaritone commented Apr 6, 2018 •

edited

Loading

lierdakil commented Apr 19, 2018 •

edited

Loading

captbaritone commented Apr 19, 2018 •

edited

Loading