Fix panic when line-number gutter's min-width is too large #7156
Conversation
|
@kirawi suggested using Edit: I see |
the-mikedavis
changed the title
…er locations in the code
| @@ -213,7 +213,7 @@ fn line_numbers_width(view: &View, doc: &Document) -> usize { | |||
| let draw_last = text.line_to_byte(last_line) < text.len_bytes(); | |||
| let last_drawn = if draw_last { last_line + 1 } else { last_line }; | |||
| let digits = count_digits(last_drawn); | |||
| let n_min = min( | |||
| let n_min = std::cmp::min( | |||
| view.gutters.line_numbers.min_width, | |||
| (view.area.width - 4) as usize, | |||
There was a problem hiding this comment.
After testing for a while, 6 is the magic number that works perfectly fine for that offset — I have no idea why.
The user also need to be alerted of illegal values.
Will you write a unit test for this?
There was a problem hiding this comment.
looking at this a bit we should be trying to stay in bounds of this debug_assert
Line 235 in 9238054
x value smaller then the buffer width. Where it actually panics from this assertion is when setting the style of one of the gutter items after the line-number. This is because that items x will be equal to the min_width + the width of the gutter items before the line-number. And as well we also need to take into account the amount of gutter items after the line-width item because even if one is within bounds the next might not be. So the calculation should be view.area.width - (width of gutter items before + width of gutter items after + how much space we want to leave for the editor). As of now the only gutter item with a variable width is the line-numbers I believe so we may be able to just count the items in the gutter, maybe someone else has a better idea though.
There was a problem hiding this comment.
You're suggesting that it's possible (in the future, perhaps) to resize/pad diff and diagnostics gutters?
And so, we have to account for them too. That's okay, because it probably explains why 6 works — it's their (diff, linenr, diag) total width + min editor space. Nevertheless, illegal values should cause a panic, no?
There was a problem hiding this comment.
I am not sure if this is really the right way to go about it. I don't have time to dig into it but if the panic really occurs because the gutter is wider than the terminal width then this is just a bandaid.
The terminal width can be very small/zero and in that case the static gutter elements would also draw past the terminal width. If that's the case then we would need to add some checks to only draw gutter elements when there is space for them.
There was a problem hiding this comment.
Its currently possible to change the items in the gutter by changing editor.gutters.layout(https://docs.helix-editor.com/configuration.html#editorgutters-section) in your config which changes the amount of room the line-numbers have to leave so we should be using that instead of a fixed number
There was a problem hiding this comment.
What I'm thinking is:
... get the overall 'width' of each gutter element, and the editing area
... set the new value of an element (linenr, in this case) within these boundaries/constraints you've suggested
... complain, if need be
Currently, there doesn't seem to be a way to get the 'width' of the other gutter elements; it seems to equal the cell size tho, except of course for linenr.
In any case, the solution will have to go beyond 'patching' min line number width.
There was a problem hiding this comment.
There is a width function for each gutter element. It should be a very simple if condition in editor.rs
There was a problem hiding this comment.
I see it. Having cooked everything to this point, let's wait and see what the author gets.
There was a problem hiding this comment.
Thanks for all the feedback everyone. This is only my second attempted PR for this project, and I was hoping just to grab a few quick ones to help out. Seems like this one might be a little more complex. I'm totally cool with sticking with it, but I do work and it's not longer a long weekend so if someone wants to get this done faster, by all means take it over from me. Also, @gyreas it seems like I may have taken this one from you, that was not my intention so I apologize for that, I was just looking for some quick ways to contribute and this one was marked as having instructions.
There was a problem hiding this comment.
It's not a problem. If anything, it's good that you're working on it. Also, no pressure. You can work on it when you're free, you have the stage set.
Best of luck 🤞
kirawi
added
C-bug
Attempted fix for #7154
Trying to fix this as my first contribution, so this may be an incredibly stupid implementation. There was a 3 being added to whatever was returned from this function, and I was incredibly unsure where that was coming from. It was not coming from whatever the current gutter min width was, so I thought I would at least put this out there and see if anyone could give me some feedback :D