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 cursor off after leaving multi-cursor mode #4237

Open
wants to merge 1 commit into
base: master
from

Conversation

@trkoch
Copy link
Contributor

trkoch commented Nov 3, 2019

What this PR does / why we need it:
Previously when leaving multi-cursor mode with <Esc> the cursor would move one column to the left. For instance, when triggering multi-cursor mode on a single word, the cursor would end at the second to last character. While this is expected behavior when leaving insert mode, it is not for visual mode (which multi-cursor mode is based on). To fix, remove special case handling altogether.

Which issue(s) this PR fixes:
#3775

Special notes for your reviewer:
After giving it some thoughts, I decided to remove the special case completely. I could not reproduce any other case where conditions would match other than move-left after multi-cursor mode. To verify, I added a console.log() in the branch in question, and ran the complete test suite. Only one test wrote a message to the console (the one I changed to expected behavior).

Previously when leaving multi-cursor mode with <Esc> the cursor would
move one column to the left. For instance, when triggering multi-cursor
mode on a single word, the cursor would end at the second to last
character. While this is expected behavior when leaving insert mode, it
is not for visual mode (which multi-cursor mode is based on). To fix,
remove special case handling altogether.

Fixes #3775.
@trkoch

This comment has been minimized.

Copy link
Contributor Author

trkoch commented Nov 3, 2019

Deleted code was introduced with 2cdc557.

@trkoch

This comment has been minimized.

Copy link
Contributor Author

trkoch commented Nov 15, 2019

Any objections?

@stevenguh

This comment has been minimized.

Copy link
Contributor

stevenguh commented Nov 20, 2019

Maybe @jpoon knows more about this code. Also, should we have a specific unit test for this case you are trying to fix?

@trkoch

This comment has been minimized.

Copy link
Contributor Author

trkoch commented Nov 20, 2019

It seems this case was not covered by a test, so in order to guard against regressions, adding a test may be good idea. However I did not add new functionality but rather removed (apparently) misbehaving code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.