On Fri, 19 Jan 2024 10:36:57 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> I'll add the `@since 23`, however it can't be called by anyone except FX
>> itself.
>>
>> Some background: the readBinary/writeBinary methods are ultimately called
>> via the publicly accessible methods `loadBinary` and `saveBinary` in
>> `javafx.css.Stylesheet`.
>>
>> The reason it needs to be `protected` now is because `CompoundSelector` is
>> using this (but IMHO, it shouldn't have). Why I say it shouldn't? That's
>> because it already knows what it will be reading will all be
>> `SimpleSelector`s, as it stores a counter (a `short`) and then loads that
>> many `SimpleSelector`s. However, by not taking the direct route of using
>> `SimpleSelector#readBinary` (and the corresponding
>> `SimpleSelector#writeBinary`) there is an additional `byte` prefix
>> indicating the type of selector -- this isn't needed and wastes some space
>> in the binary format.
>>
>> Changing that now however would make the binary format incompatible with
>> earlier versions, so I think making this `protected` is a reasonable
>> solution. It also mirrors the `writeBinary` method then, which was already
>> `protected`.
>
> I missed something in my above evaluation. The counterpart method
> `writeBinary` is not `static`. Although that makes sense as in that case you
> do have an instance you wish to write, it does make the read/write API
> asymmetric.
>
> It's not possible to make `readBinary` an instance method though as it is the
> method that is creating those instances.
>
> The other way around is possible (make `writeBinary` static), but it would
> then again need a pattern match to determine the correct static `writeBinary`
> to call when writing an arbitrary `Selector`. However, this would have
> allowed `CompoundSelector` to call a static version of
> `SimpleSelector#writeBinary` and `readBinary`, avoiding the extra identifying
> byte in the binary format, and avoiding the cast when reading it back.
>
> The read/write loops below:
>
> +
> final int nSelectors = is.readShort();
> final List<SimpleSelector> selectors = new ArrayList<>();
> for (int n=0; n<nSelectors; n++) {
> selectors.add((SimpleSelector)Selector.readBinary(bssVersion,
> is,strings));
> }
> +
> os.writeShort(selectors.size()); // writes the number of
> SimpleSelectors to the binary stream
> for (int n=0; n< selectors.size(); n++)
> selectors.get(n).writeBinary(os,stringStore); // writes each individually
>
> Would then have become:
>
> +
> final int nSelectors = is.readShort();
> final List<SimpleSelector> selectors = new ArrayList<>();
> for (int n=0; n<nSelectors; n++) {
> selectors.add(SimpleSelector.readBinary(bssVersion, is,strings));
> // cast is gone
> }
> +
> os.writeShort(selectors.size()); // writes the number of
> SimpleSelectors to the binary stream
> for (int n=0; n< selectors.size(); n++)
> SimpleSelector.writeBinaryStatic(selectors.get(n), os, stringStore); //
> writes each individually
it looks to me readBinary should be static, and writeBinary an instance method
- this is a normal pattern. the asymmetry is dictated by the fact that we
don't have an instance when reading, so typically read() methods read the
stream and create an instance at the last moment with the specific constructor.
unless, of course, the design allows for mutation and the fields can be set().
Alternatively, readBinary() could be moved to another class (helper? reader?)
to avoid making this an API change.
what do you think?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670922573
