Rearrange and rename some code in engine #553
Merged
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
Can also now apply @TestWithoutNeovim to an entire class
No code changes
Two reasons: 1. It is good if the engine uses Vim terminology. A "screen" in Vim includes all window/display lines from all windows in the terminal screen, including the status and command line. IntelliJ doesn't have this concept. A display line is most similar to IntelliJ's visual lines, but describes the window/display's character grid, not the buffer contents. 2. moveCaretToMiddleColumn needs renaming to indicate that it's for a display line, not a logical line, and to make way for an implementation of `gM`, which does work on logical lines
I.e. line number (currently buffer, should be Vim logical line), current line, or relative to current line
AlexPl292
reviewed
Nov 15, 2022
| fun moveCaretToColumn(editor: VimEditor, caret: VimCaret, count: Int, allowEnd: Boolean): Motion | ||
|
|
||
| // Move caret to column relative to the bounds of the display (aka window) | ||
| fun moveCaretToCurrentDisplayLineStart(editor: VimEditor, caret: VimCaret): Motion |
There was a problem hiding this comment.
Just sharing plans: I also would like to get rid of the "move" inside such method because it looks like this method does some modification while it's not
There was a problem hiding this comment.
I haven't been able to come up with a good name for this though - calculateMotionToCurrentDisplayLineStart or motionToCurrentDisplayLineStart? Or even more simple, because it's always called through motion - injector.motion.toCurrentDisplayLineStart?
AlexPl292
approved these changes
Nov 15, 2022
| @@ -93,10 +92,6 @@ class IjVimCaret(val caret: Caret) : VimCaretBase() { | |||
| this.caret.moveToLogicalPosition(LogicalPosition(logicalPosition.line, logicalPosition.column, logicalPosition.leansForward)) | |||
| } | |||
|
|
|||
| override fun offsetForLineStartSkipLeading(line: Int): Int { | |||
There was a problem hiding this comment.
Removing methods from our Vim*** classes makes me so happy
|
Awesome, thank you Matt. Looking forward to the next changes. |
|
Be careful what you wish for 😁 |
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 is mostly refactoring, with very little change in logic. The changes are related to a forthcoming PR with fixes for soft wrap support, and is pulled into a separate PR to try to reduce the noise for the next PR.
The main changes in this PR:
VimMotionGroupandVimCaretto group related functions, to make it easier to see what functions are availableVimMotionGroupto "display". This is to try to start using more Vim terminology and less IntelliJ terminology in the engine API. In Vim, a "screen" is a collection of all visible windows, as well as the command line and status bar. It would be more correct to use "window", but to avoid confusion with GUI windows, I've chosen to use "display", which the Vim help uses in places to mean "window" lines.moveCaretTo…to clarify if it moves to buffer line (e.g.100G), moves relative to current line (2j) or moves the caret to the start/end of the current lineThese changes only affect the engine API in
VimMotionGroup, and don't (yet) rename actions or implementations in the editor helper classes.The forthcoming changes for soft wrap make it more important to use the correct terminology, because we currently work in a mix of IntelliJ logical lines, which are equivalent to Vim's "buffer" lines, and visual lines, which don't really have a Vim equivalent. E.g. motions such as
jandkwork in visual lines, but should be in "Vim logical" lines, which are buffer lines with folding applied. Scrolling is also currently in visual lines, but should also be in "Vim logical" lines. We should be doing much more in terms of "Vim logical" lines.(Vim does have "display" or "window" lines, which map to the static window character grid and don't change as the text content is scrolled, modified or wrapped, unlike IntelliJ's visual lines. Vim has also recently introduced a
'smoothscroll'option that will scroll by display line when using<C-E>and<C-Y>, but it's off by default. And to make things even more interesting, Vim only allows folding complete lines, but IntelliJ can fold arbitrary lines, like in a Java getter or setter method, which means a "Vim logical" line should map to a range of start/end buffer lines 😁)