Refactor CommandBuilder, arguments, etc. #979
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
Only Command has a count. The motion argument is now a sealed class hierarchy, and consists only of the motion action and optional argument. This is to reduce confusion over which count to use, and potential incorrect calculation of the count
Fixes selecting last register if multiple registers are used in a command
It's easier to just look at mode. We don't need the additional check on command builder, because we can't be in OP_PENDING without pushing an operator action to the command builder
Register specific handlers for Operator-pending mode instead of relying on a runtime flag for behaviour. Also refactors and renames some arrow motion handlers.
`OperatorArguments.mode` is the mode *before* the command is completed, so might be Visual, Operator-pending, Insert, etc. It's not immediately obvious this is the case, so we're going to deprecate `OperatorArguments.mode` to avoid confusion with `editor.mode`. It's not required for this method because it's only called for Visual-block mode.
`OperatorArguments.mode` is the mode *before* the command was completed, rather than the current mode, which is non-obvious. E.g. for a command in Insert mode, it will still be Insert, and for a (simple) command in Normal, it will still be Normal. The only difference is that a command such as `dw` would be in Operator-pending before the command is completed. That logic is not required for this method, so it's safe to use the current mode. This allows us to start to deprecate `OperatorArguments.mode`.
Also rename `pushCommandPart` and `completeCommandPart`
Try to keep related functions together: awaiting arguments, count, registers, adding action/argument, processing keystrokes, build, reset.
Also refactors command nodes a bit for better debug/trace output
This also gets rid of BAD_COMMAND, which was set but never checked - the function that set the flag would immediately reset the command builder
citizenmatt
force-pushed
the
refactor/argument-motion
branch
from
a2c78b1
to
06ff832
Compare
AlexPl292
approved these changes
Aug 30, 2024
| @@ -18,23 +18,19 @@ import com.maddyhome.idea.vim.api.moveToMotion | |||
| import com.maddyhome.idea.vim.command.Command | |||
| import com.maddyhome.idea.vim.handler.ShiftedArrowKeyHandler | |||
|
|
|||
| /** | |||
| * @author Alex Plate | |||
There was a problem hiding this comment.
Oops. Sorry. Not sure why I deleted that 😳
There was a problem hiding this comment.
No worries, I actually think these @author annotations are useless and not in trend anymore. I remember I used them just I saw the similar ones by Andrei Vlasovskikh :)
...gine/src/main/kotlin/com/maddyhome/idea/vim/action/motion/leftright/MotionBackspaceAction.kt
Show resolved
Hide resolved
|
Thank you for your refactoring! |
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 a fairly thorough refactoring of
CommandBuilder,ArgumentandOperatorArguments. It might be easier to review the commits separately, as they're fairly small and self-contained, while the final diff ofCommandBuildermight be too messy to see what's changed.The idea of this PR is to tighten up the API around building a command, mostly to try to avoid confusion on concepts such as count. Previously, there were at least three ways to get the count for command being executed. Each
Commandhad a count, and if the command was an operator+motion (such asdw) then the motion would also be aCommandand have its own count. Only one of these was valid, but it was unclear which, and why. The count is also available inOperatorArguments.count, but it's not clear if this is the count for the whole command, or just for the operator or for the argument to the operator, which is a motion.Main changes in this PR:
Commandclass no longer represents an operator+motion (dw) as aCommandwith anotherCommandas an argument. Instead, theCommandhas an action for the operator, and an argument that can be a pair of motion action plus optional argument (e.g. fordfx). This means only a single count.Argumenthas been refactored to be a sealed hierarchy of classes.CommandBuilderhas been extensively refactored to have a simpler implementation.Commandparts, there is now just the action, plus an argument (which might be a motion action with its own argument).addActionandaddArgument, with no state management.OperatorArgumentsclass has been substantially deprecated, although not as much as I'd like.modeproperty that is different toeditor.mode- it's the mode before the command is completed and is only used to capture a runtime flag for the motion action to behave differently when in Operator-pending mode. This has been deprecated, and is no longer used in the codebase - separate action implementations have been registered for Op-pending mode.isOperatorPendingproperty that also doesn't reflect the currenteditor.mode, and is unncessary - it could be replaced withmode is Mode.OP_PENDING. This has also been deprecated and is no longer used.countproperties are unclear on what they represent. Given2d3w, is this just the count for the operator? Furthermore, there was existing code that would multiply the count withCommand.count, which is already multiplied when the command is built. The count is now guaranteed to be the same asCommand.count.OperatorArgumentshave been refactored out.countproperties are now the only useful properties in the class. It would be good to remove this class completely and pass the count directly. However, I'm not sure how disruptive this would be on third party extensions. We can look at this in the future.TextObjectActionHandler, which can provide a range for a given caret. This special case handling of offsets now uses the same logic as other motions, especially text objects.There are other minor refactorings along the way. There are no significant bugs fixed as part of this PR, but it does fix some pathological edge cases.