feat(barchart): allow to render charts smaller than 3 lines

[karthago1]

Add an internal structure LabelInfo, which stores the reserved height for the labels (0, 1 or 2) and also the state of the labels, whether they will be shown or not.

Add internal property to the Bar transformed_value, which is used only for in the vertical charts. This value is needed in 2 reasons:

1. to draw the vertical bar
2. to draw the bar value or text_value. If the transformed_value is below 8 and the bar_width is 1, then the value will not be printed

transformed_value is line height multiplied by 8.
(8 is the number of element of symbols::bar::Set struct)

fixes #513

[codecov]

Codecov Report

Merging #532 (2906948) into main (3bda372) will decrease coverage by 0.30%.
The diff coverage is 84.17%.

@@            Coverage Diff             @@
##             main     #532      +/-   ##
==========================================
- Coverage   89.95%   89.65%   -0.30%     
==========================================
  Files          41       41              
  Lines       11347    11437      +90     
==========================================
+ Hits        10207    10254      +47     
- Misses       1140     1183      +43     

Files	Coverage Δ
src/widgets/barchart/bar.rs	98.90% <100.00%> (+0.03%)	⬆️
src/widgets/barchart.rs	89.54% <83.62%> (-3.71%)	⬇️

... and 1 file with indirect coverage changes

[joshka]

Hey,

I'd really like to see the BarChart logic to be simplified, so took a stab at partially refactoring this into the following. It's incomplete and untested (more of an idea than a full implementation). I think this approach it's this leads to the code being a lot more (subjectively) maintainable and readable as there's less reliance on temporal changes to state (e.g. remove_invisible_groups_and_bars and calculate_transformed_value). The other core simplification is explicitly splitting the layout and rendering tasks so that each part is much clearer, this avoids spreading the layout between remove_invisible_groups_and_bars and the render_{horizontal,vertical}_bars() functions.

The other part that I'd suggest is moving away from is testing private methods like remove_invisible_groups_and_bars and label_info. Instead, it's more important to test the rendering of each scenario than the internals of how we got there. By testing from the outside of the unit, rather than testing the internals, it makes it easier to refactor the internals for maintainability / readability / performance as necessary.

impl<'a> Widget for BarChart<'a> {
    fn render(mut self, mut area: Rect, buf: &mut Buffer) {
        buf.set_style(area, self.style);
        self.render_block(area, buf);
        let area = self.layout_block(area);
        if area.is_emtpy() || self.groups.is_empty() || self.bar_width == 0 {
            return;
        }
        match self.direction {
            Direction::Vertical => self.render_vertical(area, buf),
            Direction::Horizontal => self.render_horizontal(area, buf),
        }
    }
}

impl<'a> BarChart<'a> {
    fn render_block(&self, area: Rect, buf: &mut Buffer) {
        self.block.as_ref().map(|&block| block.render(area, buf));
    }

    fn layout_block(&self, area: Rect) -> Rect {
        self.block
            .as_ref()
            .map_or_else(|| area, |&block| block.inner(area))
    }

    fn render_vertical(self, area: Rect, buf: &mut Buffer) {
        let max_value = self.max_value();
        let layout = self.layout_vertical(area);
        let (bar_height, label_height, group_label_height) = self.heights(area);
        for (group, group_area) in self.groups.iter().zip(layout) {
            group.render_vertical_bars(
                max_value,
                bar_height,
                self.bar_style,
                self.value_style,
                group_area,
                buf,
            );
            group.render_vertical_labels(label_height, self.label_style, group_area, buf);
            group.render_vertical_group_label(
                group_label_height,
                self.label_style,
                group_area,
                buf,
            );
        }
    }

    /// Returns a Rect for each group that will contain all the bars of this group.
    fn layout_vertical(&self, area: Rect) -> Vec<Rect> {
        self.groups
            .iter()
            .map(|g| g.layout_vertical(area, self.bar_width, self.bar_gap))
            .scan(0, |x, mut group_area| {
                // layout the groups from left to right ensuring that the group width is not
                // greater than the available width. Groups beyond the available width are
                // truncated
                group_area.x += *x;
                group_area.width = group_area.width.min(area.width.saturating_sub(*x));
                *x = x
                    .saturating_add(group_area.width)
                    .saturating_add(self.group_gap);
                Some(group_area)
            })
            .collect()
    }

    fn heights(&self, area: Rect) -> (u16, u16, u16) {
        let has_group_labels = self.groups.iter().any(BarGroup::has_label);
        let has_bar_labels = self.groups.iter().any(BarGroup::has_bar_labels);
        // ensure that the height of the leftover area is at least 1 so that the bars can be
        // rendered, and that labels get precedence over group labels
        let bars_height = area
            .height
            .saturating_sub(has_group_labels as u16 + has_bar_labels as u16)
            .max(1);
        let labels_height = area.height.saturating_sub(bars_height).min(1);
        let group_label_height = area
            .height
            .saturating_sub(bars_height)
            .saturating_sub(labels_height)
            .min(1);
        (bars_height, labels_height, group_label_height)
    }

    fn max_value(&self) -> u64 {
        self.max
            .or_else(|| self.groups.iter().flat_map(BarGroup::max).max())
            .unwrap_or_default()
            .max(1)
    }

Does this make more sense?

[karthago1]

I think this approach it's this leads to the code being a lot more (subjectively) maintainable and readable as there's less reliance on temporal changes to state (e.g. remove_invisible_groups_and_bars and calculate_transformed_value).

I agree. I removed calculate_transformed_value. But I am still in love with the remove_invisible_groups_and_bars. As we agreed once, you will someday eliminate my beloved anti pattern function 😢 .

The other part that I'd suggest is moving away from is testing private methods like remove_invisible_groups_and_bars and label_info. Instead, it's more important to test the rendering of each scenario than the internals of how we got there. By testing from the outside of the unit, rather than testing the internals, it makes it easier to refactor the internals for maintainability / readability / performance as necessary.

done

Does this make more sense?

maybe 😸
I don't like the suggested heights function much. It is always iterating over everything even if it is unnecessary. Readability is maybe depending on the person not better than the label_info

Anyway this is a second try, it is for sure not what you expect. But please, if it is far from your expectation or making the code worse, then don't hesitate to close the PR. Of course I will be happy to tweek/change small things, if this PR is more or less still acceptable.

(I appreciate very much your taking the time and maintaining this wonderful library ❤️ )

[joshka]

Thanks - looks good to me.

I'm approving this, (with a couple of quick nits to fix).

---

Regarding the "beloved antipattern" ;)

There's a strong rationale behind why I find it easier to read the refactored code. In general, code that is immutable is easier to reason about, as you never have to keep in mind which things have happened earlier to fields. The subjective readability measure comes from not having to read all of the entire code in order to understand what each field will be set to at various points in the creation / rendering of the widget, and not having to look for mutations inside several levels of for loops.

The remove invisible groups and bars approach (which mutates the state) works because the widget is consumed on render. If we change the widget to be borrowed in the future, then the entire logic for the widget needs to rewritten (this is something that we're considering).

[joshka]

I don't like the suggested heights function much. It is always iterating over everything even if it is unnecessary. Readability is maybe depending on the person not better than the label_info

I don't think I quite understand this comment. The heights function has the exact effect and negligible difference in perfo for small N (which is what we're dealing with at the terminal) as the label_info function.

[karthago1]

@joshka I agree with everything you said. Maybe this weekend I will try to create a PR which removes my nice function remove_invisible_groups_and_bars. I will try to get your suggestions implemented.

[joshka]

LGTM. Thanks for the PR and updates.

[joshka]

Merged with a small rewording of the commit message