Skip to content
This repository was archived by the owner on Apr 6, 2018. It is now read-only.

Conversation

@t9md
Copy link
Contributor

@t9md t9md commented Jul 21, 2015

This PR fix #785, but need discussion.

@t9md
Copy link
Contributor Author

t9md commented Jul 21, 2015

Finished this PR. Not adding spec, currently there is no test to detect this big behavior change.. need discussion further.

@jacekkopecky
Copy link
Contributor

Would it make sense, before merging, to combine all the commits into one? It ends up being a rather small and effective patch.
In think the specs for "horizontal scroll cursor keybindings" in spec/scroll-spec.coffee do something with editor width, maybe that can help with adding specs for behaviour with soft-wrapped lines.

@t9md t9md force-pushed the fix_LineWiseMovementForWrappedLine branch from 7791f5e to 1c6db91 Compare July 21, 2015 14:40
@bronson
Copy link
Contributor

bronson commented Jul 21, 2015

Cool! It would be great to remove the mealy "Currently, vim-mode requires soft line wraps to be disabled" excuse from vim-mode's readme.

That said, I run vim-mode always operating on screen lines: https://github.com/bronson/dotfiles/blob/master/.vimrc#L160-L178 So, while I'm happy to see a patch that improves Vim compatibility, I'd probably find a way to turn it off for myself. :)

Think this should include the gh, gj, gk, and gl bindings too?

@t9md
Copy link
Contributor Author

t9md commented Jul 22, 2015

"Currently, vim-mode requires soft line wraps to be disabled"

I read this sentence before, but completely have forgotten! Thanks for noting that.

I think we only need to take care of gj and gk. No gh, gl on Vim.

Here is my TODO or plan for this patch.

  • review all motion assuming bufferline motion and update to use screencount unit as necessary
  • introduce gk gj new motion which work on screenLine unit
  • update keymap
  • add spec for these.
  • remove readme's sentence to disable softwrap

This patch got unfit of issue title, this is for removing limitation 'softwrap disabled required for vim-mode'.

@jacekkopecky
Copy link
Contributor

@t9md I just found the screencount approach has a problem: if you go to the last line of a soft-wrapped long line and press j, you will go further down than expected.
To illustrate, let's imagine the first two lines below are a single soft-wrapped line:

this is a very long line that is 
softwrapped in a very narrow window
this is a second line
this is a third line

Now put the cursor on "w" in "softwrapped", press 'j' and you'll end up on the third line because the cursor moves by 2 lines - the screencount of the line it's on.

I suspect we should not use moveDown at all there, instead we should set the cursor's buffer position to its current column and its current row plus however many lines we want.

That should also put the goalColumn behaviour in line with VIM - the cursor in the example above, going down from "w" in "softwrapped", should end up at the end of the second line, not on the space after "this".

@t9md
Copy link
Contributor Author

t9md commented Jul 22, 2015

@jacekkopecky Yes, I've also noticed this PR include bug.
We need to put cursor to column which is counted by head of buffer line.
This is not feature Atom provide, Atom always move cursor screenPosition-wise.

I noticed this is not easy change as I expected at first glance.

I suspect we should not use moveDown at all there, instead we should set the cursor's buffer position to its current column and its current row plus however many lines we want.

Maybe..

Atom itself have only screen line movement motion. So we need to add independent bufferLine movement.
I want to follow Atom's way as much as possible and keep vim-mode thin layer on top of Atom.
But this change is not that.
So need further experiment and consideration this new implementation's impact might have to other vim-mode features.

So I'm in mood to stop this PR for now, and feeling I need to understand how cursor and motion implemented in Atom and vim-mode world.

@bronson
Copy link
Contributor

bronson commented Jul 29, 2015

Well, at least one goal of this patch has been merged. :) https://github.com/atom/vim-mode/pull/797/files

@t9md
Copy link
Contributor Author

t9md commented Nov 7, 2016

Close just because it's too old and I cannot remember.

@t9md t9md closed this Nov 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

can't moveDown/Up from wrapped line in visual-mode.

4 participants