Repository navigation
Conversation
c8fb1bc to
42a6276
Compare
Blending two RGB values by a ratio was copied a dozen times across platform.ui, with differing parameter types and rounding. The new JFaceColors.blend is now the single implementation: FormColors.blend and ColorUtil.blend delegate to it and the private copies are removed. Colors stay the same, or differ by at most one RGB step where a copy rounded differently. Fixes eclipse-platform#4486 Assisted-by: multiple AI agents and layers of automated tooling 🤖
42a6276 to
f3faf8d
Compare
| /** | ||
| * Blends two colors: a <code>ratio</code> of 1 returns <code>c1</code>, a | ||
| * <code>ratio</code> of 0 returns <code>c2</code>. Channel values are | ||
| * truncated and clamped to the valid range. | ||
| * | ||
| * @param ratio the share of <code>c1</code>, normally in [0, 1] | ||
| * @since 3.41 | ||
| */ | ||
| public static RGB blend(RGB c1, RGB c2, double ratio) { | ||
| return new RGB(blend(c1.red, c2.red, ratio), blend(c1.green, c2.green, ratio), | ||
| blend(c1.blue, c2.blue, ratio)); | ||
| } | ||
|
|
||
| private static int blend(int v1, int v2, double ratio) { | ||
| // the epsilon keeps ratios like 0.3 from truncating exact results one step down | ||
| int v = (int) (ratio * v1 + (1 - ratio) * v2 + 1e-9); | ||
| return Math.clamp(v, 0, 255); | ||
| } | ||
|
|
There was a problem hiding this comment.
This method do not depend on anything of JFace or platform. For me it seems a much better place would be to add this to the SWT RGB class instead, what even would avoid needing to call a static method and instead I can call RGB blended = c1.blend(c2, 0.7).
This would also address the concerns from @ptziegler that JFaceColors actually documents a different purpose and make it even more broadly available.
There was a problem hiding this comment.
We usually keep SWT super focused and so far all copies are in the surface of JFace. JFace usually server the "sugar on top of SWT" functionality. @akurtakov WDYT?
There was a problem hiding this comment.
Thanks for the explanation, I understand the wish to keep SWT focused and to deduplicate the copies.
I still see it a bit differently:
- The method only works on
RGBand has nothing to do with JFace, soJFaceColorsis not a natural home (see also @ptziegler's remark about its documented purpose). - "All copies are in JFace surface" does not seem fully accurate, as @merks noted, SWT itself blends in e.g.
CBanner. So SWT is a consumer of the same functionality, which is an argument for putting it there.
To me there are two consistent options:
- It is generic and wanted functionality, then it belongs to SWT (e.g.
RGB#blend(RGB, double)), where everyone benefits. - It only looks similar in a few places, then I'd rather not add a public helper.
The code is neither performance critical nor complicated, and a shared public method binds us and consumers to one set of rounding/clamping semantics (the PR itself notes that some callers differ by one step).
Putting it in JFaceColors sits in between: new public API to maintain, but not where it is most reusable.
There was a problem hiding this comment.
I have no personal strong preference here but I want to get the opinion of core SWT developers. Lets see if someone chips in. This stuff here is also not urgent, we have this copied code outside JFace and SWT since ages.
There was a problem hiding this comment.
Maybe @HeikoKlare has an opinion as well...
An in general SWT might consider if eliminating pointless color copying might be worthwhile goal...
There was a problem hiding this comment.
I'd be fine with this in SWT's RGB. It's pure color math, needs no native code, and fits next to RGB(float, float, float) and getHSB(). Those were added in 3.2 for the same reason, and SWT itself still doesn't use them; their callers are in platform.ui.
There was a problem hiding this comment.
I agree. SWT's RGB class would be a fitting home for this functionality (both from OO perspective as well with respect to the scope of SWT). Using readable parameter names would also be great, at least but not exclusive to public API. It currently also looks like a mixture of semantics: the parameter name fragments and the javadoc speak of colors, whereas it''s about RGB values (and color is an SWT concept on top of it).
There was a problem hiding this comment.
Regarding API/implementation - my experience is that the closer one stays to css semantics (e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/color_value/color-mix ) the easier it is for the API to be understood and used.
There was a problem hiding this comment.
It currently also looks like a mixture of semantics: the parameter name fragments and the javadoc speak of colors, whereas it''s about RGB values (and color is an SWT concept on top of it).
RGB is/was the device-less representation of a Color in SWT in the past, now Colors are not needed to be disposed its use becomes less useful from my point of view anyways and the are mostly exchangeable (even though theoretically a Color can use a different color model). On the other-hand for the same reason it wont hurt to have the same (delegating to the RGB class) code in the Color class as well.
|
In terms of code simplification/reduction, this looks super. I too was going to observe there are even places in SWT doing blending, like CBanner. But I didn't have the idea of the RGB class... Not directly related, so sorry for the digression, but I thought I'd point it out here anyway SWT does color copying which I think is pointless now that colors don't need to be disposed, e.g., 
It even keeps a map for the copies in place and disposes the value of the map: 
It could be simplified.... |
Blending two RGB values by a ratio was copied about a dozen times across platform.ui, each with its own parameter type and rounding. This adds
JFaceColors.blend(RGB, RGB, double)as the single implementation in the lowest bundle all of them depend on. The existing publicFormColors.blendandColorUtil.blendnow delegate to it and the private copies are removed. The integer percentage and 0.56 factor callers produce exactly the same colors as before, the remaining ones differ by at most one RGB step, and copies in other repositories can move to the new API once it is released.Fixes #4486