Allow truncation of table row text #6410
Conversation
Here we assume that spans with content that is 1 in length will not need to be truncated. This solves an issue with my previous commit that made the buffer picker truncate the "prefix" cells.
This removes the need for `render_cell_truncated`. We now check if we should truncate in `render_cell` which allows us to skip the additional checks in `Buffer::set_spans_truncated`.
mWalrus
changed the title
|
I had the thaught of soft wrapping text here (based on #5420, the doc formatted is very general) instead of truncating. At least for some pickers like diagnostics, it would be really nice to be able the entire item (diagnostic) instead of truncating. I am not sure if soft wrapping is always preferable to truncation but I personally am not a fan of the way paths were truncated because there was no way to horizontally scroll a picker item so it was sometimes very hard to tell certain paths apart. If this needs a fix anyway this might be a good opportunity. Although it's probably a bit more complex so maybe we try to get this PR in as a regression fix for the next release and look into soft wrapping later. |
There was a problem hiding this comment.
I actually like this a lot more than the old truncation (uses ... Instead of shortening ). It keeps more screen realeaste available the shortened path components were never really that useful to me. Other tools like fzf and skim di the same thing. The implementation needs some fixes tough
|
|
||
| for span in &spans.0 { | ||
| text.push_str(span.content.as_ref()); | ||
| span.styled_graphemes(span.style) |
There was a problem hiding this comment.
Performing grapheme segmentation here is both slow and incorrect. The idx used below is not a grapheme index but a byte index. Instead you should create an iterator that is transversed whenever the current style range is exhausted.
The current implementation can easily lead to crashes
There was a problem hiding this comment.
I figured this was a slow solution.
Could you elaborate a bit about what you mean and how I could go about correcting this? I'm pretty new to this kind of thing.
| buf.set_style(area, cell.style); | ||
| for (i, spans) in cell.content.lines.iter().enumerate() { | ||
| if i as u16 >= area.height { | ||
| break; | ||
| } | ||
| buf.set_spans(area.x, area.y + i as u16, spans, area.width); | ||
| if cell.content.width() > 1 && truncate { |
There was a problem hiding this comment.
I dont think the emptyness check is required here. Why did you add it?
In any case it should be using byte Len (or is_empty really) instead of grapheme width since it's O(1) and any non-empty string usually has a width (the cases where it doesn't can be negelagted IMO)
There was a problem hiding this comment.
I added this check to not truncate table cells that have a content length of 1. If we don't have this check and try to render the buffer picker you will see that the first two cells (the entry number and the you-are-here indicator) are being truncated as well since my current implementation doesn't distinguish between cells but applies to the entire row.
With that said, there are probably better ways of doing this, this was just the fix I could come up with without diving too deep into it.
|
Superseded by #6488 |
|
Thanks for working on this. Usually I would have given you more feedback here until this was ready but we wanted to get a fix in before the release (and there is not a lit.of.time left in March :D) so I just cherry picked your commit and implemented |
|
No worries! I'll look through the implementation for learning purposes :) |
This fixes the regression mentioned in #6346 where paths in the file picker were no longer being truncated.
We now allow all pickers who have
self.truncate_startset totrueto render truncated rows.Not sure if I'm going about this the right way, any feedback is appreciated :)