On Fri, 19 Jan 2024 09:52:23 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 179:
>>
>>> 177: * @throws IOException if reading from {@code DataInputStream}
>>> fails
>>> 178: */
>>> 179: protected static Selector readBinary(int bssVersion,
>>> DataInputStream is, String[] strings)
>>
>> This is an API addition, so will need an `@since 23`. Since you are adding
>> new API here, should it be an instance method rather than a static method?
>> Or is is called without having an instance of Selector in some cases?
>
> 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
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458762627
