On Wed, 7 Aug 2024 23:55:39 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> 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.
+1 for `StyleConverter.WithReconstructionSupport`
maybe use a shorter name, like
`StyleConverter.WithReconstruction`
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1709803243
