On Fri, 9 Aug 2024 22:36:03 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> 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. Yes, that's a good idea, I was mainly worried about introducing a new public API that we can't remove later when we haven't figured out yet what the best direction would be. With this API removed, I think we can get this merged. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1712627157
