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.

Sign up for GitHub

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

Jump to bottom

Add ability to read keybindings from 'package.json' file of a plugin #3731

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Add ability to read keybindings from 'package.json' file of a plugin #3731

merged 1 commit into from
Dec 12, 2018

Conversation

RomanNikitenko
Copy link
Contributor

@RomanNikitenko RomanNikitenko commented Dec 5, 2018 •
edited
Loading

Add ability to register keybindings from 'package.json' file of a plugin

Example:

"keybindings": [
      {
        "key": "ctrl+b",
        "command": "emacs.cursor.left",
        "when": "editorTextFocus"
      },
      {
        "key": "ctrl+f",
        "command": "emacs.cursor.right",
        "when": "editorTextFocus"
      }
    ]

Video: https://youtu.be/Tf6TvlWEZ1Y

Signed-off-by: Roman Nikitenko [email protected]

@azatsarynnyy
Copy link
Member

Note, currently, only Theia's keybinding contexts are supported to pass through the when property.
Support for the VSCode's 'when' clause contexts can be added once the #3658 is finished.

benoitf
benoitf reviewed Dec 5, 2018
View reviewed changes
packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts Outdated
private handlePartialKeybindings(keybinding: Keybinding, partialKeybindings: Keybinding[]) {
partialKeybindings.forEach(partial => {
if (keybinding.context === undefined || keybinding.context === partial.context) {
this.logger.warn('Partial keybinding is ignored;',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usage of Template literals ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benoitf thank you, fixed

benoitf
benoitf reviewed Dec 5, 2018
View reviewed changes
packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts Outdated
if (shadow.context === undefined || shadow.context === keybinding.context) {
this.keybindingRegistry.unregisterKeybinding(shadow);

this.logger.warn('Shadowing keybinding is ignored;',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems the warning message is the same than on line 51 so maybe we can have only one method ?

Copy link
Contributor Author

@RomanNikitenko RomanNikitenko Dec 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benoitf
thank you for your comment!

There are two cases:

  • the Registry contains ctrl+g v keybinding and we try to register ctrl+g, for example. It is “Partial” case.
    If toRegister keybinding does not have context or its context equals registered keybinding - toRegister shadows registered
  • the Registry contains ctrl+g keybinding and we try to register ctrl+g v, for example. It is “Shadow” case.
    If registered keybinding does not have context or its context equals toRegister keybinding - registered shadows toRegister.

So we have different conditions, different messages (who is shadow) and may be it’s more readable at the moment.
@benoitf WDYT?

@vince-fugnitto
Copy link
Member

@RomanNikitenko I'm just curious, but did you notice if these keybindings are visible from the Keyboard Shortcuts widget? Else, we may need to update the widget to handle different sources of data.

@RomanNikitenko RomanNikitenko added the plug-in system issues related to the plug-in system label Dec 5, 2018
@RomanNikitenko
Copy link
Contributor Author

@vince-fugnitto I added video for this PR - could you take a look?

@vince-fugnitto
Copy link
Member

@vince-fugnitto I added video for this PR - could you take a look?

sorry I hadn't noticed the link :) it's a very nice feature!

@RomanNikitenko RomanNikitenko merged commit fed4e73 into master Dec 12, 2018
@RomanNikitenko RomanNikitenko deleted the keybindings branch December 12, 2018 07:50
@RomanNikitenko RomanNikitenko mentioned this pull request Dec 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf approved these changes

@azatsarynnyy azatsarynnyy azatsarynnyy approved these changes

@evidolob evidolob Awaiting requested review from evidolob

Assignees
No one assigned
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RomanNikitenko @azatsarynnyy @vince-fugnitto @benoitf