Check for onEnterRules that decrease indentation when determining new line indent #136593

ssigwart · 2021-11-07T01:01:28Z

This PR fixes #136592. However, the single line if fix only works after #136577 is merged.

To test, you can use this:

./scripts/test.sh --glob **/cursor.test.js --grep 136592

… line indent

ssigwart · 2022-06-05T04:12:02Z

I rebased this to fix a merge conflict.

ssigwart · 2022-11-22T03:05:36Z

@hediet, this fixes the following PHP code. If you hit Enter on the last blank line, it will add a space in front of the new line.

/**
 * PHPDoc
 */

Similarly, if you have the following code and hit Enter on the blank line, it will add a tab in front of the new line. If #136577 is merged, this PR will solve this issue too.

if (1)
	1;

This issue is also a problem with other languages that allow single line conditionals. I think #136577 is much more impactful than this PR. This PR is also riskier than that one. I think it's the correct fix, but don't want to rush this one out if it needs more review time.

Thanks!

lionel- · 2024-03-29T17:55:51Z

@aiday-mar I've noticed that you self-assigned indentation-related opened issues. Could you also please take a look at this PR if you get a chance?

Currently dedention rules are non-sticky and we notice lots of cases like:

foo &&
    bar<CURSOR>

Where enter correctly dedents the first time:

foo &&
    bar
<CURSOR>

But on subsequent times enter suddenly reindents:

foo &&
    bar

    <CURSOR>

aiday-mar · 2024-04-02T07:14:59Z

Hi @lionel- thank you for pinging on this issue. For the example you gave I believe you could specify a new regex pattern in the indentNextLinePattern of the language configuration file, which will specify that after the line ending with bar indentation should be applied only on the next line. What language are you currently working with?

lionel- · 2024-04-03T10:16:46Z

Hi @aiday-mar, thanks for looking at this. I'm working on an improved language configuration for the R language. One important use case for R is pipelines of the form:

# ggplot2 pipeline
mtcars |>
  ggplot(aes(
    disp,
    drat
  )) +
  geom_point(
    aes(colour = cyl)
  )

# dplyr pipeline
starwars |>
  group_by(species) |>
  summarise(
    n = n(),
    mass = mean(mass, na.rm = TRUE)
  ) |>
  filter(
    n > 1,
    mass > 50
  )

We have not been able to find a way to correctly indent this kind of pipelines with components spread over multiple lines, which is partly why I opened #208985. However it should at least be possible to indent one-liners pipelines such as:

starwars |>
  group_by(species) |>
  summarise( n = n(), mass = mean(mass, na.rm = TRUE)) |>
  filter(n > 1, mass > 50)

Regarding your idea of using indentNextLinePattern, this will not work in our case. We can't use an increase-indent pattern for lines ending with an operator here, even one that only works on the next line as you suggested, because they produce this kind of staircase indentation:

increase-indent-rule.mov

To avoid this, we are using OnEnter rules that increase indentation when entering a chain of operators and decrease it when the chain is finished:

	"onEnterRules": [
		// A line ending with an operator, preceded by a blank line.
		{
			"previousLineText": "^\\s*(?:#|$)",
			"beforeText":		"^\\s*[^#]+[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$)",
			"action":			{ "indent": "indent" }
    },

		// A line ending with an operator, preceded by a line not ending with an operator.
		{
			"previousLineText": "^(?!.*[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$))[^#]+.*$",
			"beforeText":		"^\\s*[^#]+[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$)",
			"action":			{ "indent": "indent" }
		},

		// A line not ending with an operator, preceded by a line ending with an operator.
		{
			"previousLineText": "^\\s*[^#]+[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$)",
			"beforeText":		"^(?!.*[=\\+\\-\\*?/\\^><!&\\|~%\\$:@\\?]\\s*(?:#|$))[^#]+.*$",
			"action":			{ "indent": "outdent" }
		},
	]

With these rules we get the correct indenting behaviour:

onenter-rule.mov

Unfortunately the "outdent" action is not sticky, causing the following indentation bug on ulterior lines:

outdent-stickiness-bug.mov

We have verified that the PR fixes the non-stickiness of OnEnter outdent rules for our use case.

lionel- · 2024-04-04T08:13:46Z

Another case of this occurs in the javascript extension. Type Enter with this snippet:

while (true)
    foo;<CURSOR>

You end up here:

while (true)
    foo;
<CURSOR>

Type enter again and you get:

while (true)
    foo;

    <CURSOR>

aiday-mar · 2024-04-05T07:54:18Z

Hi @lionel- thanks for pinging me on this PR. Before merging this PR, I need to look more closely at the code in the feature area. As I have only recently been assigned this feature area, I will need some time to analyze the logic. I will look into this in due time.

lionel- · 2024-04-09T09:20:43Z

While testing this branch we noticed a problem in the way the afterText matcher is created here:

const afterEnterText = model.getLineContent(lineNumber);

With the rules described in #136593 (comment) and this branch, open a file and paste:

foo %>%
  bar()

Make sure the bar() call is the last line in the file and move the cursor to the end. Type Enter twice, only the first one works.

ssigwart · 2024-04-09T12:02:13Z

@aiday-mar, thanks for looking at this. Let me know if you want me to make any changes or rebase. This PR is so old. I didn't think it would ever get looked at.

@lionel-, I created this PR about 2.5 years ago, so I'm not 100% sure, but I think that the issue with hitting Enter a second time is a limitation of the way the indentation logic works, but I figured at least it solves the use case of hitting Enter once.

lionel- · 2024-04-09T13:03:11Z

@ssigwart

The problem is that no new line is entered after the first time. That's because the line I quoted tries to retrieve a line that doesn't exist in the text model and throws an error that is silently swallowed by the type() routine, interrupting the insertion of \n.

ssigwart · 2024-04-09T17:18:30Z

@lionel-, I was referring to #136593 (comment). I honestly didn't read through your comments about the R language because I thought they were directly towards @aiday-mar and I have never worked with R before.

If this PR does break something that used to work with R, but doesn't with this PR, I definitely should look into solving that. However, at the moment, I don't plan on spending more time on this. I spent a lot of time a few years ago researching issues and submitting PRs, but then I couldn't really get anyone from Microsoft to look at many. So I've given up on spending time on submitting VS Code PRs. I'll pick up work on this again if someone from Microsoft indicates that they plan on reviewing and potentially merging it.

vivodi · 2024-12-29T16:43:15Z

Is this still relevant?

ssigwart · 2024-12-29T18:22:22Z

Is this still relevant?

I think so. I just wasn't able to get enough traction from Microsoft to get it merged. I can pick it up again if someone from Microsoft indicates that it's worthwhile.

aiday-mar · 2025-05-07T09:59:44Z

So looking at the comments here it seems to me that the main issue is that when pressing Enter from an empty line, the next line is indented to have the same indent as the last non-empty line. Maybe instead of computing the onEnter result when honoring indentation, we could check if the current line is empty or consists only of whitespaces and if yes just insert the same indentation as for the current line. This would be a simpler fix and does not require fetching the onEnter rules. I am still debating however if this should be done and need to think some more.

ssigwart · 2025-05-07T12:23:56Z

Maybe instead of computing the onEnter result when honoring indentation, we could check if the current line is empty or consists only of whitespaces and if yes just insert the same indentation as for the current line.

That sounds reasonable to me.

aiday-mar

Hi @ssigwart thank you so much for the PR and apologies this took a long time to review. I have looked at the PR and thought some more about it. I think we could do the following:

First of all I think using the onEnter rules to compute inherited indent should not be default behavior right now. This is because we don't know what kind of unexpected consequences it could have. As a result I suggest we add this logic under a setting which will be false by default. Users will be able to opt into this behavior and file bugs, which we can use to improve this logic further. If we notice that generally this logic improves indentation, we can consider making the default true or entirely removing the setting.
I am wondering why the PR applies the onEnter rules only in the case honorInitialIndent is true. I am thinking if we apply the onEnter rules we should apply them in all the cases so we have consistent behavior?
Lastly I noticed that the PR only applies the onEnter result when it does an outdentation and it removes text. I think ideally if we go forwards with this, we should apply the onEnter rules in all cases, whether it does an indentation or an outdentation. This way the indentation will be predictable.

Thank you for taking the time to make this PR.