RFR: 8332895: Support interpolation for backgrounds and borders [v18]

[Michael Strauß]

On Wed, 7 Aug 2024 20:58:53 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> I just saw a boolean variable being instantiated from an annotation and thought "why jump through the multiple hoops?".  since there is a boolean, why not pass it directly?
>> 
>> it's less about memory allocation (though I would prefer to minimize that as well, but as @hjohn pointed out the difference is just a few bytes), but more about "entities must not be multiplied beyond necessity".
>
> Although I'm not advocating for a specific change, I think looking at how `T convert(Map)` works when it was newly introduced in Java 9 should be taken into account as well.
> 
> There seem to be a couple of general approaches:
> 
> 1. Have a marker that can be checked from the outside
>     - Annotation (checked from the outside)
>         - Unusual for this kind of check
>     - Marker interface
>         - Seen more often, but unusual to not put the new method there
>     - Subtype 
>         - Bad idea, you can only inherit once
> 2. Introduce a new interface with the new method
>     - Defines the method and serves as the marker at the same time
> 3. Have a method that can be called that guards a 2nd method call
>     - Doesn't matter how this is fed (constructor, annotation, override)
>         -  You see this more often, but it's not a great pattern (2 method calls for the price of one...)
> 4. Return a special value from a method that may be unsupported
>     - The most obvious (modern) candidate is to return `Optional` but is a bit unusual to indicate support/non-support
>     - Throw `UnsupportedOperationException` -- although standard, I think it indicates a programmer mistake that should be avoided (in other words, encountering this exception in production code should result in a code fix)
>     - Return `null`
> 
> Now the very last option (returning `null`) was the way this was handled when `T convert(Map)` was introduced in Java 9.  Even though it may not be the prettiest solution, it has two things going for it:
> 
> - It's fast (probably the fastest of the above options, although for most by just a slim margin)
> - It fits in with the existing design
> 
> So, my order of preference:
> - Just return `null`
> - A new interface, which includes the new method
> - A marker only interface
> - A supportsXYZ method (regardless of how that is approached)
> 
> I don't think I could get behind any of the other options, because they either stick out like a sore thumb versus the existing design, limit future options or just perform too poorly.

Returning `null` seems fine from the perspective of `StyleConverter`, but it makes the calling code very awkward. Remember, we ended up here because we needed a way to detect whether an object would support component-wise transitions. If we can't detect that without invoking `convertBack`, we would need to either:
1. Speculatively decompose the value without knowing whether there even are any transitions defined on the node. This is bad because most of the time, there will be no transitions; we will end up deconstructing many objects for no reason.
2. _Assume_ that an object is component-transitionable, look up all potential transitions, and decompose the value; then, if we were wrong with out assumption, go back to the start and try again with another code path (`Interpolatable` or no transition).

Instead, what I've implemented now is a new interface `StyleConverter.WithReconstructionSupport`, which contains both methods:

public interface WithReconstructionSupport<T> {
    T convert(Map<CssMetaData<? extends Styleable, ?>, Object> values);
    Map<CssMetaData<? extends Styleable, ?>, Object> convertBack(T value);
}


This allows us to use object deconstruction and reconstruction with a single interface reference, and also allows us to detect such support very easily.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1708139444