feat(barchart): enable barchart groups

[karthago1]

I modified the barchart to support adding multiple bars from different data set
see the the following image:

if this feature is needed, then I can continue and write some unit tests

[SLASHLogin]

I'm not sure how this compares to current barchart, implementation wise, but maybe it could be solved by adding a method/flag to the current barchart, instead of introducing a new widget?

[karthago1]

true I changed the old barchart. it should be backwards compatible

[codecov]

Codecov Report

Merging #288 (4a24730) into main (804115a) will increase coverage by 0.68%.
The diff coverage is 90.67%.

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   83.11%   83.80%   +0.68%     
==========================================
  Files          37       39       +2     
  Lines        7717     8031     +314     
==========================================
+ Hits         6414     6730     +316     
+ Misses       1303     1301       -2     

Impacted Files	Coverage Δ
src/lib.rs	100.00% <ø> (ø)
src/widgets/mod.rs	64.70% <ø> (ø)
src/widgets/barchart/mod.rs	89.37% <89.37%> (ø)
src/widgets/barchart/bar.rs	100.00% <100.00%> (ø)
src/widgets/barchart/bar_group.rs	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[joshka]

I like the idea and direction - perhaps consider documenting the user's perspective a little before continuing into the code. (It's important to tell us what you want to do, so that when it's complete, it's obvious).

I notice that this adds a new "Group" feature, but "Group" only appears in the API once. How would you design the API so that it made this feature more explicitly obvious?

I think it's probably possible to keep the single data() function, and expect the user to call it multiple times rather than adding a new add_data() function which is confusingly similar.

Can you please make an issue for this to discuss the functionality of the new feature? (Would be nice to get a few more eyes on it).

[karthago1]

I think it's probably possible to keep the single data() function, and expect the user to call it multiple times rather than adding a new add_data() function which is confusingly similar.

true. the problem with data(), that it passes always one dataset with the labels. the labels should be the same for all datasets. so if the user calls data() twice. the first time I can store the labels. and the second call should I ignore them?

possible Solutions:

1. remove data() and rename add_data() to data(). this is not backwards compatible solution. labels can be passed by the function labels() instead of the old data()
2. maybe I can modify data() to pass all datasets at once and stay backwards compatible too. I will check this possibility
3. keep the same data() and add new function add_data() which adds only one data set. (current solution)

the following 2 code sections produces the same result:

BarChart::default()
        .data(&[("Mar", 9), ("Apr", 12), ("May", 5), ("Jun", 8)])
        .add_data(&[1, 2, 3, 4])
        .group_gap(4);

BarChart::default()
        .add_data(&[9, 12, 5, 8])
        .add_data(&[1, 2, 3, 4])
        .group_gap(4)
        .labels(&["Mars", "Apr", "May", "Jun"]);

Can you please make an issue for this to discuss the functionality of the new feature? (Would be nice to get a few more eyes on it).

done

[joshka]

Hey there. I have some concerns around the size of the new render method, and the lack of docs / new unit tests. I'm liking where this is heading though.

[mindoodoo]

Moving the bar printing into it's own method helped quite a bit with readability. Dropped a couple thoughts on how render could be further split up / shortened as well as some nitpicks.

Additionally you could also move the label printing (group and bar) in another (or other) method(s).

Finally you have a little bug in your example where the bar colors are applied to parts of the legend's border.

[karthago1]

Finally you have a little bug in your example where the bar colors are applied to parts of the legend's border.

this is feature not a bug 😄 . But it is fixed. (there still is another hidden small bug in the legend 😸, but this MR is about the BarChart )

I renamed Group to BarGroup, because I think Group is too general term

[joshka]

There's two approaches to not rendering specific bars, truncating them before rendering, or filtering them after.
The original code took the filter approach, while the new code takes the truncate approach.

The truncate approach seems more difficult to reason about as it changes fields outside of the method to communicate with other methods. (This is like Alice calling Bob on the phone to tell him something then calling Carol and asking her to ask Bob for the information just provided)

I think I can sort of see how the code landed there, but the following general approach (using a remaining area Rect to render the bar / group into) might help make this code clearer:

let remaining_area = area;
for group in data {
  if too_small_for_a_group(remaining_area) {
    break;
  }
  render_group(&group, &mut remaining_area);

}

fn render_group(group &Group, &mut remaining_area: Rect) {
  render_group_label(...);
  for bar in group.bars {
    if too_small_for_any_bar(remaining_area) {
      break;
    }
    render_bar(bar, &mut remaining_area);
  }
  remaining_area = Rect { x: remaining_area.x + group_gap)
}

fn render_bar() {
  let bar_area = ...
  let label_area = ...
  let value_area = ...
  render_bar_main_part(bar, bar_area);
  render_label(bar, label_area);
  render_value(...);
  let w = bar_width + bar_gap;
  *remaining_area = remaining_area {x: remaining_area + bar_width + bar_gap, ...}
}

[karthago1]

The truncate approach seems more difficult to reason about as it changes fields outside of the method to communicate with other methods. (This is like Alice calling Bob on the phone to tell him something then calling Carol and asking her to ask Bob for the information just provided)

Preprocessing data before working on it, is very common approach. I don't think the Alice bob and Carol example is very accurate 😄 .

I think I can sort of see how the code landed there, but the following general approach (using a remaining area Rect to render the bar / group into) might help make this code clearer:

you are suggesting a third method, which is in my opinion worse than the truncate and filtering methods, which are both very similar. here my comparison:

1. Truncate method

preprocess the data once. afterwards there is no chance that you are working on the wrong data, because they simply doesn't exist. There is in total 4 places where we must filter. (in the future maybe more)

2. Filter method

the original code had 2 loops, we need to filter twice. if someone adds a 3. loop, the chances are high, that he will forget about filtering.

with this patch we need to filter the bars too, which makes this method a little bit harder to implement. now we need to filter 2 nested loops in 2 different functions. I can't see any reason why this method is superior then preprocesing the data first

3. you suggestion (general approach)

every time you draw, you need to check, if there is enough space. I see 2 drawbacks. the first one: the check logic will needs to be duplicated at least twice. when printing the bars and later when printing the labels.

the second drawback: this check will be executed in render_bars many times (area height * number of groups * number of bars inside a group) (we have 3 nested loops)

render_labels_and_values is similar. It contains 2 nested loops.

I would rather to filter or preprocess the data, than checking in every iteration, whether there is enough space. and the problem get worse with every additional loop. (code duplication)

till now you improved the code immensely, the length of that method became very short. I think most developers will understand it. 😄

[joshka]

Preprocessing data before working on it, is very common approach. I don't think the Alice bob and Carol example is very accurate 😄 .

The concept that's at play here is https://en.wikipedia.org/wiki/Action_at_a_distance_%28computer_programming%29.
This implementation is ok only because the widget is consumed after rendering.

you are suggesting a third method, which is in my opinion worse than the truncate and filtering methods, which are both very similar

The third method seems like it would lead to a simpler and more obvious solution with generally shorter methods that just do one thing. The difference in my mind is doing things in batches (calculate, render bars, render labels) vs doing things one by one in a nested fashion. In the batch approach, you have to understand how each part affects the whole, whereas the individual nested approach the interaction between the pieces is right there.

every time you draw, you need to check, if there is enough space. I see 2 drawbacks. the first one: the check logic will needs to be duplicated at least twice. when printing the bars and later when printing the labels.

This is a problem only because the rendering of the bars and labels is separate - if bars and their label / value are rendered at the same time then this isn't a problem.

the second drawback: this check will be executed in render_bars many times (area height * number of groups * number of bars inside a group) (we have 3 nested loops)

This check happens already, it just happens within the delete invisible method.

P.s. Happy to fix this later if the code seems reasonably obvious to @mindoodoo as is.

[joshka]

Approved Subject to @mindoodoo's review

[mindoodoo]

LGTM ! Thanks for the PR !

[joshka]

Merged! Thanks for your contribution @karthago1
(The last force push was to rebase and sign the commits)

[karthago1]

@joshka and @mindoodoo thank you too for your help and patience 😄 . I can't wait to switch to the crate.io ratatui version at work.

The use case we have is to show the operating hours of multiple temperature sensors in different temperature ranges:
it looks similar to the following:

          ████   ████                ████
          ████   ████                ████
     ████ ████   ████ ████           ████ ████
▄▄▄▄ ████ ████   ████ ████ ████      ████ ████ ████
████ ████ ████   ████ ████ ████ ...  ████ ████ ████ 
████ ████ ████   ████ ████ ████      ████ ████ ████
█50█ █60█ █90█   █90█ █60█ █50█      █90█ █60█ █50█
   -40..-31°C       -30..-21°C         150..159°C

the only think I wish to do, but I don't know if it is possible, is to write the numbers vertically. (instead of horizontally)
For now I made the bars very wide so I can print the values. but I have a lot bars and I need to scroll horizontally. I don't know if it is possible to write text horizontally with ratatui.