Skip to content

Caret position in fourslash#16878

Merged
aozgaa merged 7 commits into
microsoft:masterfrom
aozgaa:caretPositionInFourslash
Jul 6, 2017
Merged

Caret position in fourslash#16878
aozgaa merged 7 commits into
microsoft:masterfrom
aozgaa:caretPositionInFourslash

Conversation

@aozgaa
Copy link
Copy Markdown
Contributor

@aozgaa aozgaa commented Jul 1, 2017

Fixes #16877.

@aozgaa aozgaa requested review from a user, RyanCavanaugh and rbuckton July 1, 2017 02:58
}

verifyIndentationAfterNewLine("1", 4);
// TODO (arozga): fix this.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is fixed now?

Copy link
Copy Markdown
Contributor Author

@aozgaa aozgaa Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This problem actually already existed, but is only exposed by adjusting the caret correctly. But actually fixing indenting behaviors is outside the scope of this PR. When this change gets accepted, I'll make a new issue for this.

Comment thread src/harness/fourslash.ts
@@ -1740,31 +1722,25 @@ namespace FourSlash {
}
Copy link
Copy Markdown

@ghost ghost Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why above in if (this.enableFormatting) we update offset but not currentCaretPosition?
Also, is fixCaretPosition still necessary if we update it correctly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.applyEdits returns the number of characters added/removed in the file, but this is different from the adjustment to the caret position (which is the number of characters added/removed in the file before the caret).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixCaretPosition appears to be unnecessary now.

Comment thread src/harness/fourslash.ts Outdated
// of the incremental offset from each edit to the next. We assume these edit ranges don't overlap

edits = edits.sort((a, b) => a.span.start - b.span.start);
for (let i = 0; i < edits.length - 1; ++i) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: i++ is more conventional

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

Comment thread src/harness/fourslash.ts Outdated
* @returns The number of characters added to the file as a result of the edits.
* May be negative.
*/
private applyEdits(fileName: string, edits: ts.TextChange[], isFormattingEdit = false): number {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isFormattingEdit parameter is omitted only once; consider making it non-optional. Same for the parameters in printCurrentFileState.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

@aozgaa aozgaa merged commit 53a5abc into microsoft:master Jul 6, 2017
@aozgaa aozgaa deleted the caretPositionInFourslash branch July 6, 2017 18:43
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants