Fix non-sticky outdent action for OnEnter rules

[lionel-]

Addresses #1727

@DavisVaughan found that the indentation issue described in #1727 is caused by a bug that causes outdent actions of on-enter rules to be non-sticky. He also discovered this long-open PR that fixes the issues: microsoft/vscode#136593

I've brought attention to this PR to the VS Code dev who is currently working on indentation, with a reprex and motivation: microsoft/vscode#136593 (comment). So hopefully this will be fixed upstream.

In the meantime, this PR includes the fix into Positron. The alternative would be to use a format-on-type correction as we plan to do for other indent issues, but this approach doesn't seem like a good fit for this issue.

[DavisVaughan]

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 ---

[lionel-]

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.

[DavisVaughan]

Fine by me!

[DavisVaughan]

This afterEnterText also felt weird to me. I thought it should just be "".

The way I see it is:

• beforeEnterText is everything on the preceding unignored line
• afterEnterText is then "", because there is nothing else on the line.

I changed it to "" in my rewrite

[lionel-]

IIUC the intent is to reproduce the OnEnter matching that initially splitted the line from which we are inheriting indentation. When you type Enter, OnEnter rules are processed by matching the text before cursor to the beforeText regexp, and the text after cursor to afterText (usually but not necessarily empty). If there is a match, a newline and indentation are inserted. But here we are trying to reproduce the matching that occurred in the past, with the line already split. So that's why we are passing that whole line (the previous beforeText) to the current whole line (the previous afterText) to the enter action matcher, ignoring the whitespace in between. Does this make sense?

[DavisVaughan]

If we have this example, with the cursor at |

df %>%
  mutate(x = 1) %>%
  mutate(y = 2)

|foo

Then when I hit Enter at the |, I was expecting it to replay the past enter behavior by:

• Effectively moving the cursor to mutate(y = 2)|
• Setting beforeEnterText as mutate(y = 2)
• Setting afterEnterText as ''
• Setting previousLineText as mutate(x = 1) %>%

i.e. this recreates the scenario of being at the end of mutate(y = 2) and hitting Enter, and in my head that doesn't have anything to do with foo at all. In other words, if I'm replaying the past then I should not be looking at anything in the present to figure out what happened.

In practice, I'm not sure this will affect very many scenarios (certainly not ours, since we don't use afterEnterText) but it seemed off.

Do you interpret that differently?

[lionel-]

yep this makes sense.

I think the current implementation also makes sense in a different way. It assumes that no changes to the code has been done besides hitting enter multiple times. If that's the case, that means the starting state was

df %>%
  mutate(x = 1) %>%
  mutate(y = 2)|foo

Then Enter is hit multiple times. And in that case it's more consistent to apply exactly the same OnEnter rule for outdent as you keep hitting Enter, otherwise subsequent Enters might behave differently.

I think both approaches are valid and I agree that it should not matter most of the times. I could not find any "outdent" rules that inspect afterText inside the Code/Positron repository. So personally I would just follow the upstream PR unless it had an unambiguous flaw, for simplicity.

[DavisVaughan]

Ok, what you are saying about "hitting enter multiple times" also makes sense.

Agree about following the upstream PR for consistency and simplicity, let's do that!

[lionel-]

@DavisVaughan I've fixed the issue you noticed about typing Enter with the cursor on the last line. We're passing a getLineCount() methods with the virtual document model so that we can check that we are in bounds to retrieve the afterText. The call sites are often working with a range, so I've mapped the line count to be either the range end or the text model's line count, whichever is smallest.

Does that look good? If so I'll suggest this change to the upstream PR as well.

Unfortunately for some reason I couldn't reproduce the faulty behaviour in the snapshot tests, so I've not added one.

@jmcphers If you get a chance you might also want to take a look at this since this is modifying VS Code and might pop up in merge conflicts.

[DavisVaughan]

@lionel- when I dug into this I understood lineNumber to actually be the next line, so in the case of:

df %>%
  mutate(x = 1) %>%
  mutate(y = 2)

|foo
bar <- this is what lineNumber points to

so I thought we just wanted lineNumber - 1. But then again IIUC the getLineContent() method of the virtualModel object is special cased to return beforeEnterText if you are on the |foo line from above, so it would just return "" and that seems odd too.

[lionel-]

yes it's the next line fed as afterText that follows the logic I described in #2619 (comment). So if there is none because the document is too short we use ''. Does that make sense?

[lionel-]

We went over this in a video chat and it seems I was confused. We decided to just go with empty afterText to fix the end-of-document bug and not worry about the rest. I'll ping the upstream PR about this bug to make sure it doesn't go unnoticed on their end, we'll see what they come up with.