On Tue, 11 Mar 2025 02:34:50 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> That doesn't seem like something you should be adding to the docs I think? > > Up to you. I thought it would make it easier on anyone learning the > implementations if they understood why 2 are needed (one that stores the > value and one that relies on the value being stored elsewhere). I can always put a comment outside the docs. I think the docs (in `OldValueCachingListenerManager`) are quite clear already though why you'd use one or the other. Note that in an ideal world, we would never need `OldValueCachingListenerManager`. I mean, it is currently used for **properties** the exact thing you'd expect to have a readily available old value. However, a big design flaw in properties makes it impossible to do this: The design flaw is the **use** of a protected `fireValueChangedEvent` method within the property, that has an **implementation** and is **not final**. This is poor design, because: - You can't change the implementation, as subclasses may call `super.fireValueChangedEvent` - You can't change **when** you call it, as subclasses may be overriding it to be "aware" of changes - You can't even provide the default implementation somewhere else, then call `fireValueChangedEvent` as subclasses may be purposely disabling or altering the default implementation So, theoretically, I can **easily** provide the old value for properties, but it means I have to do one of the following: - Execute the default implementation (but now with old value support) regardless of whether `fireValueChangedEvent` was overridden -- would break code that overrides this method to block the default implementation - Change the signature of `fireValueChangedEvent` to accept a `T oldValue` -- can't do that, its `protected` - Modify the default implementation to fetch the old value from somewhere to provide it -- can't do that as subclasses may be calling it at random, and they won't have the mechanism in place to provide the old value via something other than a parameter You should never do all three of these for protected methods, as it makes the providing class fragile and tightly coupled with subclasses: - Provide an implementation in a protected method. - Make it non-final. - Call it internally when its implementation is essential for correct operation. Doing only two of these is fine: - Providing an implementation and making it overridable is fine if the base class doesn't rely on it (i.e., it's just a helper). - Providing an implementation that is final and using it internally is fine because subclasses can't alter the base class's behavior. - Calling a non-final protected method with no expected implementation in the base class is also fine. If we choose to address this at some point, to make properties more performant (with regards to listeners) and use less memory (when using a single change listener) it would mean a backwards incompatible change. The change will mostly affect FX classes (as they rely on being able to both call `fireValueChangedEvent` as well as being able to override it) but it may affect 3rd parties as well as they can call and/or override it as well. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1988542989
