Skip to content

Unify usage of indent_style - #5918

Merged
archseer merged 1 commit into
helix-editor:masterfrom
Triton171:unify-indent-style
Feb 16, 2023
Merged

Unify usage of indent_style#5918
archseer merged 1 commit into
helix-editor:masterfrom
Triton171:unify-indent-style

Conversation

@Triton171

Copy link
Copy Markdown
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.

@pascalkuthe pascalkuthe left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@pascalkuthe pascalkuthe left a comment

Copy link
Copy Markdown
Member

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()
}

@pascalkuthe pascalkuthe Feb 11, 2023

Copy link
Copy Markdown
Member

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

@pascalkuthe pascalkuthe left a comment

Copy link
Copy Markdown
Member

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