On Wed, 7 Aug 2024 20:58:53 GMT, John Hendrikx <jhendr...@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
