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

Add support for "hidden" commands #92

Merged
merged 3 commits into from Nov 2, 2017

Conversation

Projects
None yet
6 participants
@wbinnssmith
Contributor

wbinnssmith commented Oct 24, 2017

This allows for commands to opt-into not appear in command palette.

Frequently commands are an implementation detail, or even only a means of providing a keybinding, and do not benefit from being shown and selectable to the user from the command palette.

Options like "Vim Mode Plus: Set Input Char M" can be confusing to the user and cloud high-signal commands from surfacing in command palette results.

Additionally, showing so many commands can bog down the command palette and make it slow to start -- though there are certainly other means of improving its performance.

The name (hiddenInCommandPalette) is the most concise I could come up with to convey what we want here. There might be other ways of surfacing commands in an interface that might not want to obey this (or those that might), but for now I'm erring on the side of being specific to command palette. Totally open to making this more generic if there's a good argument or I'm missing something.

Released under CC0.

@wbinnssmith

This comment has been minimized.

Show comment
Hide comment
@wbinnssmith

wbinnssmith Oct 24, 2017

Contributor

It's also worth noting that this depends on the metadata changes introduced in atom/atom#15383 -- though I think this shouldn't be an issue with future releases of command-palette?

This looks similar to #90, but rather than having the user configure their command palette, the package author can determine whether their commands should be shown using the new metadata API. Both should be able to co-exist.

cc @jordwalke @t9md

Contributor

wbinnssmith commented Oct 24, 2017

It's also worth noting that this depends on the metadata changes introduced in atom/atom#15383 -- though I think this shouldn't be an issue with future releases of command-palette?

This looks similar to #90, but rather than having the user configure their command palette, the package author can determine whether their commands should be shown using the new metadata API. Both should be able to co-exist.

cc @jordwalke @t9md

Add support for "hidden" commands
This allows for commands to opt-into not appear in command palette.

Frequently commands are an implementation detail, or even only a means of
providing a keybinding, and do not benefit from being shown and selectable to
the user from the command palette.

Options like "Vim Mode Plus: Set Input Char M" can be confusing to the
user and cloud high-signal commands from surfacing in command palette
results.

Additionally, showing so many commands can [bog down the command palette and make it
slow to start](#80) --
though there are certainly other means of improving its performance.

The name is the most concise I could come up with to convey what we want
here. There might be other ways of surfacing commands in an interface
that might not want to obey this (or those that might), but for now I'm
erring on the side of being specific to command palette. Totally open to
making this more generic if there's a good argument or I'm missing
something.
@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Oct 24, 2017

Contributor

I like this idea, but I want escape hatch.

I'm maintaining over 30 packages including vim-mode-plus.
Sometime I ask my pkg user to execute command via command-palette to determine issue is keymap related or not.
If execute via command-palette works but not via keymap, it simply keymap issue.
This situation frequently happen in different keyboard layout and this trouble shoot option was great help.

I want hide most of vim-mode-plus command basically, but I want to keep the way to "force show all commands by ignoring hide flag" option.

Contributor

t9md commented Oct 24, 2017

I like this idea, but I want escape hatch.

I'm maintaining over 30 packages including vim-mode-plus.
Sometime I ask my pkg user to execute command via command-palette to determine issue is keymap related or not.
If execute via command-palette works but not via keymap, it simply keymap issue.
This situation frequently happen in different keyboard layout and this trouble shoot option was great help.

I want hide most of vim-mode-plus command basically, but I want to keep the way to "force show all commands by ignoring hide flag" option.

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Oct 24, 2017

Perhaps it's a matter of naming hiddenInCommandPalette to something more like hiddenByDefaultInCommandPalette - then it's up to various command pallets to provide a way to "show all" (hopefully a keyboard shortcut or something).

jordwalke commented Oct 24, 2017

Perhaps it's a matter of naming hiddenInCommandPalette to something more like hiddenByDefaultInCommandPalette - then it's up to various command pallets to provide a way to "show all" (hopefully a keyboard shortcut or something).

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

I'm not completely opposed to adding an ability to hide commands, but I'm very concerned that it could create confusion and opacity about which commands exist. Do we have any more concrete examples of commands that most people would consider to be noise?

For the example of Vim Mode Plus: Set Input Character M, I'm wondering if this is more related to a problem of the command system being insufficiently expressive.

What if we enhanced the commands system in the following way:

  • We enhance the key bindings system to always include the keystroke as a string-valued field on the command event in addition to the originalEvent field, which only provides the most recent raw keyboard event.
  • When listeners are registered for a command, allow the registering user to specify a requestDetail: true field on the command specification. When the command palette detected this field, it would prompt the user with a text area if they selected that command.

If we did this, then Set Input Character could be a single command that you can bind multiple keys to in specific modes of vim-mode-plus. For the common keyboard use case, the package could just read the keystroke string off the command event. For the command palette use case, the user would be prompted to enter a string. Now there's one Set Input Character command instead of a zillion of them.

I'm not sure if this solution would apply to other examples of noisy commands. I'd love to hear some more you've experienced @wbinnssmith to confirm or deny this proposed solution.

I can anticipate some potential enhancement ideas to this proposal, like specified some sort of typed schema for the detail arguments. But I think as a first pass it could just always be a string and left to be parsed and interpreted by the package.

What do people think? @wbinnssmith? @t9md?

Contributor

nathansobo commented Oct 26, 2017

I'm not completely opposed to adding an ability to hide commands, but I'm very concerned that it could create confusion and opacity about which commands exist. Do we have any more concrete examples of commands that most people would consider to be noise?

For the example of Vim Mode Plus: Set Input Character M, I'm wondering if this is more related to a problem of the command system being insufficiently expressive.

What if we enhanced the commands system in the following way:

  • We enhance the key bindings system to always include the keystroke as a string-valued field on the command event in addition to the originalEvent field, which only provides the most recent raw keyboard event.
  • When listeners are registered for a command, allow the registering user to specify a requestDetail: true field on the command specification. When the command palette detected this field, it would prompt the user with a text area if they selected that command.

If we did this, then Set Input Character could be a single command that you can bind multiple keys to in specific modes of vim-mode-plus. For the common keyboard use case, the package could just read the keystroke string off the command event. For the command palette use case, the user would be prompted to enter a string. Now there's one Set Input Character command instead of a zillion of them.

I'm not sure if this solution would apply to other examples of noisy commands. I'd love to hear some more you've experienced @wbinnssmith to confirm or deny this proposed solution.

I can anticipate some potential enhancement ideas to this proposal, like specified some sort of typed schema for the detail arguments. But I think as a first pass it could just always be a string and left to be parsed and interpreted by the package.

What do people think? @wbinnssmith? @t9md?

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Oct 26, 2017

Contributor

For the example of Vim Mode Plus: Set Input Character M, I'm wondering if this is more related to a problem of the command system being insufficiently expressive.

I think this is off-topic of original issue.
This command is just picked as one of many examples of noisy command.

In vim-mode-plus, for example each keystroke like j(move-down),k(move-up) have corresponding command.
But none of vmp user try to execute it from command-palette its always invoked via keymap.
So most of vim-mode-plus's command is just noise, no need to appear on command-palette.
vmp register over 300 commands at pkg activation.
This 300 command slow downs command-palette's response when you typed shift-cmd-p.
#81 is PR to solve this slow down issue by limiting maxResult of command-palette(which have it's own caveat that confuse user who doesn't notice this truncation).

Summary: most of vmp's command is noise, I think it's good if pkg author can control visibility of command on cmd-palette, it also reduce delay of cmd-palette.
But I want keep escape hatch for troubleshoot purpose.
E.g

  1. when user execute Command Palette: Toggle Show hidden commands(which might change color of command palette to indicate command-palette is in-special mode need to be reverted after).
  2. Then command-palette shows all commands including hidden one, which is useful to investigate keymap issue.(I want to ask user to check it work if executing from palette when they just reported that "X keystroke not work!").
Contributor

t9md commented Oct 26, 2017

For the example of Vim Mode Plus: Set Input Character M, I'm wondering if this is more related to a problem of the command system being insufficiently expressive.

I think this is off-topic of original issue.
This command is just picked as one of many examples of noisy command.

In vim-mode-plus, for example each keystroke like j(move-down),k(move-up) have corresponding command.
But none of vmp user try to execute it from command-palette its always invoked via keymap.
So most of vim-mode-plus's command is just noise, no need to appear on command-palette.
vmp register over 300 commands at pkg activation.
This 300 command slow downs command-palette's response when you typed shift-cmd-p.
#81 is PR to solve this slow down issue by limiting maxResult of command-palette(which have it's own caveat that confuse user who doesn't notice this truncation).

Summary: most of vmp's command is noise, I think it's good if pkg author can control visibility of command on cmd-palette, it also reduce delay of cmd-palette.
But I want keep escape hatch for troubleshoot purpose.
E.g

  1. when user execute Command Palette: Toggle Show hidden commands(which might change color of command palette to indicate command-palette is in-special mode need to be reverted after).
  2. Then command-palette shows all commands including hidden one, which is useful to investigate keymap issue.(I want to ask user to check it work if executing from palette when they just reported that "X keystroke not work!").
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

@t9md That seems reasonable. Basically my proposal doesn't really fix the noise problem in a general way. I'm sold that we want to add hidden commands.

Is somebody willing to add @t9md's escape hatch of Command Palette: Show Hidden Commands (that just lets you fuzzy-find through hidden commands) to the default command palette? I can get it done eventually but am dealing with higher priority stuff so if someone could just do it I am down to merge a hidden by default, showable in rare situations approach.

Contributor

nathansobo commented Oct 26, 2017

@t9md That seems reasonable. Basically my proposal doesn't really fix the noise problem in a general way. I'm sold that we want to add hidden commands.

Is somebody willing to add @t9md's escape hatch of Command Palette: Show Hidden Commands (that just lets you fuzzy-find through hidden commands) to the default command palette? I can get it done eventually but am dealing with higher priority stuff so if someone could just do it I am down to merge a hidden by default, showable in rare situations approach.

nathansobo added some commits Nov 2, 2017

Add `command-palette:show-hidden-commands` escape hatch
This ensures users can still access hidden commands for debugging
purposes.
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 2, 2017

Contributor

@t9md @wbinnssmith I've added the suggested Command Palette: Show Hidden Commands escape hatch. Once we get a green build I will merge.

Contributor

nathansobo commented Nov 2, 2017

@t9md @wbinnssmith I've added the suggested Command Palette: Show Hidden Commands escape hatch. Once we get a green build I will merge.

@nathansobo nathansobo merged commit 79272db into master Nov 2, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the fb-wbinnssmith-hidden-commands branch Nov 2, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 2, 2017

Contributor

This is published as 0.42.0 (the answer to life, the universe, everything) and upgraded in atom/atom@3d3042b. Thanks again for making this change. Sounds like it will make life much easier for vim-mode-plus users once @t9md leverages it. 📈

Contributor

nathansobo commented Nov 2, 2017

This is published as 0.42.0 (the answer to life, the universe, everything) and upgraded in atom/atom@3d3042b. Thanks again for making this change. Sounds like it will make life much easier for vim-mode-plus users once @t9md leverages it. 📈

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Nov 2, 2017

Contributor

Thank you!! I'll try.
Hope command palette's lag reported by vim-mode-plus user will be solved after that.

Contributor

t9md commented Nov 2, 2017

Thank you!! I'll try.
Hope command palette's lag reported by vim-mode-plus user will be solved after that.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Nov 6, 2017

Contributor

After I hide all vmp commands.
I quickly measure item population time as I did in #81.

collect + render equals to total item population time.

untitled_2

My summary is

  • hiddenInCommandPalette is good to reduce noise.
  • But not perfect solution to reduce command-palette's lag. I still feel big delay on opening palette by cmd-shjift-p.
    • I don't think asking all pkg auther to hide their command is ideal or concrete solution.

Then how to solve palette's delay(my thought)?

  • Still setting maxResults is effective to greatly reduce delay by reducing items to render further.

    • But this approach really confuse user.
  • Better approach is to postpone full item population until some scroll event(??) is fired.

    • First populate top 20th items.( we can get stable response time regardless total num of registered commands ).
    • When user try to access more items via mouser or core:move-down, it render full items.
    • So as long as main usecase is open > filter by querly(keyboard) > concifm, user no longer find palette's delay.
Contributor

t9md commented Nov 6, 2017

After I hide all vmp commands.
I quickly measure item population time as I did in #81.

collect + render equals to total item population time.

untitled_2

My summary is

  • hiddenInCommandPalette is good to reduce noise.
  • But not perfect solution to reduce command-palette's lag. I still feel big delay on opening palette by cmd-shjift-p.
    • I don't think asking all pkg auther to hide their command is ideal or concrete solution.

Then how to solve palette's delay(my thought)?

  • Still setting maxResults is effective to greatly reduce delay by reducing items to render further.

    • But this approach really confuse user.
  • Better approach is to postpone full item population until some scroll event(??) is fired.

    • First populate top 20th items.( we can get stable response time regardless total num of registered commands ).
    • When user try to access more items via mouser or core:move-down, it render full items.
    • So as long as main usecase is open > filter by querly(keyboard) > concifm, user no longer find palette's delay.
@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Nov 8, 2017

Another approach could be to allow the user to hide certain commands. Something akin to the way the user can disable the keymaps of a package could be useful.

I know there are only a handful of commands that I actually use so hiding the rest by default could save a lot of time in the render process.

UziTech commented Nov 8, 2017

Another approach could be to allow the user to hide certain commands. Something akin to the way the user can disable the keymaps of a package could be useful.

I know there are only a handful of commands that I actually use so hiding the rest by default could save a lot of time in the render process.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 8, 2017

Contributor

We need to just optimize the menu rendering. If anyone cc's me on a PR that does it I will prioritize working with that person and make a best effort to be timely. Otherwise it's something we'll need to put a full time engineer on at some point.

Contributor

nathansobo commented Nov 8, 2017

We need to just optimize the menu rendering. If anyone cc's me on a PR that does it I will prioritize working with that person and make a best effort to be timely. Otherwise it's something we'll need to put a full time engineer on at some point.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 8, 2017

Member

@nathansobo There is a PR open to only render elements when needed on atom-select-list. atom/atom-select-list#21

I believe this is what you are asking for?

Member

Ben3eeE commented Nov 8, 2017

@nathansobo There is a PR open to only render elements when needed on atom-select-list. atom/atom-select-list#21

I believe this is what you are asking for?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 8, 2017

Contributor

Awesome, thanks for keeping track of so much @Ben3eeE. Love it. I responded there and hopefully can work with @jarle to push it forward.

Contributor

nathansobo commented Nov 8, 2017

Awesome, thanks for keeping track of so much @Ben3eeE. Love it. I responded there and hopefully can work with @jarle to push it forward.

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request Nov 9, 2017

Respect `hiddenInCommandPalette` in command descriptors
Summary: Commands can now opt out of being shown in the palette. See atom/command-palette#92

Reviewed By: matthewwithanm

Differential Revision: D6270212

fbshipit-source-id: d9439f3cf3db698fb8a4d8e01b6750b6a1a6f7de

hansonw added a commit to facebook-atom/atom-ide-ui that referenced this pull request Nov 18, 2017

Respect `hiddenInCommandPalette` in command descriptors
Summary: Commands can now opt out of being shown in the palette. See atom/command-palette#92

Reviewed By: matthewwithanm

Differential Revision: D6270212

fbshipit-source-id: d9439f3cf3db698fb8a4d8e01b6750b6a1a6f7de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment