Skip to content

Fix BoxPlot legend and rename BoxPlot and BarChart color to default_color #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkalte666
Copy link
Contributor

@mkalte666 mkalte666 commented Apr 7, 2025

Fvg on discord noticed that BoxPlots were missing the legend.

Screenshot from 2025-04-07 23-16-30

Upon further investigation, either some merge was a bit icky, or something was missed when adding in PlotItemBase.

To fix this i have

  • Replaced BoxPlot::color with BoxPlot::default_color, as it was a name collision with PlotItem::color.
  • Replaced BarChart::color with BarChart::default_color, as it was a name collision with PlotItem::color.

I know this is a breaking change without a deprication first, but i feel this is better than going for a deprication first keeping that name collision in for longer.

  • Remove BoxPlots implementation of the PlotItem::name() function, and the BoxPlot::name field, as it was incomplete, and is also covered in PlotItemBase
  • Reduced the visibility of BarChart::default_color and BoxPlot::default_color, as it is only read, and only in plot_ui.rs, and that can be done via our newly not-name-colliding PlotItem::color.

I also noticed there are missing snapshots for the (Charts.png is only the histogram, and only vertical). Barchart and boxplot are missing. But that is worth its own PR.

In any case, with this, i get the expected legend back:

Screenshot from 2025-04-07 23-16-11

  • I have followed the instructions in the PR template
  • I have run check.sh locally and it went through.

  * *Replaced* `BoxPlot::color` with `BoxPlot::default_color`, as it was a name collision with `PlotItem::color`.
  * *Replaced* `BarChart::color` with `BarChart::default_color`, as it was a name collision with `PlotItem::color`.

 I know this is a breaking change without a deprication first, but i feel this is better than going for a deprication first.

  * Remove BoxPlots implementation of the `PlotItem::name()` function, and the `BoxPlot::name` field, as it was incomplete, and is also covered in `PlotItemBase`

I also noticed there are missing snapshots for the barchart and boxplot, but i'd say thats worth its own PR.
@frankvgompel
Copy link

Doesn't this fix imply that PlotItem::name() is completely redundant? All item names (that are used in the legend) seem already defined in the new() function.

@mkalte666
Copy link
Contributor Author

mkalte666 commented Apr 8, 2025

Doesn't this fix imply that PlotItem::name() is completely redundant? All item names (that are used in the legend) seem already defined in the new() function.

PlotItem defines shared functionality between all the things that need plotting - such as Lines, BoxPlots, BarCharts, Points, and whatever else. Specifically in this case, the legend does not care what is providing a legend for - it just eats some form of dyn PlotItems in the end.

PlotItem::name() is a getter, not a setter - and indeed, the fields are set via new().

What i have done is remove the BoxPlot-specific implementation of PlotItem::name(), as the provided default implementation already gets the name from the PlotItemBase.
Additionally, i removed the BoxPlot::name field, as that was redundant with the PlotItemBase::name field, and is currently not correctly initialized in the new function anyway. (see here:

name: String::new(),
).

Technically, just removing the name field and the name implementation is enough to fix the legend issue, i have however decided to - while at it - rename the BoxPlot::color function to BoxPlot::default_color, (and same for BarChart, as otherwise it would collide with the color-getter from PlotItem, and you would need to do some as PlotItem access to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants