Barchart doesn't render labels in entire width

[joshka]

Description

The barchart widget doesn't render labels that are the full width (this affects both non-unicode and unicode)

To Reproduce

fn render_simple_barchart(area: Rect, buf: &mut Buffer) {
    let data = [
        ("Sat", 76),
        ("Sun", 69),
        ("Mon", 65),
        ("Tue", 67),
        ("Wed", 65),
        ("Thu", 69),
        ("Fri", 73),
    ];
    let data = data
        .into_iter()
        .map(|(label, value)| {
            Bar::default()
                .value(value)
                // cannot use this until the column width bug is fixed
                .text_value(format!("{}°", value))
                .label(label.into())
        })
        .collect_vec();
    let group = BarGroup::default().bars(&data);
    BarChart::default()
        .data(group)
        .bar_width(3)
        .bar_gap(1)
        .render(area, buf);
}

with the following line removed:

                .text_value(format!("{}°", value))

And the same occurs replacing that with

                .text_value(format!("{}x", value))

Expected behavior

I think this should probably render like the following (hacking a couple of things in the barchart implementation to make it work):

Environment

Mac/iterm/FiraCode(NF)/main/crossterm

Additional context

• Is this expected behavior for some historical reason?
• If this is expected, how should it be documented?
• What significance should unicode play here (e.g. the degree symbol has string len = 2 and unicode width = 1)? Is Buffer: unicode-width and emojis #75 relevant to the degree symbol?
• What is the history of this? Check for any guidance from commits, and comments on PRs/ issues in Ratatui and tui-rs.

[kdheepak]

I looked into the history for this a little bit.

tui-rs has used value.width() since the introduction of the barchart from 2016. And that is still how it is in the latest archived version of tui-rs too.

This PR in ratatui seems to have changed it from .width() to .len(): #288

See LHS:

https://github.com/ratatui-org/ratatui/pull/288/files#diff-7a977f85afc247cf4ed30e8453eb44fd67f06ba2c92115f157c58ebb23d0a3f6L199-L201

and RHS:

https://github.com/ratatui-org/ratatui/pull/288/files#diff-9ae27448be18568ad88363c462b5bc8f61aa2288b626db114d83cc60c322c705R61-R62

You'll have to expand the diff to see the lines. Because of the refactor, the side by side diff doesn't show the change; and maybe that's why this change wasn't discussed in more detail. There are a lot of comments on the PR but I don't think I see comments in the PR related to this particular change.

Maybe @karthago1 can comment on the change? I haven't tried using .width() to see if we had this particular issue before or not.

I do think in general we could start to use unicode-segmentation for all displayable "length" calculations. At the very least we should be using unicode-width. I'm guessing using .len() is just bound to give the unexpected results in a lot of cases when it comes unicode. This is related to #75.

[karthago1]

@kdheepak sorry for the bug. I created the PR #515 . it is still WIP. I need to adapt the other unit tests
I will do it tomorrow

[kdheepak]

No need to apologize! Pinged you to check if you had a good reason for the change!

For context, here's a test I just ran (cargo add unicode-width unicode-segmentation):

// main.rs
use unicode_width::UnicodeWidthStr;
use unicode_segmentation::UnicodeSegmentation;

fn main() {
  let s = "abc";
  dbg!(s);
  dbg!(s.len());
  dbg!(s.width());
  dbg!(s.graphemes(true).count());

  println!("");

  let s = "写作业";
  dbg!(s);
  dbg!(s.len());
  dbg!(s.width());
  dbg!(s.graphemes(true).count());

  println!("");

  let s = "🤦🏼‍♂️";
  dbg!(s);
  dbg!(s.len());
  dbg!(s.width());
  dbg!(s.graphemes(true).count());
}

$ rustc main.rs && ./main
[src/main.rs:6] s = "abc"
[src/main.rs:7] s.len() = 3
[src/main.rs:8] s.width() = 3
[src/main.rs:9] s.graphemes(true).count() = 3

[src/main.rs:14] s = "写作业"
[src/main.rs:15] s.len() = 9
[src/main.rs:16] s.width() = 6
[src/main.rs:17] s.graphemes(true).count() = 3

[src/main.rs:22] s = "🤦🏼‍♂️"
[src/main.rs:23] s.len() = 17
[src/main.rs:24] s.width() = 5
[src/main.rs:25] s.graphemes(true).count() = 1

[joshka]

Ok, that explains the unicode bit - changing back to .width() is reasonable because it's always been that way (even if it handles emoji oddly - that's to fix elsewhere)

Is there an answer to why < vs <= somewhere? Perhaps it was to make it ok to print bars without gaps?

[kdheepak]

For more reference, here's how vim / neovim prints the s = "🤦🏼‍♂️" in the terminal:

Moving the cursor through the emoji requires 5 presses of a left or right arrow key. Hitting backspace in the midde messes things up.

If we want something that works consistently the same in all terminals, maybe we should use unicode-width (I need to read up on this more).

If we want something that does the best it can on terminals that support it, maybe we should use unicode-segmentation.

Ideally, this would be a end user defined option (not the app developer) or a terminal detected solution. Maybe we can write a trait for trait TerminalDisplayWidth for String and use that everywhere in ratatui, and make the method in the trait return either unicode-width's .width() or unicode-segmentation's .graphemes(true).count() based on either an environment variable (or a terminfo value? or terminal emulator if we can detect it?).

[kdheepak]

Is there an answer to why < vs <= somewhere? Perhaps it was to make it ok to print bars without gaps?

That is unclear, and without pinging @fdehau who implemented it first in 2016 using <, I think we can only speculate.

[joshka]

One user friendIy approach to emoji is to specifically display them as 2 chars everywhere, but I think let's worry about exactly how unicode should be displayed in #75 and leave the concerns for this on whether it does.

That is unclear, and without pinging @fdehau who implemented it first in 2016 using <, I think we can only speculate.

Got it - I think based on this use case (of showing text labels the full width of a bar it's reasonable then to make this <= given there's no explanation of <.

[kdheepak]

One user friendIy approach to emoji is to specifically display them as 2 chars everywhere, but I think let's worry about exactly how unicode should be displayed in #75 and leave the concerns for this on whether it does.

I'm not following the 2 chars width suggestion you made but I agree let's discuss that more in #75!

I'm good with .width() + <= for now for addressing this issue.

[joshka]

I'm not following the 2 chars width suggestion you made

[kdheepak]

Somehow I went all this time without realizing emojis are 2 width wide!

[karthago1]

I just realized, that I didn't introduce this behavior 😸 .

Is there an answer to why < vs <= somewhere? Perhaps it was to make it ok to print bars without gaps?

The only thing I can think of, to be able to distinguish small values. example:

        ███
▃▃▃ ███ ███
B1  B2  B2

now you will see

        ███
123 200 400
B1  B2  B2

I don't know what is better, the first or second chart? (in this specific case maybe the first chart looks better)

do we still want #515 (of course after fixing the unit tests), or maybe we should close it, if the original behavior is fine?

I'm good with .width() + <= for now for addressing this issue.

why? because width() is less wrong? 😄

[karthago1]

why? because width() is less wrong?

to answer my self. 😸 (I just realized that unicodes are 2 character wide 🥲 ) except for emojis width() delivers correct values. I modified #515 to use width()

but I reverted the comparison back to <. because if we use <=the following example

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
a b c d e f g h i 

becomes

  1 2 3 4 5 6 7 8 
a b c d e f g h i 

I will think of a solution in another PR. something similar to: if the bar contains only one character height and value text width is equal bar width, then we shouldn't print the value

[kdheepak]

I just realized, that I didn't introduce this behavior 😸 .

My bad, did that PR I linked above not introduce this? Maybe I read the diffs incorrectly? Do you happen to know where it was introduced?

if the bar contains only one character height and value text width is equal bar width, then we shouldn't print the value

I think this makes sense.

[joshka]

There are two "bugs"

• .len() -> .width() introduced in feat(barchart): enable barchart groups #288
• < which has been there since the beginning.

This is a really good example for working out what to show:

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
a b c d e f g h i 

  1 2 3 4 5 6 7 8 
a b c d e f g h i 

I think we can probably agree that for that scenario, we can't show the values at all (except perhaps 8).
but what about if the area is one cell taller? Does the following make sense?

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
  1 2 3 4 5 6 7 8 
a b c d e f g h i 

Or should it be:

          ▂ ▄ ▆ █ 
  ▂ ▄ ▆ 4 5 6 7 8 
a b c d e f g h i 

Or:

          ▂ ▄ ▆ █ 
  ▂ ▄ ▆ █ 5 6 7 8 
a b c d e f g h i 

[kdheepak]

I actually think this is what I'd prefer:

  1 2 3 4 5 6 7 8 
  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
a b c d e f g h i 

and I'd prefer this if there's lesser room:

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
a b c d e f g h i 

And this with a height of just 1:

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 

My reasoning is that if someone is using the barchart widget, not getting any bar chart is an unexpected experience.

[joshka]

That does look nicer. So there's probably a few ways to choose where exactly to display the labels... which means there's probably some complexity in the implementation for this.
We could provide the user configuration options to choose the position, or automatically choose the best position, or do the first bit and then the second bit.

[karthago1]

I will try to help solving the following 2 cases:
▁ ▂ ▃ ▄ ▅ ▆ ▇ █

and

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
a b c d e f g h i 

but for the following case maybe I will ignore it for the time being, because the numbers are reserving some space, which would be available for the bars currently

  1 2 3 4 5 6 7 8 
  ▁ ▂ ▃ ▄ ▅ ▆ ▇ █ 
a b c d e f g h i 

[joshka]

Please consider adding each of those as tests (rendering that data into 1,2,3 height), as they're good characterization tests to have if we decide to make it possible to set the label position.

[karthago1]

I created an PR which handles all cases, that I could think of:

1. ▁ ▂ ▃ ▄ ▅ ▆ ▇ 8
   notice that the 8 is taking a full cell, so it is printed. In a real application the 8 can have for example a white background and black foreground, then it will look nice
   

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ 8
a b c d e f g h i
      Group      

  ▁ ▂ ▃ ▄ ▅ ▆ ▇ 8
a b c d e f g h i

4. note: as mentioned in under 1. using the right background the 4 .. 8 will look fine

          ▂ ▄ ▆ █
  ▂ ▄ ▆ 4 5 6 7 8
a b c d e f g h i

   1▁ 2▂ 3▃ 4▄ 5▅ 6▆ 7▇ 8█
a  b  c  d  e  f  g  h  i 

which will look like this: