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

Implement cmap #225

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Implement cmap #225

merged 1 commit into from
Nov 25, 2020

Conversation

sumoooru2
Copy link
Contributor

@sumoooru2 sumoooru2 commented Feb 20, 2020

I implemented c[nore]map partially. It would be ideal if I could fully, but it seems to need a lot of work.
It's partial because

  • cmap can't be set in the command line unless it's written in ideavimrc or listed in ExKeyBindings.kt
  • cmap only supports one-to-many keycaps, e.g. cmap <C-a><C-b> <Home> is impossible

@AlexPl292 AlexPl292 self-assigned this Feb 27, 2020
@AlexPl292
Copy link
Member

Hi, thank you for the contribution! That's would nice if you could add some tests to prove the changes. Also, I didn't completely understand why it's possible to add only one-to-many mappings?

@sumoooru2
Copy link
Contributor Author

The reason for only one-to-many mappings is that the command line mode uses different way to implement key mappings from other modes. Because of the difference it's difficult to implement many-to-many mappings, but I'm trying to implement it.

@sumoooru2 sumoooru2 changed the title Implement cmap partially Implement cmap Oct 24, 2020
@sumoooru2
Copy link
Contributor Author

Hi, I implemented cmap and removed "partially" from the title. I added unit tests.

Copy link
Member

@AlexPl292 AlexPl292 left a comment

Choose a reason for hiding this comment

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

Hi @sumoooru2!
Thank you for your updates. Now I really want to merge the changes into the repository. Hope we can make it this time.
Few questions regarding the pr:

  1. Do I understand correctly that this PR fixes also some issue with literals and digraphs in command mode?
  2. Do I understand correctly that the main idea is to process keys from command line using KeyHandler and not processExKey?

Comment on lines +30 to +32
var prevExpectedArgumentType: Argument.Type? = null
private set

Copy link
Member

Choose a reason for hiding this comment

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

Is this value presented to properly support some commands with digraphs? E.g. dt<C-k>o:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is. Without it , e.g. testDeleteToDigraph will fail.

@@ -753,8 +763,10 @@ private void startWaitingForArgument(Editor editor,
// the key handler when it's complete.
if (action instanceof InsertCompletedDigraphAction) {
editorState.startDigraphSequence();
setPromptCharacterEx('?');
Copy link
Member

Choose a reason for hiding this comment

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

Is this code supposed to insert ? character under the caret when the user inserts digraph? IIRC, this functionality already works, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but using KeyHandler changed the logic and I needed to implement it here

@sumoooru2
Copy link
Contributor Author

  1. Do I understand correctly that this PR fixes also some issue with literals and digraphs in command mode?

No, I re-introduced some features about literals and digraphs (e.g. show '?' or '^'). They used to be done by Actions in ExEditorKit, which is no longer used.

  1. Do I understand correctly that the main idea is to process keys from command line using KeyHandler and not processExKey?

Yes, but processExKey is still used by KeyHandler.

@sumoooru2
Copy link
Contributor Author

sumoooru2 commented Nov 23, 2020

This page shows "All checks have failed". I run the test "DeleteVisualLinesEndActionTest" locally several times, but it succeeded 🤔

@AlexPl292
Copy link
Member

Awesome, thank you for your work! Looks like ex panel needs some polishing, but this is definitely out of this scope.

@AlexPl292 AlexPl292 merged commit 5c9faba into JetBrains:master Nov 25, 2020
@sumoooru2
Copy link
Contributor Author

Thank you for reviewing and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants