[vim] click places the cursor in illegal position

marijnh commented

2012-12-11 07:16:51 +01:00

(Migrated from gitlab.com)

When using http://codemirror.net/demo/vim.html, moving around with direction keys doesn't allow the fat cursor to go past the last column. However, clicking to the far right of a line will place the cursor just after the last column. Instead, it should place it on the last column.

When using http://codemirror.net/demo/vim.html, moving around with direction keys doesn't allow the fat cursor to go past the last column. However, clicking to the far right of a line will place the cursor just after the last column. Instead, it should place it _on_ the last column.

marijnh commented

2012-12-11 12:01:12 +01:00

(Migrated from gitlab.com)

As an aside, you might want to have the vim mode use defineOption to add a vimMode option which, when turned on, sets the keymap to vim and does a few other things, such as registering cursoractivity handlers that force the cursor back when it is placed in an invalid position.

As an aside, you might want to have the vim mode use `defineOption` to add a `vimMode` option which, when turned on, sets the keymap to vim _and does a few other things_, such as registering cursoractivity handlers that force the cursor back when it is placed in an invalid position.

marijnh commented

2013-02-19 17:34:42 +01:00

(Migrated from gitlab.com)

mentioned in merge request !8420

marijnh commented

2013-03-15 20:38:51 +01:00

(Migrated from gitlab.com)

As an aside, you might want to have the vim mode use defineOption to add a vimMode option which, when turned on, sets the keymap to vim and does a few other things, such as registering cursoractivity handlers that force the cursor back when it is placed in an invalid position.

I've been looking into this suggestion recently and thought I should share my progress at some point so @mightyguava can look at it and make a few suggestions. In metatheos/CodeMirror@318c66d8ba I made vim.js register the option vimlike. It sets the keymap, creates the vim-state and registers for beforeSelectionChange-events.

The large amount of red in the commit might be surprising. Actually it was reasonably easy to get the basic functionality of handling cursor-changes and turning visual mode on/off based on mouse input. The most difficult point was to deal with vertical movement since I need to set the two column-variables whenever there is an external change to the cursor.

To solve this I've ended up managing a local vim.head so I can (mostly) tell where a change is coming from. To make sure the head is updated in every local operation, those operations now use setCursor(cm,vim,pos,force) which sets the cursor and also updates vim.head. The force-parameter is intended for all those cases where the head is allowed to be placed after the last character of a line.

There are a few additional changes covered in the commit:

Cursor is clipped correctly after d and D (adapted tests as well)
In visual mode I'm not using the cursor as extension of selection anymore, since I found the behavior confusing and inconsistent. I'm hiding the cursor, since I can't either show my own or change it to beam for the duration of selecting.

I didn't make a pull request yet, since with all that shotgun-surgery there might be issues no test caught. Additionally I'm thinking of getting rid of all the passing around of vim, since with the vim-state being created during the option-change I can expect cm.vimState to be valid. I might also improve the handling during linewise visual mode by managin a local anchor.

> As an aside, you might want to have the vim mode use `defineOption` to add a `vimMode` option which, when turned on, sets the keymap to vim _and does a few other things_, such as registering cursoractivity handlers that force the cursor back when it is placed in an invalid position. I've been looking into this suggestion recently and thought I should share my progress at some point so `@mightyguava` can look at it and make a few suggestions. In metatheos/CodeMirror@318c66d8baf2d246925263fc0c21232067fd8fa0 I made `vim.js` register the option `vimlike`. It sets the keymap, creates the vim-state and registers for `beforeSelectionChange`-events. The large amount of red in the commit might be surprising. Actually it was reasonably easy to get the basic functionality of handling cursor-changes and turning visual mode on/off based on mouse input. The most difficult point was to deal with vertical movement since I need to set the two column-variables whenever there is an external change to the cursor. To solve this I've ended up managing a local `vim.head` so I can (mostly) tell where a change is coming from. To make sure the head is updated in every local operation, those operations now use `setCursor(cm,vim,pos,force)` which sets the cursor and also updates `vim.head`. The `force`-parameter is intended for all those cases where the head is allowed to be placed after the last character of a line. There are a few additional changes covered in the commit: - Cursor is clipped correctly after `d` and `D` (adapted tests as well) - In visual mode I'm not using the cursor as extension of selection anymore, since I found the behavior confusing and inconsistent. I'm hiding the cursor, since I can't either show my own or change it to beam for the duration of selecting. I didn't make a pull request yet, since with all that shotgun-surgery there might be issues no test caught. Additionally I'm thinking of getting rid of all the passing around of `vim`, since with the vim-state being created during the option-change I can expect `cm.vimState` to be valid. I might also improve the handling during linewise visual mode by managin a local `anchor`.

marijnh commented

2013-03-15 22:50:00 +01:00

(Migrated from gitlab.com)

The above all sounds very good to me, more or less the direction I hoped the mode would go in. (I didn't look at the code yet, and of course, @mightyguava has the last word what happens in this file)

The above all sounds very good to me, more or less the direction I hoped the mode would go in. (I didn't look at the code yet, and of course, `@mightyguava` has the last word what happens in this file)

marijnh commented

2013-03-16 05:14:57 +01:00

(Migrated from gitlab.com)

Took a pass through the code. Overall I really like it. Here's some initial comments (I'm quite nitpicky :)

selChange() mostly looks good. I'm in agreement with your approach. Will take a deeper look when you make the actual pull request. One suggestion I have is that the logic may read better and be more compact if you switch the order of the conditionals, i.e. make the outer conditional if (!cursorEqual(sel.head, sel.anchor).
I agree that it is confusing to use the cursor as an extension of selection looking at this as a dev, but it is intuitive to me as a user. I initially tried what you are doing now (by extending the actual selection and turning off the cursor), but you no longer see where head is. I would prefer not to change this unless you find a way to highlight head. Maybe by setting a textmarker with a CSS class that follows vim.selHead and pretends to be a cursor?
- If you kept the cursor instead of extending the selection, I don't think you would have to maintain separate vim.head and vim.selHead? Minor enough though.
b is pretty broken in visual mode, and ge is a little buggy too. Didn't look into the cause but might be a bug when head crosses over anchor? I'm sorry to say that there are practically no tests for visual mode so a lot of regressions won't be caught, and the negligence is now coming back to bite. It would be great if you could add tests. I think with a bit of thought, it would be possible to make most of the existing motion tests double as visual mode tests.
The selection can now extend 1 character past the end of line, which causes some quirkiness in vertical movement.
I originally kept vim separate from cm not because it was not guaranteed to be created (getVimState() is called first thing in handleKey, the real entrypoint to vim.js), but that I wanted to abstract hide that vim is a property of cm. The passing around of vim is so that if someday I got around to adding a way to define custom actions/motions/operators they would have access to it. I am open to changes on this if you have better suggestions that would not detract from the above, but let that be in a separate change and focus on visual mode here.
Some style nits: please put { } and line breaks around even trivial conditionals, reasonable spacing around expressions, '' instead of "" for strings for easy searching, as respected by the rest of the file. A consistent style makes the code oh so much easier to read.
Could you split the changes to d and D and text object manipulation into a separate branch? It should be straightforward with git add -p.

Took a pass through the code. Overall I really like it. Here's some initial comments (I'm quite nitpicky :) - `selChange()` mostly looks good. I'm in agreement with your approach. Will take a deeper look when you make the actual pull request. One suggestion I have is that the logic may read better and be more compact if you switch the order of the conditionals, i.e. make the outer conditional `if (!cursorEqual(sel.head, sel.anchor)`. - I agree that it is confusing to use the cursor as an extension of selection looking at this as a dev, but it is intuitive to me as a user. I initially tried what you are doing now (by extending the actual selection and turning off the cursor), but you no longer see where `head` is. I would prefer not to change this unless you find a way to highlight head. Maybe by setting a textmarker with a CSS class that follows `vim.selHead` and pretends to be a cursor? - If you kept the cursor instead of extending the selection, I don't think you would have to maintain separate `vim.head` and `vim.selHead`? Minor enough though. - `b` is pretty broken in visual mode, and `ge` is a little buggy too. Didn't look into the cause but might be a bug when `head` crosses over `anchor`? I'm sorry to say that there are practically no tests for visual mode so a lot of regressions won't be caught, and the negligence is now coming back to bite. It would be great if you could add tests. I think with a bit of thought, it would be possible to make most of the existing motion tests double as visual mode tests. - The selection can now extend 1 character past the end of line, which causes some quirkiness in vertical movement. - I originally kept `vim` separate from `cm` not because it was not guaranteed to be created (`getVimState()` is called first thing in `handleKey`, the real entrypoint to `vim.js`), but that I wanted to abstract hide that `vim` is a property of `cm`. The passing around of `vim` is so that if someday I got around to adding a way to define custom actions/motions/operators they would have access to it. I am open to changes on this if you have better suggestions that would not detract from the above, but let that be in a separate change and focus on visual mode here. - Some style nits: please put { } and line breaks around even trivial conditionals, reasonable spacing around expressions, '' instead of "" for strings for easy searching, as respected by the rest of the file. A consistent style makes the code oh so much easier to read. - Could you split the changes to `d` and `D` and text object manipulation into a separate branch? It should be straightforward with `git add -p`.

marijnh commented

2013-03-16 13:21:19 +01:00

(Migrated from gitlab.com)

One suggestion I have is that the logic may read better and be more compact if you switch the order of the conditionals

You're right I'll really need to look over all the code for the pull request, reorder things and look for obsolete code. This is just the state after I finally got it to mostly behave right.

but it is intuitive to me as a user

I had mostly two issues from the user-standpoint:

It's visually inconsistent (sometimes the cursor is over the selection, sometimes it isn't) and a blinking extension for selection is not intuitively identifiable as either being covered by operations or not.
It's technically inconsistent. If you d text selected through visual mode, the result is different from right click and cut.

Showing a fake cursor to the user would indeed be a solid alternative to moving the actual sel.head, sel.from and sel.to around independently. Unfortunately I don't have any idea if there is a clean way to do this. @marijnh maybe you can think of something to show a cursor at a custom location without invasive changes to CodeMirror itself?

I don't think you would have to maintain separate vim.head and vim.selHead?

Might very well be true, I have a branch in my personal setup where I could try merging back a few of the issues I've fixed later on. There I've tried allowing to pass my own sel.from and sel.to in the beforeSelectionChange-callback and didn't need this separation.

b is pretty broken in visual mode, and ge is a little buggy too.
The selection can now extend 1 character past the end of line

I'll look into that and add tests, thanks for trying it out. I've touched way too much code to find all issues fast myself I fear.

but that I wanted to abstract hide that vim is a property of cm

How about having vim.cm store a pointer to the parent object and pass around vim instead of cm?

Some style nits

Sure, I'll add that to my cleanup

Could you split the changes to d and D and text object manipulation into a separate branch?

As for d and D this would be difficult, since it fell into place by itself and I had to make tests check for the correct behavior, I've actually changed c and C to avoid clipping there. The changes to text object manipulation on the other hand should be filtered out, I included them by accident since they were lingering around from before the other changes. I wanted diw,caW and anything similar to behave better, but tripped on whitespace-handling around eol.

> One suggestion I have is that the logic may read better and be more compact if you switch the order of the conditionals You're right I'll really need to look over all the code for the pull request, reorder things and look for obsolete code. This is just the state after I finally got it to mostly behave right. > but it is intuitive to me as a user I had mostly two issues from the user-standpoint: 1. It's visually inconsistent (sometimes the cursor is over the selection, sometimes it isn't) and a blinking extension for selection is not intuitively identifiable as either being covered by operations or not. 2. It's technically inconsistent. If you `d` text selected through visual mode, the result is different from right click and cut. Showing a fake cursor to the user would indeed be a solid alternative to moving the actual `sel.head`, `sel.from` and `sel.to` around independently. Unfortunately I don't have any idea if there is a clean way to do this. `@marijnh` maybe you can think of something to show a cursor at a custom location without invasive changes to CodeMirror itself? > I don't think you would have to maintain separate `vim.head` and `vim.selHead`? Might very well be true, I have a branch in my personal setup where I could try merging back a few of the issues I've fixed later on. There I've tried allowing to pass my own `sel.from` and `sel.to` in the `beforeSelectionChange`-callback and didn't need this separation. > `b` is pretty broken in visual mode, and `ge` is a little buggy too. > The selection can now extend 1 character past the end of line I'll look into that and add tests, thanks for trying it out. I've touched way too much code to find all issues fast myself I fear. > but that I wanted to abstract hide that `vim` is a property of `cm` How about having `vim.cm` store a pointer to the parent object and pass around `vim` instead of `cm`? > Some style nits Sure, I'll add that to my cleanup > Could you split the changes to `d` and `D` and text object manipulation into a separate branch? As for `d` and `D` this would be difficult, since it fell into place by itself and I had to make tests check for the correct behavior, I've actually changed `c` and `C` to avoid clipping there. The changes to text object manipulation on the other hand should be filtered out, I included them by accident since they were lingering around from before the other changes. I wanted `diw`,`caW` and anything similar to behave better, but tripped on whitespace-handling around eol.

marijnh commented

2013-03-16 15:55:12 +01:00

(Migrated from gitlab.com)

Unfortunately I don't have any idea if there is a clean way to do this. @marijnh maybe you can think of something to show a cursor at a custom location without invasive changes to CodeMirror itself?

I didn't notice those inconsistences before. Thanks for pointing them out.

I don't think using cm.markText() is an invasive change. I was thinking that you would maintain a TextMarker object (separate from the vim marks) that follows selHead, update it in selChange, and add a small CSS class (or use the fat cursor's) for the mark in codemirror.css. While not ideal, it would be ok if the cursor didn't blink in visual mode.

How about having vim.cm store a pointer to the parent object and pass around vim instead of cm?

That does sound good, but would that create a circular reference, which may cause garbage collection problems trying to dispose of cm? I am not experienced with Javascript memory. @marijnh do you have any suggestions on a good way to have an abstraction between vim and cm state yet link one to the other? I thought about maintaining a map of cm => vim in vim.js but wasn't sure if that would end up causing garbage collection issues als well.

As for d and D this would be difficult

Sounds good, leave it in. It's just a nit.

> Unfortunately I don't have any idea if there is a clean way to do this. `@marijnh` maybe you can think of something to show a cursor at a custom location without invasive changes to CodeMirror itself? I didn't notice those inconsistences before. Thanks for pointing them out. I don't think using `cm.markText()` is an invasive change. I was thinking that you would maintain a TextMarker object (separate from the vim marks) that follows `selHead`, update it in `selChange`, and add a small CSS class (or use the fat cursor's) for the mark in `codemirror.css`. While not ideal, it would be ok if the cursor didn't blink in visual mode. > How about having vim.cm store a pointer to the parent object and pass around `vim` instead of `cm`? That does sound good, but would that create a circular reference, which may cause garbage collection problems trying to dispose of `cm`? I am not experienced with Javascript memory. `@marijnh` do you have any suggestions on a good way to have an abstraction between `vim` and `cm` state yet link one to the other? I thought about maintaining a map of `cm => vim` in `vim.js` but wasn't sure if that would end up causing garbage collection issues als well. > As for d and D this would be difficult Sounds good, leave it in. It's just a nit.

marijnh commented

2013-03-16 22:35:13 +01:00

(Migrated from gitlab.com)

@marijnh do you have any suggestions on a good way to have an abstraction between vim and cm state yet link one to the other?

What is wrong with just storing it as a property of the CM instance?

Using markText for the cursor should work. It's not as efficient as the default overlay cursor (requires redraws on the lines themselves), but that's only problematic for very long lines.

> `@marijnh` do you have any suggestions on a good way to have an abstraction between vim and cm state yet link one to the other? What is wrong with just storing it as a property of the CM instance? Using `markText` for the cursor should work. It's not as efficient as the default overlay cursor (requires redraws on the lines themselves), but that's only problematic for very long lines.

marijnh commented

2013-03-16 22:58:45 +01:00

(Migrated from gitlab.com)

What is wrong with just storing it as a property of the CM instance?

Nothing wrong with storing it that way. I don't want it to be directly accessed that way since it's a private property. Rethinking it though, it's fine to let the client just break if they play around with the state. So @metatheos feel free to get rid of vim passing, preferably in a separate branch.

> What is wrong with just storing it as a property of the CM instance? Nothing wrong with storing it that way. I don't want it to be directly accessed that way since it's a private property. Rethinking it though, it's fine to let the client just break if they play around with the state. So `@metatheos` feel free to get rid of `vim` passing, preferably in a separate branch.

marijnh commented

2013-06-23 01:07:25 +02:00

(Migrated from gitlab.com)

mentioned in merge request !8788

marijnh commented

2013-07-02 13:43:55 +02:00

(Migrated from gitlab.com)

Closed by 70a267a

marijnh (Migrated from gitlab.com) closed this issue

2013-07-02 13:43:55 +02:00

Rows
Columns

[vim] click places the cursor in illegal position #1084