[vi-mode] Correctly closing unbalanced start edit undo groups #2012
Conversation
| if (_editGroupStart != -1) | ||
| { | ||
| // Nesting not supported. | ||
| throw new InvalidOperationException(); | ||
| } |
There was a problem hiding this comment.
Without this check, for the edit groups started by calling StartEditGroup (without going through GroupUndoHelper.StartGroup), the current edit is not saved anywhere and will be lost. That doesn't seem right.
There was a problem hiding this comment.
If I understand correctly, _groupUndoHelper and _groupUndoStates are used only when in VI mode, and the starting nested edit group issue is only relevant in VI modes.
If so, then maybe we should only skip the _editGroupStart != -1 check in GroupUndoHelper.StartGroup, but not skipping generally in StartEditGroup.
There was a problem hiding this comment.
I have re-introduced a protection in the general StartEditGroup() method.
There was a problem hiding this comment.
I have re-introduced a protection in the general StartEditGroup() method.
@springcomp the latest updates in the PR doesn't show a change in the StartEditGroup() method. Did you push all changes in this branch?
There was a problem hiding this comment.
Sorry for the confusion.
Taking a step back and re-assessing the original feedback, I’m not actually in favor of the requested change.
My understanding is that this would allow nested (pending) edit groups in VI mode only, but still protected the general case from unwantingly starting nested groups.
I have pushed a last commit to address this feedback but this leads to code that is – in my opinion – not very elegant.
Please, let me know what you think.
There was a problem hiding this comment.
@springcomp Thank you so much for your patience.
I spent nearly 4 hours today reviewing the changes and thinking about what the right thing should be. Overall, I think allowing nested edit groups adds too much complexity and may make it more error-prone when writing code, because it will be harder to notice when a nested edit group was made by mistake. It also makes it harder to reason the code, because part of the code base assumes the edit group is always balanced while the other part of the code base allows unbalanced edit groups.
I wonder whether it's possible to make the VI mode works as expected while requiring balanced edit groups.
Take ViReplaceToChar as an example (triggered by cf in command mode), it calls ViInsertWithAppend (trigged by a in command mode) internally which also starts an edit group with your change in this PR (expected behavior when it's explicitly triggered by user). In this case, ViInsertWithAppend should be changed to the following, and ViReplaceToChar should call ViInsertWithAppendImpl instead.
public static void ViInsertWithAppend(ConsoleKeyInfo? key = null, object arg = null)
{
_singleton._groupUndoHelper.StartGroup(ViInsertWithAppend, arg);
ViInsertWithAppendImpl(key, arg);
}
private static void ViInsertWithAppendImpl(ConsoleKeyInfo? key = null, object arg = null)
{
ViInsertMode(key, arg);
ForwardChar(key, arg);
}
| _instigator = instigator; | ||
| _instigatorArg = instigatorArg; |
There was a problem hiding this comment.
It doesn't seem to be right to override the existing _instigator and _instigatorArg. Consider ViReplaceToChar as an example, it calls ViInsertWithAppend internally, which also starts a group with your change, however, when redoing, it should be calling the instigator set by ViReplaceToChar instead of the one set by ViInsertWithAppend, because the use didn't explicitly trigger ViInsertWithAppend.
|
@springcomp Given the concerns described in #2012 (review) and that the PR #3845 was merged, I will close this one. But as always, I really appreciate your consistent contribution to PSRL! |
PR Summary
Fixes #1281.
Adds an
elseblock in each occurrence where a_groupUndoHelper.StartGroup()is made but ultimately unbalanced (not ended) because no matching characters were found. As a result, the next time this operation is attempted PSRL no longer crashes due to not supporting nested undo groups.Changes implementations of the following commands:
When authoring this PR I introduced a regression that boils down to the function
ViInsertWithAppend.This method does not start an undo group, and therefore, when undoing, only the last character appended is removed from the buffer.
Adding a call to
_groupUndoHelper.StartGroup(), however, triggers a nested call, from one of the modified commands above as well as many other occasions, such asViAppendLinefor instance. One attempt would be to just ignore the nested call and simply pretend we are in the same, current group. But I propose another attempt which is to replace all the calls toViInsertWithAppendwhich are done internally by a dedicated method that does the same, but without the call to_groupUndoHelper.StartGroup().Then I noticed that all the occurrences of calls to
ViInsertWithAppendare in a block that looks like:That’s why I propose a new internal method that does this pattern in place of each occurrence of this block in the code.
PR Checklist
Microsoft Reviewers: Open in CodeFlow