Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8089557

bindBidirection works for ReadOnly*Wrapper incorrectly

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: P3
    • Resolution: Fixed
    • Affects Version/s: 8u31
    • Fix Version/s: 9
    • Component/s: javafx
    • Environment:

      java version "1.8.0_31"
      Java(TM) SE Runtime Environment (build 1.8.0_31-b13)
      Java HotSpot(TM) Client VM (build 25.31-b07, mixed mode)

    • Subcomponent:

      Backports

        Description

        ReadOnlyStringWrapper a = new ReadOnlyStringWrapper("a");
        ReadOnlyStringWrapper b = new ReadOnlyStringWrapper("b");

        b.bindBidirectional(a);
        //a.bindBidirectional(b); //only this can fix it

        a.set("a1"); // a == "a1",b == "a1"
        b.set("b1"); // b == "b1", a == "a1" (!!!)

        a.set("a2"); // a == "a2",b == "a2"
        b.set("b2"); // b == "b2", a == "a2" (!!!)

          Issue Links

            Activity

            Hide
            vadim Vadim Pakhnushev (Inactive) added a comment -
            BidirectionalBinding makes an assumption that the property passed in the changed method is the same as was used in the addListener, but the ReadOnly*Wrapper actually pass all calls to the ReadOnlyPropertyImpl.
            So the source of the changed() call is neither property1 nor property2 and the binding is only works from property2 to property1 by accident.
            Show
            vadim Vadim Pakhnushev (Inactive) added a comment - BidirectionalBinding makes an assumption that the property passed in the changed method is the same as was used in the addListener, but the ReadOnly*Wrapper actually pass all calls to the ReadOnlyPropertyImpl. So the source of the changed() call is neither property1 nor property2 and the binding is only works from property2 to property1 by accident.
            Hide
            vadim Vadim Pakhnushev (Inactive) added a comment - - edited
            Chien, please review this fix:
            http://cr.openjdk.java.net/~vadim/8089557/webrev.00/

            ReadOnlyWrappers were simplified so they do not forward add/removeListener calls to the ReadOnlyProperty.
            This actually make much more sense, so you can attach distinct listeners to any property and they will receive synchronized events.
            This is checked in the existing tests, so they will fail if fireValueChangedEvent do not pass the event to either super or readOnlyProperty.
            Changes in the ReadOnlyWrapperTests reflect the fact that listener will get respective observable.
            BidirectionalBindingTest was greatly simplified and ReadOnlyWrappers were added to the parameters.
            Show
            vadim Vadim Pakhnushev (Inactive) added a comment - - edited Chien, please review this fix: http://cr.openjdk.java.net/~vadim/8089557/webrev.00/ ReadOnlyWrappers were simplified so they do not forward add/removeListener calls to the ReadOnlyProperty. This actually make much more sense, so you can attach distinct listeners to any property and they will receive synchronized events. This is checked in the existing tests, so they will fail if fireValueChangedEvent do not pass the event to either super or readOnlyProperty. Changes in the ReadOnlyWrapperTests reflect the fact that listener will get respective observable. BidirectionalBindingTest was greatly simplified and ReadOnlyWrappers were added to the parameters.
            Hide
            ckyang Chien Yang added a comment -
            +1. Looks good to me.
            Show
            ckyang Chien Yang added a comment - +1. Looks good to me.
            Hide
            vadim Vadim Pakhnushev (Inactive) added a comment -
            Changeset: 155ad1e34c44
            Author: vadim
            Date: 2015-08-19 13:46 +0300
            URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/155ad1e34c44
            Show
            vadim Vadim Pakhnushev (Inactive) added a comment - Changeset: 155ad1e34c44 Author: vadim Date: 2015-08-19 13:46 +0300 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/155ad1e34c44
            Hide
            kcr Kevin Rushforth added a comment -
            @Vadim: is there a chance of a user-visible behavioral regression with this fix? It seems safe enough, but since this bug isn't a regression, I would hate to fix it only to introduce a bug.
            Show
            kcr Kevin Rushforth added a comment - @Vadim: is there a chance of a user-visible behavioral regression with this fix? It seems safe enough, but since this bug isn't a regression, I would hate to fix it only to introduce a bug.
            Hide
            vadim Vadim Pakhnushev (Inactive) added a comment -
            Kevin, the only visible change of behavior I could think of is that observable value passed to the listener's changed method will be different.
            That is, before the fix this value would be equal to the value returned from the getReadOnlyProperty and after the fix it will be equal to the wrapper property itself.
            While this could theoretically break someone's code I think it's better to have this value equals to the observable value to which this listener was added after all.
            Show
            vadim Vadim Pakhnushev (Inactive) added a comment - Kevin, the only visible change of behavior I could think of is that observable value passed to the listener's changed method will be different. That is, before the fix this value would be equal to the value returned from the getReadOnlyProperty and after the fix it will be equal to the wrapper property itself. While this could theoretically break someone's code I think it's better to have this value equals to the observable value to which this listener was added after all.
            Hide
            kcr Kevin Rushforth added a comment -
            I agree.

            Approved to backport to 8u-dev.
            Show
            kcr Kevin Rushforth added a comment - I agree. Approved to backport to 8u-dev.
            Hide
            kcr Kevin Rushforth added a comment -
            Covered by the following unit tests:

            com/sun/javafx/binding/BidirectionalBindingTest
            javafx/beans/property/ReadOnlyBooleanWrapperTest
            javafx/beans/property/ReadOnlyDoubleWrapperTest
            javafx/beans/property/ReadOnlyFloatWrapperTest
            javafx/beans/property/ReadOnlyIntegerWrapperTest
            javafx/beans/property/ReadOnlyListWrapperTest
            javafx/beans/property/ReadOnlyLongWrapperTest
            javafx/beans/property/ReadOnlyMapWrapperTest
            javafx/beans/property/ReadOnlyObjectWrapperTest
            javafx/beans/property/ReadOnlySetWrapperTest
            javafx/beans/property/ReadOnlyStringWrapperTest
            Show
            kcr Kevin Rushforth added a comment - Covered by the following unit tests: com/sun/javafx/binding/BidirectionalBindingTest javafx/beans/property/ReadOnlyBooleanWrapperTest javafx/beans/property/ReadOnlyDoubleWrapperTest javafx/beans/property/ReadOnlyFloatWrapperTest javafx/beans/property/ReadOnlyIntegerWrapperTest javafx/beans/property/ReadOnlyListWrapperTest javafx/beans/property/ReadOnlyLongWrapperTest javafx/beans/property/ReadOnlyMapWrapperTest javafx/beans/property/ReadOnlyObjectWrapperTest javafx/beans/property/ReadOnlySetWrapperTest javafx/beans/property/ReadOnlyStringWrapperTest

              People

              • Assignee:
                vadim Vadim Pakhnushev (Inactive)
                Reporter:
                duke J. Duke (Inactive)
              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Imported: