Bug ID: JDK-8092401 Change BehaviorSkinBase registerChangeListener to use Callback functional interface rather than strings

Type: Enhancement
Component: javafx
Sub-Component: controls
Affected Version: 8u20

Priority: P4
Status: Closed
Resolution: Not an Issue

Submitted: 2014-03-06
Updated: 2022-06-28
Resolved: 2022-06-28

Even though what to do with BehaviorSkinBase is up for debate, we can make it marginally better by replacing:

-    protected final void registerChangeListener(ObservableValue<?> property, String reference) {
+    protected final void registerChangeListener(ObservableValue<?> property, Callback<ObservableValue<?>, Void> callback) {

This would do away with the monolithic handleControlPropertyChanged and allow for use of lambda's. Besides the novelty and fun of using lambda, this would make for cleaner skin code. For example, in LabeledSkinBase, instead of 

        registerChangeListener(labeled.fontProperty(), "FONT");

with the big if-then-else block that is handleControlPropertyChanged, we'd have 

        registerChangeListener(labeled.fontProperty(), (observable) -> {
            textMetricsChanged();
            invalidateWidths();
            ellipsisWidth = Double.NEGATIVE_INFINITY;
        }

Attached patch.patch which is a quick pass at the changes that would be made in BehaviorSkinBase and MultiplePropertyChangeListenerHandler

Yep, can be closed.
28-06-2022
JEP 253 introduced SkinBase#registerChangeListener(ObservableValue<?> property, Consumer<ObservableValue<?>> consumer) method so I think this issue can be closed.
24-02-2016
a step into the right direction, but should go further: you are still throwing away all information that comes with the ChangeEvent. That might be good enough for "simple" properties which might be the major use-case that simplified registration was intended for. It gets critical if the real interest is in a "child" property of the property's value: such context requires re-wiring listeners on that child property if the parent value changes. Without receiving the root value in notification, the skin needs to keep an alias to the root value. Which indeed are scattered across the skin code. In a code cleanup strike, maybe the usage of this simplified registration should be discouraged (and removed) for complexer notification requirements. Itchy lines to look for are something like (these are from ChoiceBoxSkin, but can be found in others as well) // here the real interest is in the list changes of the value, need to listen to items // to re-wire the listChangeListener registerChangeListener(control.itemsProperty(), "ITEMS"); // here the real interest is in the selectedItem property of the value registerChangeListener(control.selectionModelProperty(), "SELECTION_MODEL"); // this is plain wrong ... keeps listening to old value registerChangeListener(control.getSelectionModel().selectedItemProperty(), "SELECTION_CHANGED");
07-10-2014
@Steve - there is more to this change than just the patch I posted. There are some 134 uses (according to idea) of BehaviorSkinBase registerChangeListener. While most are probably trivial, I worry that some may not be. I don't think this is something we should take on in 8u20.
07-03-2014
Response from Brian G. This is a simple question with a slightly messy answer. Asymptotically, lambdas are much smaller than inner classes. A lambda maps to one method plus one invokedynamic call site, whereas an inner class maps to an entire class plus a constructor call site. The method is dramatically shorter than the class. However, if you wrote a simple test class with exactly one lambda or inner class instance and nothing else, you would find the size difference to be smaller than you expect. That's because there's a constant overhead of 100-200 bytes if you have any lambdas, due to the relatively long signatures of the invokedynamic bootstraps involved. So, in the worst case of one lambda per file, you won't win by much switching from inner classes to lambdas, but if you've got more typical code with dozens of inner classes in a file, you'll win pretty big switching to lambdas.
06-03-2014
If that is the case then it sounds better - but I thought that invokedynamic lambda's weren't enabled yet.
06-03-2014
The compiler does lambdas with invokedynamic. I'm not sure what that does to the static footprint. I'll fire off an email to Brian G.
06-03-2014
In general I have no problem with changes here, although I do worry about the static footprint explosion from creating a bunch more lambdas (which at present I believe are still represented as inner classes when compiled). What do you reckon?
06-03-2014