On Wed, 19 Oct 2022 20:14:18 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> @mstr2 : >> `getProperties()` is a perfectly fine way of storing a property instance, >> especially considering the fact that this property will be called exactly >> once - during the fluent pipeline setup. Of course, it does not apply to >> other properties that are being called all the time. >> >> See ButtonBarSkin:136 >> >> A separate ticket for optimizing property storage might be a good idea, I >> agree. > >> > `getProperties()` isn't the place for this. >> >> can you explain why? > > This map is used for storing values for the node. As the API note says, > layout managers use this to store constraints that be updated by the user. I > think that this is already not the best way to do it. The user can also store > arbitrary data like markers or other associations. Storing a `Property` is > like storing functionality, not data. > > In addition, the map is publicly accessible, so the user can clear it. In > this case you risk GC of the instance. This is equivalent to making a > property field both `public` and not `final`, allowing the user to overwrite > the reference - a rather disastrous scenario. Thank you. You do bring a good point - clearing the map. There is some mild language warning about clearing the map in javadoc already, but perhaps `Node.getProperties().clear()` should be forbidden (throw an IOE?). Also, the devs risk unexpected side effects if they try to use Strings as keys, due to possible collision with keys used by FX internals. And, of course, the API does not preclude storing properties in `getProperties()`, and FX is already doing it, as illustrated by ButtonBarSkin:136 (perhaps more, I did not perform an exhaustive check). ------------- PR: https://git.openjdk.org/jfx/pull/830
