Skip to content

Change key handler to use CM6 keymap#54

Closed
timdown wants to merge 1 commit intoreplit:masterfrom
timdown:use-keymap
Closed

Change key handler to use CM6 keymap#54
timdown wants to merge 1 commit intoreplit:masterfrom
timdown:use-keymap

Conversation

@timdown
Copy link
Copy Markdown
Contributor

@timdown timdown commented Oct 3, 2022

This PR changes the key handling to use a keymap with a single keybinding with the any method, which is new in @codemirror/view 6.3.0.

This means that rather than the current situation where the Vim key handler runs before any CM6 keymap, the Vim key handler is a regular keymap and participates in the precedence hierarchy with other keymaps. I'm not aware of any current issues this will fix but it makes it possible to override Vim keybindings with a higher-precedence keymap, which was not possible before and is a definite improvement.

I've done some testing and haven't spotted any problems with the default set-up. However, this has the potential to break existing installations with other keymaps, because a keymap that currently always only runs after the Vim key handler may now have higher precedence and run before it instead, so I think there should probably be a note in the README and possibly a minor version number bump.

Background: https://discuss.codemirror.net/t/autocompletion-keymap-precedence-again/4827

Equivalent Emacs keybindings PR: replit/codemirror-emacs#5

@masad-frost masad-frost requested review from nightwing and removed request for nightwing October 4, 2022 20:54
Copy link
Copy Markdown
Member

@masad-frost masad-frost left a comment

Choose a reason for hiding this comment

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

Nice. Thanks a lot for the fix.

It seems that it may have broken about 28 tests, and likely caused real issues. Do you have access to the test results?

You can also run them locally

@timdown
Copy link
Copy Markdown
Contributor Author

timdown commented Oct 5, 2022

Nice. Thanks a lot for the fix.

It seems that it may have broken about 28 tests, and likely caused real issues. Do you have access to the test results?

You can also run them locally

Yeah, I saw the CI fail after I submitted the PR. I got as far as running the tests locally and seeing the same thing and am intending to look into it but haven't got to it yet. I should get a chance today.

@timdown
Copy link
Copy Markdown
Contributor Author

timdown commented Oct 5, 2022

My observation when I tried the tests the other day and again now is that manually replicating the failing tests (the five I've tried, at least) in the dev/index.html page shows no problem, i.e. the tests seem as though they should pass, which suggests some issue with the test set-up or inconsistency between the tests and the dev page. I haven't got to the bottom of that.

@masad-frost
Copy link
Copy Markdown
Member

Ah interesting. If you don't get to the bottom of it I can jump in sometime next week. Or maybe @nightwing can take a look if he's got some time.

@gburtini
Copy link
Copy Markdown

Bumping this one for @nightwing as well.

@nightwing
Copy link
Copy Markdown
Collaborator

The tests are failing because codemirror captures the esc key when it waits for autocompletion results to be gathered, even though the popup itself is not shown. It is usually impossible to replicate manually because default completers are resolved very quickly
I have fixed the tests by disabling the autocomplete plugin in test runner. But i am not sure if we should use this approach or not see my comment at #93 (comment)

@nightwing
Copy link
Copy Markdown
Collaborator

As discussed in the linked pull request, overriding other keymaps seems to be required to correctly emulate vim. If there are other issue reports we may reopen this and try to find a workaround for autocomplete, but for now closing this pull request.

@nightwing nightwing closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants