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

Fix indentation lines (#6134) #6136

Merged
merged 1 commit into from
Mar 1, 2023
Merged

Fix indentation lines (#6134) #6136

merged 1 commit into from
Mar 1, 2023

Conversation

NomisIV
Copy link
Contributor

@NomisIV NomisIV commented Feb 28, 2023

Closes #6134.

This is my first contribution and I'm honestly surprised at how quick I was able to implement the fix.
Criticism is welcome - I don't know what I'm doing xD

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.

Good catch, this particular mixup of tab width and indent width was missed in #5918.

helix-term/src/ui/document.rs Outdated Show resolved Hide resolved
helix-term/src/ui/document.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2023
@NomisIV
Copy link
Contributor Author

NomisIV commented Feb 28, 2023

Btw, there's something fishy with where the start of the indentation lines is calculated:

image

As you can see, they are drawn over the line numbers and the line gutter...

I can try to fix this in another PR, when I understand how starting_indent works

EDIT:
It seems that the issue is the implicit flooring of the number with the integer division in starting_indent. I tried casting the division to a float and ceiling it instead and then casting it back to usize, and now it works, but it looks kind of ugly though...

EDIT2:
This looks better

            starting_indent: (col_offset + indent_width - 1) / indent_width
                + editor_config.indent_guides.skip_levels as usize,

I could just push this change in this PR too to reduce the overhead of another PR

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 28, 2023

Ah yeah the upwards rounding is the correct fix. This bug seems to exist eversibce indent guides were added but the reason it wasnt visible.before is that gutters were drawn after the text in the past so the additional indent guides were overwritten but since #5420 gutters are drawn as a gemric decrostions which get rendered on the fly just before the text on the same line is drawn SL.the existing bug is moticable now.

It would be great if you fixed the rounding inside this commit. However a small.nit: foe upwards rounding integer division col_offset / indent_width + (col_offset % indent_width != 0) as u16 prevents overflows and is faster as module and div are usually computed by the same instruction

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.

LGTM, this cleans up some loose ends from #5420 and #5918

@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 28, 2023
@archseer archseer merged commit c082ef2 into helix-editor:master Mar 1, 2023
estin pushed a commit to estin/helix that referenced this pull request Mar 4, 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
C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indent guides don't follow indent-style
3 participants