codemirror/codemirror5
1
0
Fork 0
Code Issues 412 Pull requests Activity

[vim keymap] Visual mode rewrite and other changes #8420

Closed
marijnh wants to merge 6 commits from github/fork/metatheos/master into master
pull from: github/fork/metatheos/master
merge into: codemirror:master
Conversation 16 Commits 6 Files changed 3 +73 -21
marijnh commented 2013-02-18 21:25:48 +01:00 (Migrated from gitlab.com)
Copy link

My second batch of changes, containing the remaining additions from my local setup. Should also fix some problems discussed in Issue #1246.

Changes

  1. New moveByLinesToFirstNonWhiteSpaceCharacter, used to implement the motions + (move {repeat} lines down, then to the first non-whitespace-character on that line), - (same for up) and _ (same as + but with repeat reduced by 1, so with a repeat of 1 it behaves like ^).
  2. Some commands require the user to input a character to operate on. Previously pressing any handled special key (like Enter, Space or Down) would pass it on as string (see Issue #1246). These keys are now converted to a printable character if possible and rejected otherwise.
  3. If there is text selected, r{character} replaces all individual characters in the selection by {character}, while leaving linebreaks intact. Special case as by vim help is \n as {character}, which creates only one linebreak for an r operation.
  4. Visual mode is significantly changed, using some of the properties of CodeMirror v3. Notes below.

DIY-Example: Inputting ggOTitle<Esc>yypVr= (with <Esc> being the escape-key) should now behave just as it does in vim.

Notes on moveByLinesToFirstNonWhiteSpaceCharacter

I'm on the fence about whether to keep moveByLinesToFirstNonWhiteSpaceCharacter or merge it with moveToFirstNonWhiteSpaceCharacter. Merging means there would be one less motion, but instead one with an if at the start picking between the two code paths. I also find the name to be obscenely long. @mightyguava what would you prefer here?

Notes on visual mode (selection)

Vim's cursor is not between characters but on one. This also has implications on the selection model. For the beginning of the selection the left side of the cursor is used, while for the end it is the right side. This requires sel.to to be incremented by 1 from the position CodeMirror's setSelection would place it at. Even more of a special case is linewise selection. Here sel.from is moved to the first character of the line, while sel.to is moved behind the last one.

CodeMirror as it is doesn't seem to require sel.from and sel.to to be either sel.anchor or sel.head. I'm not sure if I should expect this special case to be stable and especially whether I really want you to maintain this rather exotic use-case. I am replicating setSelection's behavior for now in an own function (vim.js:1511) and adapt the values at the end. From my testing it seemed to work out quite well. Obviously it cannot deal with folding yet, since I cannot access skipAtomic from my code (a hacky workaround would be calling setCursor(curEnd) or something similar before the custom operation). The only other problem I ran into is how CodeMirror restores selection on undo, which is a separate issue I'll look into as well. Also of course replicating internal code is really bad, I'll try to think of a way that doesn't need much change to the API but still allows me to get the core-code out of my setSelection.

I made one change to codemirror.css. The reason is that setting showCursorWhenSelecting doesn't help if the cursor is covered by the selection (selection has z-index set to 1).

My second batch of changes, containing the remaining additions from my local setup. Should also fix some problems discussed in Issue #1246. ## Changes 1. New `moveByLinesToFirstNonWhiteSpaceCharacter`, used to implement the motions `+` (move `{repeat}` lines down, then to the first non-whitespace-character on that line), `-` (same for up) and `_` (same as `+` but with repeat reduced by 1, so with a repeat of 1 it behaves like `^`). 2. Some commands require the user to input a character to operate on. Previously pressing any handled special key (like `Enter`, `Space` or `Down`) would pass it on as string (see Issue #1246). These keys are now converted to a printable character if possible and rejected otherwise. 3. If there is text selected, `r{character}` replaces all individual characters in the selection by `{character}`, while leaving linebreaks intact. Special case as by vim help is `\n` as `{character}`, which creates only one linebreak for an `r` operation. 4. Visual mode is significantly changed, using some of the properties of CodeMirror v3. Notes below. DIY-Example: Inputting `ggOTitle<Esc>yypVr=` (with `<Esc>` being the escape-key) should now behave just as it does in vim. ## Notes on moveByLinesToFirstNonWhiteSpaceCharacter I'm on the fence about whether to keep `moveByLinesToFirstNonWhiteSpaceCharacter` or merge it with `moveToFirstNonWhiteSpaceCharacter`. Merging means there would be one less motion, but instead one with an `if` at the start picking between the two code paths. I also find the name to be obscenely long. `@mightyguava` what would you prefer here? ## Notes on visual mode (selection) Vim's cursor is not between characters but on one. This also has implications on the selection model. For the beginning of the selection the left side of the cursor is used, while for the end it is the right side. This requires `sel.to` to be incremented by 1 from the position CodeMirror's `setSelection` would place it at. Even more of a special case is linewise selection. Here `sel.from` is moved to the first character of the line, while `sel.to` is moved behind the last one. CodeMirror as it is doesn't seem to require `sel.from` and `sel.to` to be either `sel.anchor` or `sel.head`. I'm not sure if I should expect this special case to be stable and especially whether I really want you to maintain this rather exotic use-case. I am replicating `setSelection`'s behavior for now in an own function ([vim.js:1511](https://github.com/metatheos/CodeMirror/blob/7bfb17ec7dd1977388cced4ddd10affc54e81f99/keymap/vim.js#L1511)) and adapt the values at the end. From my testing it seemed to work out quite well. Obviously it cannot deal with folding yet, since I cannot access `skipAtomic` from my code (a hacky workaround would be calling `setCursor(curEnd)` or something similar before the custom operation). The only other problem I ran into is how CodeMirror restores selection on undo, which is a separate issue I'll look into as well. Also of course replicating internal code is really bad, I'll try to think of a way that doesn't need much change to the API but still allows me to get the core-code out of my `setSelection`. I made one change to `codemirror.css`. The reason is that setting `showCursorWhenSelecting` doesn't help if the cursor is covered by the selection (selection has `z-index` set to 1).
marijnh commented 2013-02-19 10:42:23 +01:00 (Migrated from gitlab.com)
Copy link

Great!
@mightyguava I'd like you to glance over this again (if you don't have time for these reviews, let me know, I'll stop notifying you.)

Great! ` @mightyguava` I'd like you to glance over this again (if you don't have time for these reviews, let me know, I'll stop notifying you.)
marijnh commented 2013-02-19 16:15:53 +01:00 (Migrated from gitlab.com)
Copy link

Sorry for the delay. I was away for the long weekend. The new visual mode looks vastly superior to the previous version. Thanks!

If you want to merge moveByLinesToFirstNonWhiteSpaceCharacter, I suggest merging with moveByLines instead, and add an additional argument for which character the cursor lands on the line. It would also make a bit more structural sense and probably less duplication.

Regarding the custom selection function. I strongly agree that that changing CodeMirror's internal state here is bad. A rule I followed implementing vim.js was to not change CodeMirror's internal state in any way. This made some parts of the logic a bit convoluted, but let vim.js be much more future proof. As you said, a better solution would be to refactor CodeMirror's selection API a bit to make it work for the vim use case. Maybe changing setSelection to accept a sel object with anchor, head, from, and to? Defer to @marijnh here.

Sorry for the delay. I was away for the long weekend. The new visual mode looks vastly superior to the previous version. Thanks! If you want to merge `moveByLinesToFirstNonWhiteSpaceCharacter`, I suggest merging with `moveByLines` instead, and add an additional argument for which character the cursor lands on the line. It would also make a bit more structural sense and probably less duplication. Regarding the custom selection function. I strongly agree that that changing CodeMirror's internal state here is bad. A rule I followed implementing `vim.js` was to not change CodeMirror's internal state in any way. This made some parts of the logic a bit convoluted, but let `vim.js` be much more future proof. As you said, a better solution would be to refactor CodeMirror's selection API a bit to make it work for the `vim` use case. Maybe changing `setSelection` to accept a `sel` object with `anchor`, `head`, `from`, and `to`? Defer to `@marijnh` here.
marijnh commented 2013-02-19 17:34:42 +01:00 (Migrated from gitlab.com)
Copy link

If you want to merge moveByLinesToFirstNonWhiteSpaceCharacter, I suggest merging with moveByLines instead

Now I feel stupid. I'll do just that. Any idea how I could improve the handling of 0-based repeat on _, while I'm at it? I don't like how I introduce motionArgs.repeatOffset just for that.

Also: would you mind if I rename textObjectManipulation to textObjectRange and invert its inclusive-parameter, renaming it to inner? I found these names rather confusing as they are.

As for the detection for mouse interaction, I've restored that for now, but I hope I can take a stab at solving #1084 the way @marijnh suggests soon. This might result in some tools to handle various mouse-related issues and also help with solving #1038.

> If you want to merge `moveByLinesToFirstNonWhiteSpaceCharacter`, I suggest merging with `moveByLines` instead Now I feel stupid. I'll do just that. Any idea how I could improve the handling of 0-based repeat on `_`, while I'm at it? I don't like how I introduce `motionArgs.repeatOffset` just for that. Also: would you mind if I rename `textObjectManipulation` to `textObjectRange` and invert its `inclusive`-parameter, renaming it to `inner`? I found these names rather confusing as they are. As for the detection for mouse interaction, I've restored that for now, but I hope I can take a stab at solving #1084 the way `@marijnh` suggests soon. This might result in some tools to handle various mouse-related issues and also help with solving #1038.
marijnh commented 2013-02-19 20:00:52 +01:00 (Migrated from gitlab.com)
Copy link

Now I feel stupid. I'll do just that. Any idea how I could improve the handling of 0-based repeat on _, while I'm at it? I don't like how I introduce motionArgs.repeatOffset just for that.

Sorry, don't have a better idea. I find _ weird by itself, and don't see how you could not special case it.

Also: would you mind if I rename textObjectManipulation to textObjectRange and invert its inclusive-parameter, renaming it to inner? I found these names rather confusing as they are.

Sure. The text object manipulation motion is a bad hack I threw in to carry over the functionality from the original vim.js implementation. Any changes to clean it up is fine by me.

As for the detection for mouse interaction, I've restored that for now, but I hope I can take a stab at solving #1084 the way @marijnh suggests soon. This might result in some tools to handle various mouse-related issues and also help with solving #1038.

Sounds great. I didn't do it earlier because the fix would be v3 specific and somewhat involved. At this point, keeping vim.js v2 compatible is moot.

> Now I feel stupid. I'll do just that. Any idea how I could improve the handling of 0-based repeat on _, while I'm at it? I don't like how I introduce motionArgs.repeatOffset just for that. Sorry, don't have a better idea. I find `_` weird by itself, and don't see how you could not special case it. > Also: would you mind if I rename textObjectManipulation to textObjectRange and invert its inclusive-parameter, renaming it to inner? I found these names rather confusing as they are. Sure. The text object manipulation motion is a bad hack I threw in to carry over the functionality from the original `vim.js` implementation. Any changes to clean it up is fine by me. > As for the detection for mouse interaction, I've restored that for now, but I hope I can take a stab at solving #1084 the way `@marijnh` suggests soon. This might result in some tools to handle various mouse-related issues and also help with solving #1038. Sounds great. I didn't do it earlier because the fix would be v3 specific and somewhat involved. At this point, keeping `vim.js` v2 compatible is moot.
marijnh commented 2013-02-20 11:55:54 +01:00 (Migrated from gitlab.com)
Copy link

Any changes to clean it up is fine by me.

I just looked through it while trying to improve cw, which led me to looking into aw and iw. At first "Manipulation" confused me a bit, I think "Limits" gives a better idea what it is there for. Also inclusive has a special meaning in vim that is -- as far as I can tell -- unrelated to the use there, so inner might be more descriptive. I had a patch to improve aw and iw but just noticed that it still needs some tweaks to behavior around the end of lines. The actual fix for cw seems to be even more tricky.

I merged the two motions, and added two small fixes in another patch. One was an oversight by me (gj and gk are not considered linewise in vim) and the other one was t walking back if there was no such character on the line (because it could not tell whether it got an actual coordinate or an error-value).

Since you mention v2, how relevant is support for the version? I'm not too familiar with how CodeMirror is used by others so I hope I'm not ignorantly trampling on anything here.

> Any changes to clean it up is fine by me. I just looked through it while trying to improve `cw`, which led me to looking into `aw` and `iw`. At first "Manipulation" confused me a bit, I think "Limits" gives a better idea what it is there for. Also `inclusive` has a special meaning in vim that is -- as far as I can tell -- unrelated to the use there, so `inner` might be more descriptive. I had a patch to improve `aw` and `iw` but just noticed that it still needs some tweaks to behavior around the end of lines. The actual fix for `cw` seems to be even more tricky. I merged the two motions, and added two small fixes in another patch. One was an oversight by me (`gj` and `gk` are not considered linewise in vim) and the other one was `t` walking back if there was no such character on the line (because it could not tell whether it got an actual coordinate or an error-value). Since you mention v2, how relevant is support for the version? I'm not too familiar with how CodeMirror is used by others so I hope I'm not ignorantly trampling on anything here.
marijnh commented 2013-02-20 13:14:11 +01:00 (Migrated from gitlab.com)
Copy link

I don't have a clear grasp of what the selection problem you're having is like, exactly. Just an idea -- would a beforeSelectionChange event, similar to the beforeChange event added this week, which allows you to check and adjust selection changes as they are made, be helpful for you? If not, please explain the nature of the problem or point me to a previous explanation.

I don't have a clear grasp of what the selection problem you're having is like, exactly. Just an idea -- would a `beforeSelectionChange` event, similar to the `beforeChange` event added this week, which allows you to check and adjust selection changes as they are made, be helpful for you? If not, please explain the nature of the problem or point me to a previous explanation.
marijnh commented 2013-02-20 13:15:09 +01:00 (Migrated from gitlab.com)
Copy link

Should I merge the patches here as they stand, by the way, or is there going to be more adjustment?

Should I merge the patches here as they stand, by the way, or is there going to be more adjustment?
marijnh commented 2013-02-20 16:00:39 +01:00 (Migrated from gitlab.com)
Copy link

The idea of beforeSelectionChange sounds really interesting, maybe that would work even better than an extended api for setSelection().

I don't have a clear grasp of what the selection problem you're having is like, exactly.

As I've written in the notes on visual mode, there are a few differences between vim-selection and the default selection handling. The short version: it covers at least one additional character at the end, and sometimes more in both directions (moving sel.from and sel.to without changing sel.anchor or sel.head).

You can also try the vim-demo if you prefer that, press v to activate visual mode and move around using hjkl/arrow keys to select text. You'll notice that in the current version on the homepage the selection does not include the character under the cursor, while in my version the selection expands under the cursor as well. One notable special situation is that on pressing v, initially sel.to is set to sel.head+1, while sel.anchor and sel.head are still in the same position, which would make setSelection() return early (in the homepage-version there is some flipping of head and anchor to avoid that and I could work around that by moving the cursor before setting selection). Another thing to try is V for linewise visual mode, which now allows to move the cursor freely within the selected lines (so sel.from and sel.to are only moved whenever sel.head changes lines).

While what I do works whenever I am actively setting selection, a few areas are out of reach this way (namely click/drag and undo). The vim-keymap does not register for any events at this point, but I'm planning to try your suggestion from #1084 soon. What I like about beforeSelectionChange is that it might allow me to cover both cases with common code. Do you think it should be safe to modify sel.from and sel.to in general and especially during beforeSelectionChange? In that case I would call the normal setSelection() (which I now emulate instead) and do my final touches in the event handler.

As for the merge it depends on whether @mightyguava likes the current state and the way I implemented his suggestions. Especially he might not want the workaround of reimplementing setSelection() to stay in, which I would understand.

The idea of `beforeSelectionChange` sounds really interesting, maybe that would work even better than an extended api for `setSelection()`. > I don't have a clear grasp of what the selection problem you're having is like, exactly. As I've written in the notes on visual mode, there are a few differences between vim-selection and the default selection handling. The short version: it covers at least one additional character at the end, and sometimes more in both directions (moving `sel.from` and `sel.to` without changing `sel.anchor` or `sel.head`). You can also try the vim-demo if you prefer that, press `v` to activate visual mode and move around using hjkl/arrow keys to select text. You'll notice that in the current version on the homepage the selection does not include the character under the cursor, while in my version the selection expands under the cursor as well. One notable special situation is that on pressing `v`, initially `sel.to` is set to `sel.head+1`, while `sel.anchor` and `sel.head` are still in the same position, which would make `setSelection()` return early (in the homepage-version there is some flipping of `head` and `anchor` to avoid that and I could work around that by moving the cursor before setting selection). Another thing to try is `V` for linewise visual mode, which now allows to move the cursor freely within the selected lines (so `sel.from` and `sel.to` are only moved whenever `sel.head` changes lines). While what I do works whenever I am actively setting selection, a few areas are out of reach this way (namely click/drag and undo). The vim-keymap does not register for any events at this point, but I'm planning to try your suggestion from #1084 soon. What I like about `beforeSelectionChange` is that it might allow me to cover both cases with common code. Do you think it should be safe to modify `sel.from` and `sel.to` in general and especially during `beforeSelectionChange`? In that case I would call the normal `setSelection()` (which I now emulate instead) and do my final touches in the event handler. As for the merge it depends on whether `@mightyguava` likes the current state and the way I implemented his suggestions. Especially he might not want the workaround of reimplementing `setSelection()` to stay in, which I would understand.
marijnh commented 2013-02-20 16:30:26 +01:00 (Migrated from gitlab.com)
Copy link

+1 vote for something like beforeSelectionChange - that definitely has uses on my end as well.

+1 vote for something like beforeSelectionChange - that definitely has uses on my end as well.
marijnh commented 2013-02-20 16:33:11 +01:00 (Migrated from gitlab.com)
Copy link

👍 for beforeSelectionChange -- Would be useful in my project.

:+1: for `beforeSelectionChange` -- Would be useful in my project.
marijnh commented 2013-02-21 05:45:33 +01:00 (Migrated from gitlab.com)
Copy link

If it would be safe to modify sel.from and sel.to in a beforeSelectionChange event, without changing sel.anchor and sel.head, then I agree it could make for a great solution.

As for the merge it depends on whether @mightyguava likes the current state and the way I implemented his suggestions. Especially he might not want the workaround of reimplementing setSelection() to stay in, which I would understand.

Ah didn't know you were waiting on me for this. If @marijnh will be adding beforeSelectionChange, then I would rather the code to emulate setSelection to not be merged. As I said before my concern is forward compatibility with possible future changes to CodeMirror's selection model (for example, multi selections https://github.com/marijnh/CodeMirror/issues/778). I don't mean to make it hard on you but I'd rather not chance breaking visual mode completely down the road. Thanks for bearing with me.

If it would be safe to modify `sel.from` and `sel.to` in a `beforeSelectionChange` event, without changing `sel.anchor` and `sel.head`, then I agree it could make for a great solution. > As for the merge it depends on whether `@mightyguava` likes the current state and the way I implemented his suggestions. Especially he might not want the workaround of reimplementing setSelection() to stay in, which I would understand. Ah didn't know you were waiting on me for this. If `@marijnh` will be adding `beforeSelectionChange`, then I would rather the code to emulate `setSelection` to not be merged. As I said before my concern is forward compatibility with possible future changes to CodeMirror's selection model (for example, multi selections https://github.com/marijnh/CodeMirror/issues/778). I don't mean to make it hard on you but I'd rather not chance breaking visual mode completely down the road. Thanks for bearing with me.
marijnh commented 2013-02-21 11:52:52 +01:00 (Migrated from gitlab.com)
Copy link

If it would be safe to modify sel.from and sel.to in a beforeSelectionChange event, without changing sel.anchor and sel.head, then I agree it could make for a great solution.

There's an invariant that from/to always correspond to anchor/head (from to the lesser one, to to the greater). This invariant is not going away.

> If it would be safe to modify sel.from and sel.to in a beforeSelectionChange event, without changing sel.anchor and sel.head, then I agree it could make for a great solution. There's an invariant that from/to always correspond to anchor/head (from to the lesser one, to to the greater). This invariant is not going away.
marijnh commented 2013-02-21 12:38:06 +01:00 (Migrated from gitlab.com)
Copy link

There's an invariant that from/to always correspond to anchor/head (from to the lesser one, to to the greater). This invariant is not going away.

Ok, that kicks my approach out of the window. I understand that you don't want to risk stability issues you'd have to deal with though, I'll see if I can get my changes to visual mode out again so the remaining ones can be merged.

> There's an invariant that from/to always correspond to anchor/head (from to the lesser one, to to the greater). This invariant is not going away. Ok, that kicks my approach out of the window. I understand that you don't want to risk stability issues you'd have to deal with though, I'll see if I can get my changes to visual mode out again so the remaining ones can be merged.
marijnh commented 2013-02-22 14:31:42 +01:00 (Migrated from gitlab.com)
Copy link

@mightyguava are you ok with how the patch looks right now? In that case I'd suggest a merge as

[vim keymap] better r, t, new +/-/_
- handling special keys better for commands requiring a character
- r can operate on selection
- fixed: t moves back if character not found
- +/-/_ via extension of moveByLines motion

Also I'd like you to take a look at d776f74 if you're interested. There I played around with making the selection right but the (now hidden) cursor wrong.

`@mightyguava` are you ok with how the patch looks right now? In that case I'd suggest a merge as ``` [vim keymap] better r, t, new +/-/_ - handling special keys better for commands requiring a character - r can operate on selection - fixed: t moves back if character not found - +/-/_ via extension of moveByLines motion ``` Also I'd like you to take a look at [d776f74](https://github.com/metatheos/CodeMirror/tree/d776f74060495d7e982f9d7245dc429e56c8283a) if you're interested. There I played around with making the selection right but the (now hidden) cursor wrong.
marijnh commented 2013-02-22 15:24:54 +01:00 (Migrated from gitlab.com)
Copy link

I've merged the changes as they stand.

(I somehow missed the horrible things you were doing in your setSelection replacement before. Such an approach is not acceptable. All selection changes are supposed to go through CodeMirror's internal setSelection, and poking at the selection from outside is bound to create all kinds of inconsistencies.)

I've merged the changes as they stand. (I somehow missed the horrible things you were doing in your `setSelection` replacement before. Such an approach is **not** acceptable. All selection changes are supposed to go through CodeMirror's internal `setSelection`, and poking at the selection from outside is bound to create all kinds of inconsistencies.)
marijnh (Migrated from gitlab.com) closed this pull request 2013-02-22 15:24:54 +01:00
marijnh commented 2013-02-22 20:20:29 +01:00 (Migrated from gitlab.com)
Copy link

You're right, the original commit would have better just been an use-case. I should have opened a normal Issue first to ask whether sel.from/sel.to values not being a permutation of sel.anchor/sel.head is something CodeMirror is designed to handle. If it was, I still could have made a pull-request with proper api-access to the values. Sorry about that.

You're right, the original commit would have better just been an use-case. I should have opened a normal Issue first to ask whether `sel.from`/`sel.to` values not being a permutation of `sel.anchor`/`sel.head` is something CodeMirror is designed to handle. If it was, I still could have made a pull-request with proper api-access to the values. Sorry about that.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
codemirror/codemirror5!8420
No description provided.