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.

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

Add basic indentation handling as a fallback for treesitter queries #1341

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

Triton171
Copy link
Contributor

Previously, when there was no treesitter grammar and indents.toml for a language, new lines had no indentation. This changes it, so that as a fallback, the indentation of the current line is used.

While changing this, I also noticed a few issues with the treesitter indentation handling, which I also fixed where possible:
There was an off-by-one error in the position used for the indent query, which caused a line to be indented too much:

fn test() { 0 |}

When pressing Enter with this cursor position, the closing brace was indented too much:

fn test() { 0 
    |}

There was also an issue where the result of the indent query was influenced by the presence/absence of nodes that are not specified in indents.toml. For example, these 2 cursor positions resulted in different indentations when pressing Enter:

let a = (0,| 1);

and

let a = (0, |1);

In this case, this happened because if the cursor is on a whitespace, the smallest treesitter node containing it is the tuple expression, otherwise there is a smaller node (the "," or the expression node).

I fixed both these issues and it now works correctly in all cases i tested (I've mainly tested it on rust and c++ code).
If anyone wants to try it out, that would be great to make sure that I haven't missed any edge cases.

…ion rules (always use the indent of the current line for a new line).

Fix several bugs in the treesitter indentation calculation.
} else {
// TODO: heuristics for non-tree sitter grammars
0
None
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking you could just call indent_level_for_line here? Hence the TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current version we can't really do it here, because the indentation in this case also depends on the (previous) cursor position: The indentation of the new line is the indentation of the current line (which could be above or below depending on whether you us o or O). Because it doesn't (just) depend on the position of the newline, including it in suggested_indent_for_pos doesn't really make sense.

I'm not sure if this indentation behavior is what we want, we could also simply copy the indentation from the line above. Then we could also include it in suggested_indent_for_pos and the behavior would be the same for o and Enter in insert mode. I'm not super happy with the current solution either so let me know if you have any other ideas.

text.push_str(doc.line_ending.as_str());
text.push_str(&indent);
pos + offs + text.chars().count()
};

// TODO: range replace or extend
Copy link
Member

Choose a reason for hiding this comment

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

This is an old TODO and range.put_cursor now exists with this functionality, can drop the TODO

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Tested today, seems to work fine 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants