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

Reset goal column on all cursor changes #17636

Merged
merged 1 commit into from Jul 6, 2018

Conversation

Projects
None yet
2 participants
@matthewwithanm
Member

matthewwithanm commented Jul 6, 2018

Previously, pressing the home key (move-to-first-character-of-line)
while on an empty line wouldn't clear the goal column. This is because
it was only cleared on cursor change and that didn't result in a
change. With this commit, it's always cleared. Operations that want
to preserve the goal column can reset it afterwards.

Reset goal column on all cursor changes
Previously, pressing the home key (move-to-first-character-of-line)
while on an empty line wouldn't clear the goal column. This is because
it was only cleared on cursor *change* and that didn't result in a
change. With this commit, it's *always* cleared. Operations that want
to preserve the goal column can reset it afterwards.

@matthewwithanm matthewwithanm requested a review from maxbrunsfeld Jul 6, 2018

@maxbrunsfeld

Thanks for jumping on this!

This change seems like it'll need some manual testing to make sure that everything related to goal columns still works as expected. Does everything feel right after coding for a while with this change?

this.setBufferPosition(this.editor.getEofBufferPosition())
this.goalColumn = column

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jul 6, 2018

Contributor

Why is the goalColumn only preserved this way in this one method?

Edit - Nevermind, I'm guessing this is the only cursor motion besides moveUp and moveDown that needs to be affected by the goal column. That's interesting; I would have thought there would be more.

This comment has been minimized.

@matthewwithanm

matthewwithanm Jul 6, 2018

Member

honestly, there may be. this is the only one that caused a test to fail though.

@matthewwithanm

This comment has been minimized.

Member

matthewwithanm commented Jul 6, 2018

yeah, it feels right. granted i haven't coded much with it, but just doing the normal things (selecting lines, home, end, etc.) all seem good.

@maxbrunsfeld maxbrunsfeld merged commit bb3cce9 into master Jul 6, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the fb-mdt-reset-goal-column-on-home branch Jul 6, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Jul 6, 2018

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment