Cleaning up the MoveLeft and MoveRight functions #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Document what mode the function is expected to be called in, and what positive and negative distances mean.
The rest of this function already expects us to be in visual mode. I think it is clearer if we just use '< and '> instead of relying on a:firstline and a:lastline, which are finicky to use.
Use a single function, to reduce code duplication and reduce the number of corner cases - Always move the same way, using | instead of h,l,0,$. - Always paste the same way, using P instead of sometimes P and sometimes p. - Always use virtualedit=all
I think that silencing errors is no longer necessary now that our movement commands check the bounds before attempting a move.
We should save the default register @" in a local variable instead of a global variable. This way the value is discarded after the function returns instead of being preserved forever.
As before, merge the two functions into a single function to consolidate the logic in a single place. We also introduce the name "before" in the MoveHorizontally functions, to parallel the "after" from MoveVertically functions.
hugomg
changed the title
Hopefully it should be more self explanatory now. The situation that we have to worry about is when we create a selection that is already past the end of line. For example if we have ABC DEFGH and ask to move the rectangle "C FGH" to the right, we want to stay put, because we area already past the end of line. We should be careful to not accidentally move it backwards. Without that `last < shortest` test it would be transformedi into something like C AB FGHED
Looks good to me and works as expected. Do you plan to add more clean ups? |
|
For now the only thing I still want to do is reorder the functions so the two MoveVertically and the two MoveHorizontally functions appear next to each other. But I didn't include that commit in this PR in order to keep the diff easier to read. I could add it to this PR or to a new PR. |
|
As you prefer ;-) |
Put the MoveVertically and the MoveHorizontally functions next to each other.
|
OK, I added that commit as part of this PR. |
|
Great thanks! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR continues the cleanup effort from #45.
I think everything is still working but I would appreciate if you could double check, specially the MoveBlockHorizontally. Some of those changes were not trivial and I'm not 100% sure that I was able to test all the corner cases.