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

Fix zz updating desired column #4424

Merged
merged 9 commits into from Dec 28, 2019

Conversation

schu34
Copy link
Contributor

@schu34 schu34 commented Dec 27, 2019

What this PR does / why we need it:
basically the same as #4298. That PR was causing a bunch of difficult to debug issues since it updated the default behaviour for all actions. This PR takes mostly the same approach, except it defaults doesntChangeDesiredColumn to false and just sets it to true for zz. There are probably other actions that update desiredColumn when they shouldn't, and this PR won't address those, but at least now there's an easy route to address those bugs as they're noticed. (just add doesntChangeDesiredColumn = true)

I also added a bunch of unit tests that I wrote while trying to debug the last PR.

Which issue(s) this PR fixes
#4296

Special notes for your reviewer:

src/actions/base.ts Outdated Show resolved Hide resolved
@J-Fields
Copy link
Member

Thanks @schu34, going with this less disruptive approach is probably better, even if there's some subtle existing misbehavior we haven't caught.

@J-Fields
Copy link
Member

What about other commands which move the view, but (generally) not the cursor? For instance, zt/zb or <C-e>/`?

@schu34
Copy link
Contributor Author

schu34 commented Dec 27, 2019

Yeah sure, wasn't aware of all those but easy enough to add. Is there a list of commands that move the view but not the cursor that I could reference?

@J-Fields
Copy link
Member

Not that I know of. Maybe H/L/M too? I can't right now but before I merge I'll double check that these all do behave this way in vim and we didn't miss anything too obvious.

@schu34
Copy link
Contributor Author

schu34 commented Dec 27, 2019

did some testing in neovim. It seems that all the z commands as well as <C-e> don't update the desiredColumn but H/L/M/<C-y> take you to the first non-whitespace character of the line so they do.

@J-Fields
Copy link
Member

I think that behavior is dependent on startofline being set. Can you double check that the desired column isn't updated when that's unset?

@schu34
Copy link
Contributor Author

schu34 commented Dec 28, 2019

Yeah, you're right about startofline, in actual vim when it's not set the desired column isn't updated. but it seems like the plugin currently doesn't respect startofline for H/L/M anyway unless I'm missing something. In any case, should I update doesntChangeDesiredColumn to be a function so it can change values based on config options?

@J-Fields
Copy link
Member

Thanks for pointing that out, I just created #4428.

Yeah, if you could make doesntChangeDesiredColumn a function in preparation for that change, that'd be awesome

@schu34
Copy link
Contributor Author

schu34 commented Dec 28, 2019

Alright updated that and also removed it from BaseMovement since that inherits from BaseAction

@J-Fields
Copy link
Member

I think you missed <C-y>. Other than that, looks good to me. Thanks a lot for the rapid iteration on this!

@J-Fields
Copy link
Member

Thanks again! I'll merge once the tests are confirmed to be green

@J-Fields J-Fields merged commit 798741a into VSCodeVim:master Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants