-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 multiple issues with expand selection commands and pair/block movement #2921
Changes from 19 commits
2677f01
9d643d2
250521b
2fe9aae
9874514
b6ccb76
25ee40e
fdb53ee
ae5af17
f34047e
263962c
f29ca45
1efed15
8b2fc45
2afe005
4d17c7c
1fed3da
e1bff56
ff7f027
9b344a6
5e32f93
371f3ef
21fb2bf
8210643
7f69e09
7eaee13
5b5a304
e59952e
b60f5f8
bbce8a7
aa37795
3830e2a
f2662f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1338,14 +1338,14 @@ class MoveToMatchingBracket extends BaseMovement { | |
if (PairMatcher.pairings[text[i]]) { | ||
// We found an opening char, now move to the matching closing char | ||
const openPosition = new Position(position.line, i); | ||
return PairMatcher.nextPairedChar(openPosition, text[i], true) || failure; | ||
return PairMatcher.nextPairedChar(openPosition, text[i]) || failure; | ||
} | ||
} | ||
|
||
return failure; | ||
} | ||
|
||
return PairMatcher.nextPairedChar(position, charToMatch, true) || failure; | ||
return PairMatcher.nextPairedChar(position, charToMatch) || failure; | ||
} | ||
|
||
public async execActionForOperator( | ||
|
@@ -1407,13 +1407,27 @@ export abstract class MoveInsideCharacter extends BaseMovement { | |
protected includeSurrounding = false; | ||
|
||
public async execAction(position: Position, vimState: VimState): Promise<IMovement> { | ||
const failure = { start: position, stop: position, failed: true }; | ||
const text = TextEditor.getLineAt(position).text; | ||
const closingChar = PairMatcher.pairings[this.charToMatch].match; | ||
const closedMatch = text[position.character] === closingChar; | ||
|
||
let cursorStartPos = new Position( | ||
vimState.cursorStartPosition.line, | ||
vimState.cursorStartPosition.character | ||
); | ||
// maintain current selection on failure | ||
const failure = { start: cursorStartPos, stop: position, failed: true }; | ||
if (!this.includeSurrounding) { | ||
const adjacentPosLeft = cursorStartPos.getLeftThroughLineBreaks(); | ||
const adjacentPosRight = position.getRightThroughLineBreaks(); | ||
const adjacentCharLeft = TextEditor.getCharAt(adjacentPosLeft); | ||
const adjacentCharRight = TextEditor.getCharAt(adjacentPosRight); | ||
if (adjacentCharLeft === this.charToMatch && adjacentCharRight === closingChar) { | ||
cursorStartPos = adjacentPosLeft; | ||
vimState.cursorStartPosition = adjacentPosLeft; | ||
position = adjacentPosRight; | ||
vimState.cursorPosition = adjacentPosRight; | ||
} | ||
} | ||
// First, search backwards for the opening character of the sequence | ||
let startPos = PairMatcher.nextPairedChar(position, closingChar, closedMatch); | ||
let startPos = PairMatcher.nextPairedChar(cursorStartPos, closingChar, vimState); | ||
if (startPos === undefined) { | ||
return failure; | ||
} | ||
|
@@ -1426,7 +1440,8 @@ export abstract class MoveInsideCharacter extends BaseMovement { | |
startPlusOne = new Position(startPos.line, startPos.character + 1); | ||
} | ||
|
||
let endPos = PairMatcher.nextPairedChar(startPlusOne, this.charToMatch, false); | ||
let endPos = PairMatcher.nextPairedChar(position, this.charToMatch, vimState); | ||
|
||
if (endPos === undefined) { | ||
return failure; | ||
} | ||
|
@@ -1452,13 +1467,72 @@ export abstract class MoveInsideCharacter extends BaseMovement { | |
vimState.recordedState.operatorPositionDiff = startPos.subtract(position); | ||
} | ||
|
||
vimState.cursorStartPosition = startPos; | ||
return { | ||
start: startPos, | ||
stop: endPos, | ||
diff: new PositionDiff(0, startPos === position ? 1 : 0), | ||
}; | ||
} | ||
|
||
public async execActionWithCount( | ||
position: Position, | ||
vimState: VimState, | ||
count: number | ||
): Promise<Position | IMovement> { | ||
let recordedState = vimState.recordedState; | ||
let result: Position | IMovement = new Position(0, 0); // bogus init to satisfy typechecker | ||
|
||
if (count < 1) { | ||
count = 1; | ||
} else if (count > 99999) { | ||
count = 99999; | ||
} | ||
|
||
for (let i = 0; i < count; i++) { | ||
const lastIteration = i === count - 1; | ||
const temporaryResult = | ||
recordedState.operator && lastIteration | ||
? await this.execActionForOperator(position, vimState) | ||
: await this.execAction(position, vimState); | ||
|
||
// return failure result when repeat prefixing | ||
if (isIMovement(temporaryResult)) { | ||
if ((<IMovement>temporaryResult).failed) { | ||
return temporaryResult; | ||
} | ||
} | ||
|
||
if (temporaryResult instanceof Position) { | ||
result = temporaryResult; | ||
position = temporaryResult; | ||
} else if (isIMovement(temporaryResult)) { | ||
if (result instanceof Position) { | ||
result = { | ||
start: new Position(0, 0), | ||
stop: new Position(0, 0), | ||
failed: false, | ||
}; | ||
} | ||
|
||
result.failed = result.failed || temporaryResult.failed; | ||
|
||
// update start every iteration as selection expands | ||
(result as IMovement).start = temporaryResult.start; | ||
|
||
if (lastIteration) { | ||
(result as IMovement).stop = temporaryResult.stop; | ||
} else { | ||
position = temporaryResult.stop; | ||
} | ||
|
||
result.registerMode = temporaryResult.registerMode; | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
public async execActionForOperator( | ||
position: Position, | ||
vimState: VimState | ||
|
@@ -1707,11 +1781,7 @@ class MoveToUnclosedRoundBracketBackward extends MoveToMatchingBracket { | |
public async execAction(position: Position, vimState: VimState): Promise<Position | IMovement> { | ||
const failure = { start: position, stop: position, failed: true }; | ||
const charToMatch = ')'; | ||
const result = PairMatcher.nextPairedChar( | ||
position.getLeftThroughLineBreaks(), | ||
charToMatch, | ||
false | ||
); | ||
const result = PairMatcher.nextPairedChar(position, charToMatch); | ||
|
||
if (!result) { | ||
return failure; | ||
|
@@ -1727,11 +1797,7 @@ class MoveToUnclosedRoundBracketForward extends MoveToMatchingBracket { | |
public async execAction(position: Position, vimState: VimState): Promise<Position | IMovement> { | ||
const failure = { start: position, stop: position, failed: true }; | ||
const charToMatch = '('; | ||
const result = PairMatcher.nextPairedChar( | ||
position.getRightThroughLineBreaks(), | ||
charToMatch, | ||
false | ||
); | ||
const result = PairMatcher.nextPairedChar(position, charToMatch); | ||
|
||
if (!result) { | ||
return failure; | ||
|
@@ -1756,11 +1822,7 @@ class MoveToUnclosedCurlyBracketBackward extends MoveToMatchingBracket { | |
public async execAction(position: Position, vimState: VimState): Promise<Position | IMovement> { | ||
const failure = { start: position, stop: position, failed: true }; | ||
const charToMatch = '}'; | ||
const result = PairMatcher.nextPairedChar( | ||
position.getLeftThroughLineBreaks(), | ||
charToMatch, | ||
false | ||
); | ||
const result = PairMatcher.nextPairedChar(position, charToMatch); | ||
|
||
if (!result) { | ||
return failure; | ||
|
@@ -1776,11 +1838,7 @@ class MoveToUnclosedCurlyBracketForward extends MoveToMatchingBracket { | |
public async execAction(position: Position, vimState: VimState): Promise<Position | IMovement> { | ||
const failure = { start: position, stop: position, failed: true }; | ||
const charToMatch = '{'; | ||
const result = PairMatcher.nextPairedChar( | ||
position.getRightThroughLineBreaks(), | ||
charToMatch, | ||
false | ||
); | ||
const result = PairMatcher.nextPairedChar(position, charToMatch); | ||
|
||
if (!result) { | ||
return failure; | ||
|
@@ -1805,20 +1863,24 @@ abstract class MoveTagMatch extends BaseMovement { | |
public async execAction(position: Position, vimState: VimState): Promise<IMovement> { | ||
const editorText = TextEditor.getText(); | ||
const offset = TextEditor.getOffsetAt(position); | ||
const tagMatcher = new TagMatcher(editorText, offset); | ||
const tagMatcher = new TagMatcher(editorText, offset, vimState); | ||
const cursorStartPos = new Position( | ||
vimState.cursorStartPosition.line, | ||
vimState.cursorStartPosition.character | ||
); | ||
const start = tagMatcher.findOpening(this.includeTag); | ||
const end = tagMatcher.findClosing(this.includeTag); | ||
|
||
if (start === undefined || end === undefined) { | ||
return { | ||
start: position, | ||
start: cursorStartPos, | ||
stop: position, | ||
failed: true, | ||
}; | ||
} | ||
|
||
let startPosition = start ? TextEditor.getPositionAt(start) : position; | ||
let endPosition = end ? TextEditor.getPositionAt(end) : position; | ||
let startPosition = start >= 0 ? TextEditor.getPositionAt(start) : cursorStartPos; | ||
let endPosition = end >= 0 ? TextEditor.getPositionAt(end) : position; | ||
|
||
if (position.isAfter(endPosition)) { | ||
vimState.recordedState.transformations.push({ | ||
|
@@ -1841,12 +1903,71 @@ abstract class MoveTagMatch extends BaseMovement { | |
failed: true, | ||
}; | ||
} | ||
vimState.cursorStartPosition = startPosition; | ||
return { | ||
start: startPosition, | ||
stop: endPosition.getLeftThroughLineBreaks(true), | ||
}; | ||
} | ||
|
||
public async execActionWithCount( | ||
position: Position, | ||
vimState: VimState, | ||
count: number | ||
): Promise<Position | IMovement> { | ||
let recordedState = vimState.recordedState; | ||
let result: Position | IMovement = new Position(0, 0); // bogus init to satisfy typechecker | ||
|
||
if (count < 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we've had this pattern but we should really move this sort of check somewhere outside this function (on a different PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking of abstracting more of the code in this function to private methods. This would probably allow us to add a couple of classes between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were you thinking of doing that in this PR or sometime later? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try to cleanup some of the changes in this PR and sprinkle in some more comments. If you have any other concerns or suggestions I'd like to organize what I need to do so I can plan some time to get to it. I think the refactor I proposed fits well with this PR since I ended up duplicating |
||
count = 1; | ||
} else if (count > 99999) { | ||
count = 99999; | ||
} | ||
|
||
for (let i = 0; i < count; i++) { | ||
const lastIteration = i === count - 1; | ||
const temporaryResult = | ||
recordedState.operator && lastIteration | ||
? await this.execActionForOperator(position, vimState) | ||
: await this.execAction(position, vimState); | ||
|
||
// return failure result when repeat prefixing | ||
if (isIMovement(temporaryResult)) { | ||
if ((<IMovement>temporaryResult).failed) { | ||
return temporaryResult; | ||
} | ||
} | ||
|
||
if (temporaryResult instanceof Position) { | ||
result = temporaryResult; | ||
position = temporaryResult; | ||
} else if (isIMovement(temporaryResult)) { | ||
if (result instanceof Position) { | ||
result = { | ||
start: new Position(0, 0), | ||
stop: new Position(0, 0), | ||
failed: false, | ||
}; | ||
} | ||
|
||
result.failed = result.failed || temporaryResult.failed; | ||
|
||
// update start every iteration as selection expands | ||
(result as IMovement).start = temporaryResult.start; | ||
|
||
if (lastIteration) { | ||
(result as IMovement).stop = temporaryResult.stop; | ||
} else { | ||
position = temporaryResult.stop; | ||
} | ||
|
||
result.registerMode = temporaryResult.registerMode; | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
public async execActionForOperator( | ||
position: Position, | ||
vimState: VimState | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.