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

Unify usage of indent_style #5918

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

Triton171
Copy link
Contributor

Previously, we used sometimes doc.indent_style and sometimes doc.language_config.indent. This caused some issues if they differ (e.g. if a non-default indent style is auto-detected in a document or manually set). For example, in a markdown file, setting :indent-style 8 and then continuously pressing enter (starting on an indented line) increases the indent each time. This happens because the indent_style used to detect the indentation level of an existing line is different from the indent_style used to convert an indentation level to a string. The same issue occurs in any language without indent queries.

With these changes, language_config.indent is only used to initialize doc.indent_style and afterwards ignored. Resolves #2236 and #3406.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

If I understand this change correctly that means that the number of spaces in the indent unit now always corresponds to the tabwidth.

In other words:

  • If my document contains both spaces and tabs
  • I set the indent width to 4
  • Then all tabs are rendered with a width of 4

This does not correspond to the current config whichs allows setting tab-width separately from the unit field.

So for example this change would break using 4 spaces for indentation and a tab-width of two:

indent-style = { unit = "    ", tab-width=2 }

Is unifying these two really necessary to fix the underling issue here?

From my understanding, the correct fix would be to have to separate parameters for indent_level_for_line
the tab-width which is used when \t is encountered and the indent-width winch is used for calculating the indent level

@pascalkuthe pascalkuthe added E-medium Call for participation: Experience needed to fix: Medium / intermediate A-core Area: Helix core improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. C-bug Category: This is a bug labels Feb 11, 2023
@Triton171
Copy link
Contributor Author

Thanks for the super-fast review! You're right, I hadn't really thought about the distinction between tab_width (the width the tab character is rendered at) and indent_width (the width of an indentation level). It appears that I'm not the only one, as a significant percentage of the usages of tab_width in the current master should actually be indent_width. I've now made the difference a bit clearer in the code and fixed these bugs.

I also removed the doc.tab_width() function and replaced their usages by doc.indent_style.tab_width. This hopefully makes it more obvious that there are 2 different widths to consider when computing indentation.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Yeah it's definitely a bit subtle 👍 People usually stick to a single indent style they tend to o think about indent_width and tab_width as the same thing.

The changes to the autodetection now LGTM 👍

However this PR regresses some things like config reloading. I think a simpler fix is possible by simply adding a new indent_width function instead of actually changing IndentStyle. I added suggestion below.

@@ -1122,13 +1130,6 @@ impl Document {
self.syntax.as_ref()
}

Copy link
Member

@pascalkuthe pascalkuthe Feb 11, 2023

Choose a reason for hiding this comment

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

With this config removed tab-width is not updated on :config-reload anymore. I actually think that we should keep this function around and not make IndentStyle a struct at all.

Rather we should add a second function to the document to compute the indent_width and then use that in the right places.

Suggested change
/// Tab size in columns.
pub fn tab_width(&self) -> usize {
self.language_config()
.and_then(|config| config.indent.as_ref())
.map_or(4, |config| config.tab_width) // fallback to 4 columns
}
/// Indentation size in columns
pub fn indent_width(&self) -> usize {
match self.indent_style{
IndentStyle::Tab => self.tab_width(),
IndentStyle::Spaces(width) => width as usize
}
}

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this, LGTM now 👍

Ident width and tab width is a subtle but important distinction, great to see that cleared up

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 13, 2023
@Triton171
Copy link
Contributor Author

Thanks for all the feedback!

@archseer archseer merged commit a1a6d5f into helix-editor:master Feb 16, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Helix core improvements C-bug Category: This is a bug E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When editing HTML automatically inserted tabs are broken.
3 participants