Skip to content
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

Add translateY style parameter to legend + test #568

Merged
merged 18 commits into from
Sep 3, 2024
Merged

Conversation

schaumb
Copy link
Contributor

@schaumb schaumb commented Aug 26, 2024

No description provided.

@schaumb schaumb requested a review from simzer August 26, 2024 12:55
@schaumb schaumb requested a review from simzer August 29, 2024 11:06
src/chart/rendering/drawlegend.h Show resolved Hide resolved
src/chart/rendering/drawlegend.cpp Show resolved Hide resolved

canvas.setBrushGradient(
{transform(line.begin), transform(line.end)},
gradient);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this setter is a bit overcomplicated for the problem.
What we would like to be able to do is to set a transparency gradient for the label, if needed.
So why not simply give a Math::SegmentedFunction<double> transparencyGradient, Geom::Line gradientPosition parameters for DrawLabel and calculate the color gradient in DrawLabel from the calculated text color?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it's not just the label but the marker needs to. And that will be a duplication for the same gradient problem.

Copy link
Member

@simzer simzer Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It's unfortunate. Still could be more clean. I see three problems:

  1. This class encapsulates three things (building of the ColorGradient, transforming the line, and setting it to the canvas). I think it would be better if it would have a singular purpose, building the ColorGradient.

  2. (and 3.) How we build the color gradient is that we have an alpha/transparency gradient, and we get the color from outside. The class does not represent this, since it contains a colorgradient, and the transparency gradient is hard coded in the function.

I think a clearer and more generic approach would be to have a class, which stores an arbitrary alpha gradient and has a method which accepts a color and builds the ColorGradient.

Something like this:

/// legend draw:
fadeoutLine = markerWindowRect.leftSide()
fadeoutGradient = AlphaGradient({ 
  {0.0, 0.0}, 
  {fadeElementPercent, 1.0}, 
  {1.0 - fadeElementPercent, 1.0}, 
  {1.0, 0.0} 
})

// marker draw and label draw
canvas.setBrushGradient(transform(fadeoutLine), fadeoutGradient.toColorGradient(color))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, draw label is a more generic lower level utility class then legend draw. I think it's not a good practice to make the interface of the lower level class more cryptic to optimise code duplication in the higher level user class.

@schaumb schaumb requested a review from simzer September 3, 2024 12:01
src/base/math/segmentedfunc.tpp Show resolved Hide resolved
@schaumb schaumb merged commit 8c9a2f7 into main Sep 3, 2024
1 check passed
@schaumb schaumb deleted the Translate_legend branch September 3, 2024 12:47
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