On Fri, 9 Aug 2024 13:36:34 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Clear, thanks MIchael. I would suggest `Reconstructable` as the name, which
>> seems more inline with interface naming (especially `Interpolatable`). Reads
>> nice also:
>>
>> if (converter instanceof Reconstructable r) { ... }
>
> But the converter is not reconstructable (in comparison to Interpolatable,
> which is something the marked types actually are).
Sorry to keep harping on this, but so far, I think all the options are quite
poor from the perspective of the user, and from a perspective of "could it have
been designed this way in the first place".
I also think I smell something fishy. The `StyleConverter` seems to serve two
unrelated purposes. One is to convert raw parsed values for the CSS parser
(`convert(ParsedValue, Font)`) to a Java object.
The other is to support sub-properties (which you can also detect by checking
if `CssMetaData.getSubProperties` is not empty). Although that method is also
called `convert` it is only used by `CssStyleHelper`, which is completely
unrelated to parsing. A more accurate name for that method would be
"consolidateSubPropertyValues" (and the opposite would then be something like
"extractSubPropertyValues", or `consolidate` and `extract` for short).
There does not seem to be any overlap between these two purposes. That is it
to say, a style converter that is used for decoding sub-properties is never
used for parsing, and vice versa. FX also does not support special syntax for
specifying say a border in short hand form (like `-fx-border: dashed red`).
I think it may be a good idea to perhaps have a look if these purposes
shouldn't just be split. On the one hand, you have a style converter which
uses `convert` for parsing. On the other hand, you have `CssMetaData`, which
when it has sub-properties **must** support a back-and-forth translation
(`convert` + `convertBack`) -- this is currently not enforced (ie. you can
create `CssMetaData` with sub-properties but then **not** implement
`convert(Map)` in the used style converter... this causes problems later on.
The only CSS property currently that supports both short-hand and
sub-properties is actually the newly introduced `transition` property. This
could either implement two interfaces, or it could just be two classes.
I've done a quick attempt to do such a split (not looking too much at API) and
the whole thing compiles and passes all tests still. This is surprising,
because I deleted several pieces of code, that are apparently never used (or
perhaps not tested for, we can investigate this further).
See here: #1533
Diff:
https://github.com/openjdk/jfx/pull/1533/commits/8f6d1e56a43068184599ad1ea47b4da98eb70bff#diff-fc96cf3204909a6110e64c37a935927be56efedc9f3f4f6f6843cc4fa7da5a1f
Let me know if you feel this has merit.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1712057854
