[vim] clip cursor, fix #1084

marijnh commented

2013-06-23 01:05:30 +02:00

(Migrated from gitlab.com)

my try at a fix for #1084. instead of @metatheos' approach of tracking vim.head, i peek at the current mode

some notes / thoughts:

the mode should be a string instead of an increasing number of bools, which would eventually show up in the status bar #1628
similarly, instead of defining a new bool for each keymap w/ an init function, keymaps should have optional callbacks. although, vim may be the only keymap needing an init function
i had to modify codemirror's historyChangeFromChange, as modifying the cursor in the beforeSelectionChange callback caused an issue where undoing a deletion would delete a character to the left of the cursor
@mightyguava

my try at a fix for #1084. instead of `@metatheos`' approach of tracking `vim.head`, i peek at the current mode some notes / thoughts: - the mode should be a string instead of an increasing number of bools, which would eventually show up in the status bar #1628 - similarly, instead of defining a new bool for each keymap w/ an init function, keymaps should have optional callbacks. although, vim may be the only keymap needing an init function - i had to modify codemirror's `historyChangeFromChange`, as modifying the cursor in the `beforeSelectionChange` callback caused an issue where undoing a deletion would delete a character to the left of the cursor ` @mightyguava`

marijnh commented

2013-06-23 06:27:13 +02:00

(Migrated from gitlab.com)

This looks like a breaking change for existing users of the vim keymap, which is not ok. We need to have a period of time (at least a few CodeMirror releases and possibly until the next major release) for both registering as a keymap and registering as a mode to work. I would restore the initialization functions, keep track of whether Vim is initialized, and if initialization has not already been done by the time handleKey is executed, do the initialization then.

Also why are you clearing global state whenever the vimMode option is set? The whole point of global state is for it to persist between codemirror instances, even if new instances are created later.

Finally, there are a few reasons why I kept the state as getters instead of directly referencing the variables

I don't like the fact that vimState needs to be kept as cm.vimState. That vim and cm are kept distinct with getVimState(cm) makes it easier for me to think about them separately and leaves room for a different future implementation.
And at some point, I think it would be good add an option to specify the object to use for global state instead of creating an internal object. This both improves testability and gives the user more control over the keymap. Keeping truly global state is pretty bad
Having functions leaves more room for abstraction and testing niceness if I ever decide to clean up the mess of unit tests.

This looks like a breaking change for existing users of the vim keymap, which is not ok. We need to have a period of time (at least a few CodeMirror releases and possibly until the next major release) for both registering as a keymap and registering as a mode to work. I would restore the initialization functions, keep track of whether Vim is initialized, and if initialization has not already been done by the time `handleKey` is executed, do the initialization then. Also why are you clearing global state whenever the `vimMode` option is set? The whole point of global state is for it to persist between codemirror instances, even if new instances are created later. Finally, there are a few reasons why I kept the state as getters instead of directly referencing the variables 1. I don't like the fact that `vimState` needs to be kept as `cm.vimState`. That `vim` and `cm` are kept distinct with `getVimState(cm)` makes it easier for me to think about them separately and leaves room for a different future implementation. 2. And at some point, I think it would be good add an option to specify the object to use for global state instead of creating an internal object. This both improves testability and gives the user more control over the keymap. Keeping truly global state is pretty bad 3. Having functions leaves more room for abstraction and testing niceness if I ever decide to clean up the mess of unit tests.

marijnh commented

2013-06-26 19:03:37 +02:00

(Migrated from gitlab.com)

if vim is kept as a keymap instead of an option, how can i deregister the beforeSelectionChange listener when the keymap is changed? should i only add that listener if vim is an option (leaving behind legacy users), or should i patch codemirror to allow update functions in keymaps? i suppose i could call getOption('keyMap') inside the listener

https://github.com/marijnh/CodeMirror/issues/1084#issuecomment-15012968

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.

this led me to believe that you were ok with storing vimState on cm

if vim is kept as a keymap instead of an option, how can i deregister the `beforeSelectionChange` listener when the keymap is changed? should i only add that listener if vim is an option (leaving behind legacy users), or should i patch codemirror to allow update functions in keymaps? i suppose i could call `getOption('keyMap')` inside the listener https://github.com/marijnh/CodeMirror/issues/1084#issuecomment-15012968 > > 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. this led me to believe that you were ok with storing `vimState` on `cm`

marijnh commented

2013-06-27 04:23:23 +02:00

(Migrated from gitlab.com)

if vim is kept as a keymap instead of an option, how can i deregister the beforeSelectionChange listener when the keymap is changed? should i only add that listener if vim is an option (leaving behind legacy users), or should i patch codemirror to allow update functions in keymaps? i suppose i could call getOption('keyMap') inside the listener

I agree with you and @marijnh that the Vim keymap has grown enough that it should be considered more of a mode than a keymap, so I wouldn't patch codemirror to callback on keymap change.

I think not deregistering the beforeSelectionChange is fine because keymap change isn't really something that would usually be done except at cm initialization, and we currently don't deregister any of the visual/insert mode listeners on keymap change. But that's a weak argument, so leaving behind legacy users would also be an ok option. Either is fine really. I just don't want this change to completely break the vim keymap.

#1084

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.
this led me to believe that you were ok with storing vimState on cm

I am a bit back and forth on that yes... Since you called it out, stick with storing vimState on cm, but I still prefer to keep the getVimGlobalState function.

> if vim is kept as a keymap instead of an option, how can i deregister the beforeSelectionChange listener when the keymap is changed? should i only add that listener if vim is an option (leaving behind legacy users), or should i patch codemirror to allow update functions in keymaps? i suppose i could call getOption('keyMap') inside the listener I agree with you and `@marijnh` that the Vim keymap has grown enough that it should be considered more of a mode than a keymap, so I wouldn't patch codemirror to callback on keymap change. I think not deregistering the `beforeSelectionChange` is fine because keymap change isn't really something that would usually be done except at cm initialization, and we currently don't deregister any of the visual/insert mode listeners on keymap change. But that's a weak argument, so leaving behind legacy users would also be an ok option. Either is fine really. I just don't want this change to completely break the vim keymap. > #1084 > > 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. > this led me to believe that you were ok with storing vimState on cm I am a bit back and forth on that yes... Since you called it out, stick with storing `vimState` on `cm`, but I still prefer to keep the `getVimGlobalState` function.

marijnh commented

2013-06-27 08:20:43 +02:00

(Migrated from gitlab.com)

keymap change isn't really something that happens except at cm instance creation

Well, that's not really true. setOption on the keyMap option is expected to work, and I can imagine IDEs providing a keybindings dropdown somewhere in the interface allowing people to switch bindings on the fly.

As for storing things in a CM instance -- a lot of addons have moved to a model where they add a property to the .state object of editor instances, so doing that too is nothing to be ashamed of.

> keymap change isn't really something that happens except at cm instance creation Well, that's not really true. `setOption` on the keyMap option is expected to work, and I can imagine IDEs providing a keybindings dropdown somewhere in the interface allowing people to switch bindings on the fly. As for storing things in a CM instance -- a lot of addons have moved to a model where they add a property to the `.state` object of editor instances, so doing that too is nothing to be ashamed of.

marijnh commented

2013-06-27 15:34:20 +02:00

(Migrated from gitlab.com)

state checked for in handleKey, globalState reset only once, beforeSelectionChange listener added only when vim is an option. also switched to the .state convention

state checked for in `handleKey`, `globalState` reset only once, `beforeSelectionChange` listener added only when vim is an option. also switched to the `.state` convention

marijnh commented

2013-06-29 03:02:09 +02:00

(Migrated from gitlab.com)

getVimState isn't really useful anymore. Can you rename it to maybeInitVimState() and use it only in defineOption and handleKey?

Looks good otherwise. Thanks for the changes.

`getVimState` isn't really useful anymore. Can you rename it to `maybeInitVimState()` and use it only in `defineOption` and `handleKey`? Looks good otherwise. Thanks for the changes.

marijnh commented

2013-06-29 20:12:48 +02:00

(Migrated from gitlab.com)

done

marijnh commented

2013-06-30 18:06:43 +02:00

(Migrated from gitlab.com)

Thanks, good to merge.

marijnh commented

2013-07-02 13:44:13 +02:00

(Migrated from gitlab.com)

Nice. Merged as 70a267a