Skip to content
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.

Sign up for GitHub

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 non-sticky outdent action for OnEnter rules #2619

Merged
merged 6 commits into from
Apr 5, 2024
Merged

Fix non-sticky outdent action for OnEnter rules #2619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a793dcb
Fix non-sticky outdent action for `OnEnter` rules
ssigwart Apr 3, 2024
28ef97f
Pass line count and use it to check bounds
lionel- Apr 5, 2024
1d6b109
Revert "Pass line count and use it to check bounds"
lionel- Apr 5, 2024
d307157
Use empty `afterText` after all
lionel- Apr 5, 2024
74e78f2
Fix merge conflict
lionel- Apr 5, 2024
79066ff
Update test comment now it's failing again (as expected)
lionel- Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions extensions/positron-r/src/test/snapshots/indentation-cases.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ data |>

# ---
# Continuing a one-liner pipeline (after a comment line)
# FIXME (once we merge the sticky dedent PR)
# FIXME
data |>
fn1() |>
# foo
Expand Down Expand Up @@ -113,14 +113,12 @@ data |>
# ---
# Stickiness of dedent after pipeline
# https://github.com/posit-dev/positron/issues/1727
# FIXME
data |>
fn()
"<>"

# ---
# Stickiness of dedent after pipeline (trailing comment)
# FIXME
data |>
fn()
"<>" # foo
Expand Down
10 changes: 4 additions & 6 deletions extensions/positron-r/src/test/snapshots/indentation-snapshots.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ data |>

# ---
# Continuing a one-liner pipeline (after a comment line)
# FIXME (once we merge the sticky dedent PR)
# FIXME
data |>
fn1() |>
# foo
Expand All @@ -108,7 +108,7 @@ data |>
fn1() |>
# foo

"<>"
"<>"

# ---
# Continuing a one-liner pipeline (longer pipeline)
Expand Down Expand Up @@ -198,7 +198,6 @@ data |>
# ---
# Stickiness of dedent after pipeline
# https://github.com/posit-dev/positron/issues/1727
# FIXME
data |>
fn()
"<>"
Expand All @@ -207,11 +206,10 @@ data |>
data |>
fn()

"<>"
"<>"

# ---
# Stickiness of dedent after pipeline (trailing comment)
# FIXME
data |>
fn()
"<>" # foo
Expand All @@ -220,7 +218,7 @@ data |>
data |>
fn()

"<>" # foo
"<>"# foo

# ---
# Indent after function in call
Expand Down
44 changes: 39 additions & 5 deletions src/vs/editor/common/languages/autoIndent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ export function getInheritIndentForLine(
model: IVirtualModel,
lineNumber: number,
honorIntentialIndent: boolean = true,
languageConfigurationService: ILanguageConfigurationService
// --- Start Positron ---
languageConfigurationService: ILanguageConfigurationService,
indentConverter: IIndentConverter | undefined = undefined
// --- End Positron ---
): { indentation: string; action: IndentAction | null; line?: number } | null {
if (autoIndent < EditorAutoIndentStrategy.Full) {
return null;
Expand Down Expand Up @@ -165,8 +168,33 @@ export function getInheritIndentForLine(
}

if (honorIntentialIndent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW when I was playing with this I left everything in this if statement the same and instead put this chunk of code right above this if statement. I'm not sure if honorIntentionalIndent is supposed to matter here

		// --- Start Positron ---
		// Check for onEnter rules that should decrease the indent by looking at the rule
		// used on the last unignored line.
		// Adapted from https://github.com/microsoft/vscode/pull/136593/.
		if (indentConverter) {
			const richEditSupport = languageConfigurationService.getLanguageConfiguration(model.tokenization.getLanguageId());
			if (richEditSupport) {
				const previousLineText = precedingUnIgnoredLine < 1 ? '' : model.getLineContent(precedingUnIgnoredLine - 1);
				const beforeEnterText = precedingUnIgnoredLineContent;
				const afterEnterText = '';
				const enterResult = richEditSupport.onEnter(autoIndent, previousLineText, beforeEnterText, afterEnterText);
				if (enterResult) {
					const precedingUnIgnoredLineIndentation = strings.getLeadingWhitespace(precedingUnIgnoredLineContent);
					if (enterResult.indentAction === IndentAction.Outdent) {
						return {
							indentation: indentConverter.unshiftIndent(precedingUnIgnoredLineIndentation),
							action: null,
							line: precedingUnIgnoredLine
						};
					} else if (enterResult.removeText && precedingUnIgnoredLineIndentation.length >= enterResult.removeText) {
						return {
							indentation: precedingUnIgnoredLineIndentation.substring(0, precedingUnIgnoredLineIndentation.length - enterResult.removeText - 1),
							action: null,
							line: precedingUnIgnoredLine
						};
					}
				}
			}
		}
		// --- End Positron ---

Copy link
Contributor Author

@lionel- lionel- Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That seems plausible but when I follow the code paths where honorIntentionalIndent would be false, I can't really produce any unintended behaviour. Since we're not sure, and if we are not otherwise changing the PR (if you agree with my notes on the other comment), I would keep this as is to make it easier to track the upstream PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me!

// --- Start Positron ---
// Patch from https://github.com/microsoft/vscode/pull/136593
// For https://github.com/posit-dev/positron/issues/1727

let indentation = strings.getLeadingWhitespace(model.getLineContent(precedingUnIgnoredLine));
// Check for onEnter rules that should decrease the indent
if (indentConverter) {
const richEditSupport = languageConfigurationService.getLanguageConfiguration(model.tokenization.getLanguageId());
if (richEditSupport) {
const previousLineText = precedingUnIgnoredLine < 1 ? '' : model.getLineContent(precedingUnIgnoredLine - 1);
const afterEnterText = '';
const enterResult = richEditSupport.onEnter(autoIndent, previousLineText, precedingUnIgnoredLineContent, afterEnterText);
if (enterResult) {
if (enterResult.indentAction === IndentAction.Outdent) {
indentation = indentConverter.unshiftIndent(indentation);
} else if (enterResult.removeText && indentation.length >= enterResult.removeText) {
indentation = indentation.substring(0, indentation.length - enterResult.removeText - 1);
}
}
}
}
// --- End Positron ---

return {
indentation: strings.getLeadingWhitespace(model.getLineContent(precedingUnIgnoredLine)),
// --- Start Positron ---
indentation,
// --- End Positron ---
action: null,
line: precedingUnIgnoredLine
};
Expand Down Expand Up @@ -235,7 +263,9 @@ export function getGoodIndentForLine(
return null;
}

const indent = getInheritIndentForLine(autoIndent, virtualModel, lineNumber, undefined, languageConfigurationService);
// --- Start Positron ---
const indent = getInheritIndentForLine(autoIndent, virtualModel, lineNumber, undefined, languageConfigurationService, indentConverter);
// --- End Positron ---
const lineContent = virtualModel.getLineContent(lineNumber);

if (indent) {
Expand Down Expand Up @@ -361,7 +391,9 @@ export function getIndentForEnter(
};

const currentLineIndent = strings.getLeadingWhitespace(lineTokens.getLineContent());
const afterEnterAction = getInheritIndentForLine(autoIndent, virtualModel, range.startLineNumber + 1, undefined, languageConfigurationService);
// --- Start Positron ---
const afterEnterAction = getInheritIndentForLine(autoIndent, virtualModel, range.startLineNumber + 1, undefined, languageConfigurationService, indentConverter);
// --- End Positron ---
if (!afterEnterAction) {
const beforeEnter = embeddedLanguage ? currentLineIndent : beforeEnterIndent;
return {
Expand Down Expand Up @@ -430,7 +462,9 @@ export function getIndentActionForType(
if (!indentRulesSupport.shouldDecrease(beforeTypeText + afterTypeText) && indentRulesSupport.shouldDecrease(beforeTypeText + ch + afterTypeText)) {
// after typing `ch`, the content matches decreaseIndentPattern, we should adjust the indent to a good manner.
// 1. Get inherited indent action
const r = getInheritIndentForLine(autoIndent, model, range.startLineNumber, false, languageConfigurationService);
// --- Start Positron ---
const r = getInheritIndentForLine(autoIndent, model, range.startLineNumber, false, languageConfigurationService, indentConverter);
// --- End Positron ---
if (!r) {
return null;
}
Expand Down
Loading