On Fri, 25 Jun 2021 23:17:10 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> 1. `FocusPropertyBase` overrides `set` (which is unusual). The current > implementation (of `FocusedProperty `) has some custom logic as well, but it > didn't try to use the set method of a writable property. Also, you use a > writable property and don't override `invalidated`. Can you say why you made > the changes you made? Technically, it doesn't _override_ `set`. `FocusPropertyBase` extends `ReadOnlyBooleanPropertyBase`, and also defines a _new_ `set` method. As implemented, setting the value is completely decoupled from notifying listeners, which leads me directly to your next question... > 2. I'm curious about the approach for notifying listeners. It seems similar > to what is currently done for `focusedProperty`, but there are differences. > Also, I'm not sure that the two newly added properties need to handle > notifications in the same way as focused. I can think of at least one > potential issue. If focus shifts from one node to another in the same parent, > you might end up notifying a parent's listeners even though its `focusWithin` > is unchanged. `FocusPropertyBase.set` just records the new value, and the decision of whether to notify listeners is deferred until after all focus-related properties have been set on the current node and any of its parents. In the scenario you describe, the recorded value of `focusWithin` is changed twice, but ends up being the same value as before. In this case, `FocusPropertyBase.notifyListeners` will _not_ fire a change notification. There's a test for this scenario: `FocusTest.testFocusWithinListenerIsNotInvokedIfPropertyDidNotEffectivelyChange`. > 3. I think the way you are propagating `focusWithin` might not work if nodes > are added or removed from the scene graph. Good point, I'll try to come up with some test cases to see if there are any issues. > 4. Normally a read-only property that needs to be settable by the > implementation would use `ReadOnlyBooleanWrapper`. And then the public > property method would call `getReadOnlyProperty` to return the read-only > property to the user. The implementation would access the wrapper and you > don't need to cast. You could consider having `FocusPropertyBase` extend > `ReadOnlyBooleanWrapper`. That could certainly be an option, although it would increase the memory footprint of `Node`. I'm thinking instead to eagerly instantiate all three focus-related properties. In this case, the property field could be referenced directly, which would also remove the need for most casts. I think that having the same deferred-notification property implementation for all three focus-related properties is the best approach, since the implementation guarantees that listeners are only notified after all changes are committed. At least, I see no upside to _not_ guarantee this. Maybe the atomicity guarantee for `focused`, `focusVisible` and `focusWithin` should be included in the documentation. ------------- PR: https://git.openjdk.java.net/jfx/pull/475
