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

Enable packages to control order of context menu items #16661

Merged
merged 4 commits into from Feb 22, 2018

Conversation

Projects
None yet
4 participants
@captbaritone
Contributor

captbaritone commented Feb 1, 2018

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:

screen shot 2018-01-25 at 2 28 25 pm

-- 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.

@captbaritone captbaritone requested a review from matthewwithanm Feb 1, 2018

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

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

captbaritone added some commits Feb 5, 2018

@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.

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

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Feb 21, 2018

Contributor

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

@captbaritone

This comment has been minimized.

Contributor

captbaritone commented Feb 21, 2018

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

@captbaritone

This comment has been minimized.

Contributor

captbaritone commented Feb 22, 2018

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

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Feb 22, 2018

👍 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.

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@codebytere codebytere referenced this pull request Mar 20, 2018

Merged

Add menu item order control #12362

2 of 2 tasks complete
@stevenzeck

This comment has been minimized.

stevenzeck commented Apr 6, 2018

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

This comment has been minimized.

Contributor

captbaritone commented Apr 6, 2018

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

@lierdakil

This comment has been minimized.

Contributor

lierdakil commented Apr 19, 2018

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:

# ## 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.
#

@lierdakil lierdakil referenced this pull request Apr 19, 2018

Merged

[atom] Support version v1.26. #25111

8 of 9 tasks complete
@captbaritone

This comment has been minimized.

Contributor

captbaritone commented Apr 19, 2018

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

@lierdakil

This comment has been minimized.

Contributor

lierdakil commented Apr 19, 2018

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

This comment has been minimized.

Contributor

captbaritone commented Apr 19, 2018

@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.

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request Jul 31, 2018

Start stabilizing the order of File Tree context menu items
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment