On Fri, 9 Aug 2024 19:42:56 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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. I also think that we're probably dealing with two different things that are mixed up in `StyleConverter`. But instead of having this discussion here, it probably makes more sense to have it in the context of [JDK-8333121](https://bugs.openjdk.org/browse/JDK-8333121) because it seems relevant to eventually support both short-hand and long-hand property notations. I've renamed `WithReconstructionSupport` to `SubPropertyConverter` and moved it to an internal package for now. Since both `BackgroundConverter` and `BorderConverter` are not public, we can change it any way we want at a later time. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1712306610
