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.

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

Fixes for Vscode emacs extension #6625

Merged
merged 1 commit into from
Dec 16, 2019
Merged

Fixes for Vscode emacs extension #6625

merged 1 commit into from
Dec 16, 2019

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Nov 25, 2019

Several fixes and adaptions that allow to use VsCode Emacs Keymap extension

What it does

How to test

Apply VsCode Emacs Keymap extension
Tested Commands:

Move commands

  • C-f | Move forward
  • C-b | Move backward
  • C-n | Move to the next line (in collision with browser keybinding)
  • C-p | Move to the previous line
  • C-a | Move to the beginning of line
  • C-e | Move to the end of line
  • M-f | Move forward by one word unit
  • M-b | Move backward by one word unit
  • M-> | Move to the end of buffer
  • M-< | Move to the beginning of buffer
  • C-v | Scroll down by one screen unit
  • M-v | Scroll up by one screen unit
  • M-g g | Jump to line (command palette)
  • M-g n | Jump to next error
  • M-g p | Jump to previous error
  • C-l | Center screen on current line

Search Commands

  • C-s | Search forward
  • C-r | Search backward
  • A-% | Replace
  • C-Enter | Replace One Match (In replace dialog)
  • C-M-n | Add selection to next find match

Edit commands

  • C-d | Delete right (DEL)
  • C-h | Delete left (BACKSPACE)
  • M-d | Delete word
  • M-Bksp | Delete word left
  • C-k | Kill to line end (requires vscode cursorMove built-in command)
  • C-S-Bksp | Kill entire line (requires vscode cursorMove built-in command)
  • C-o | open-line
  • C-w | Kill region (in collision with browser keybinding)
  • M-w | Copy region to kill ring
  • C-y | Yank (requires vscode lineBreakInsert built-in command)
  • C-j | Enter (requires vscode lineBreakInsert built-in command)
  • C-m | Enter (requires vscode lineBreakInsert built-in command)
  • C-x C-o | Delete blank lines around
  • C-x h | Select All
  • C-x u (C-/, C-_) | Undo
  • C-; | Toggle line comment in and out
  • M-; | Toggle region comment in and out
  • C-x C-l | Convert to lower case
  • C-x C-u | Convert to upper case

Other Commands

  • C-g | Cancel
  • C-space | Set mark
  • C-quote | IntelliSense Suggestion
  • M-x | Open command palette
  • C-M-SPC | Toggle SideBar visibility (requires vscode workbench.action.toggleSidebarVisibility built-in command)
  • C-x z |  (requires vscode workbench.action.toggleZenMode built-in command)
  • C-x r (requires vscode workbench.action.openRecent built-in command)

File Commands

  • C-x C-s | Save
  • C-x C-w | Save as (in collision with browser keybinding)
  • C-x C-n | Open new window (in collision with browser keybinding)

Tab / Buffer Manipulation Commands

  • C-x b | Switch to another open buffer
  • C-x C-f | QuickOpen a file
  • C-x k | Close current tab (buffer)
  • C-x C-k | Close all tabs
  • C-x 0 | Close editors in the current group.
  • C-x 1 | Close editors in other (split) group.
  • C-x 2 | Split editor horizontal (requires vscode workbench.action.splitEditorDown built-in command)
  • C-x 3 | Split editor vertical (requires vscode workbench.action.splitEditorRight built-in command)
  • C-x 4 | Toggle split layout (vertical to horizontal) (requires vscode workbench.action.toggleEditorGroupLayout built-in command)
  • C-x o | Focus other split editor (requires vscode workbench.action.navigateEditorGroups built-in command)

Review checklist

Reminder for reviewers

@akosyakov akosyakov added keybindings issues related to keybindings vscode issues related to VSCode compatibility labels Nov 25, 2019
@akosyakov
Copy link
Member

akosyakov commented Nov 25, 2019

@vinokurig test cases don't pass anymore.

Could someone review that changes to the core package does not cause any regressions?

@vinokurig
Copy link
Contributor Author

@akosyakov fixed tests

@vinokurig vinokurig force-pushed the che-9244 branch 2 times, most recently from 09111b2 to 0462391 Compare November 26, 2019 12:10
@vinokurig
Copy link
Contributor Author

@akosyakov @svenefftinge @vince-fugnitto any updates?

@akosyakov
Copy link
Member

Code-wise changes look good to me, but someone have to test it.

@akosyakov
Copy link
Member

@vinokurig don’t forget to clean up the history please

@akosyakov
Copy link
Member

We discussed with @svenefftinge. There is a concern that we still want to see errors for collided default keybindings and try to treat them as before, i.e. without reversing order. An alternative less impactful approach could be to introduce a new scope between DEFAULT and USER, not sure how to name it. For this scope keybindings should be added in reverse order and collisions should be ignored to align with VS Code. @vinokurig Do you see any concerns with such approach?

@vinokurig vinokurig force-pushed the che-9244 branch 2 times, most recently from 3d907ea to 591247f Compare December 5, 2019 10:50
@vinokurig
Copy link
Contributor Author

@akosyakov I've added a PLUGIN scope and applied the previous logic to it according to you comment, could you please check?

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

yes, something like that

packages/core/src/browser/keybinding.ts Show resolved Hide resolved
packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved
packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@vinokurig
Copy link
Contributor Author

@akosyakov could you please check my latest comment: #6625 (comment)

@akosyakov
Copy link
Member

akosyakov commented Dec 15, 2019

@vinokurig very good analysis of keybindings, could you open a follow up issue summing up what still does not work and which VS Code commands we have to implement to support everything? We should also think how we can go about keybindings which are in collision with browser. I wonder does it work in VS Online and if not there is an issue in VS Code for it. Maybe they can introduce a new attribute to keybindings for web target.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It works nicely for me and I don't think it can break in the current state.

@vinokurig vinokurig merged commit 582ae9d into master Dec 16, 2019
@vinokurig vinokurig deleted the che-9244 branch December 16, 2019 08:50
@vinokurig
Copy link
Contributor Author

vinokurig commented Dec 16, 2019

@akosyakov

could you open a follow up issue summing up what still does not work and which VS Code commands we have to implement to support everything?

I've added comments to existed issue: #4050 (comment) and #4050 (comment)

We should also think how we can go about keybindings which are in collision with browser.

Opened an issue: #6755

I wonder does it work in VS Online and if not there is an issue in VS Code for it. Maybe they can introduce a new attribute to keybindings for web target.

It doesn't work in VS Online as well and I've found a related issue in VsCode: microsoft/vscode#85252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants