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

ListPropertyBase: must not fire changeEvent on modifying the list

    Details

    • Subcomponent:

      Description

      The test below is failing. I would expect:

      - to receive a changeEvent on changing the identity of the list
      - to receive a listChangeEvent on both modifying the list and changing the identity of the list
      - to not receive a changeEvent on modifying a list

      first two expectations seem to be met (as experimented so far) but not the last. Not entirely sure, though, it the expectation is correct. ListProperty isn't used at all internally in the controls related code (which is a pity, would make listening to collection-values properties much cleaner), so no examples to learn from)

          @Test
          public void testListPropertyChangeNotificationOnRemoveItem() {
              ObservableList<String> list = createObservableList(true);
              ListProperty<String> listProperty = new SimpleListProperty<>(list);
              ChangeReport report = new ChangeReport(listProperty);
              list.remove(0);
              assertEquals("listProperty must not fire changeEvent on removing item", 0, report.getEventCount());
          }
          

        Issue Links

          Activity

          Hide
          kcr Kevin Rushforth added a comment -
          Adding Martin who can comment, but I suspect that we will not change the behavior.
          Show
          kcr Kevin Rushforth added a comment - Adding Martin who can comment, but I suspect that we will not change the behavior.
          Hide
          kleopatrajfx Kleopatra (Inactive) added a comment -
          @Kevin

          why do you suspect so? Any reference to this being specified/intended behavior?
          Show
          kleopatrajfx Kleopatra (Inactive) added a comment - @Kevin why do you suspect so? Any reference to this being specified/intended behavior?
          Hide
          kcr Kevin Rushforth added a comment -
          In general changing a behavior like this has to be done for a good reason, since applications may be depending on the current behavior.
          Show
          kcr Kevin Rushforth added a comment - In general changing a behavior like this has to be done for a good reason, since applications may be depending on the current behavior.
          Hide
          kleopatrajfx Kleopatra (Inactive) added a comment -
          well, I reported this because I think the current behaviour is a bug and
          violates its contract as ObservableValue. Api doc of addListener:

          "Adds a ChangeListener which will be notified whenever the value of the ObservableValue changes"

          in negation: no notification if the value is the same.

          Specifically, I would expect that change notification of ObjectProperty<ObservableList<>> is
          the same as ListProperty<>.

           ... maybe that's arguable (identity vs. equality) and my expectation is wrong,
           then I would like to hear/read the arguments somewhere.
          Show
          kleopatrajfx Kleopatra (Inactive) added a comment - well, I reported this because I think the current behaviour is a bug and violates its contract as ObservableValue. Api doc of addListener: "Adds a ChangeListener which will be notified whenever the value of the ObservableValue changes" in negation: no notification if the value is the same. Specifically, I would expect that change notification of ObjectProperty<ObservableList<>> is the same as ListProperty<>.  ... maybe that's arguable (identity vs. equality) and my expectation is wrong,  then I would like to hear/read the arguments somewhere.
          Hide
          msladecek Martin Sládeček added a comment -
          I'm not the original author of this API, so I don't know why it was done this way. It's in the tests, so it was probably designed this way. But Kleopatra is right that it violates the contract. In the change event, both old value and new value points to the same list, so listening on that event might be a pitfall as the old value does not represent the state of the list before the change. So the other options we have here is to create a new list with the list content before the change or do not send change events at all in this case. The first option might be expensive and it would look like the list object changed too (which is not true) so I would lean towards the latter, which corresponds to javadoc and therefore what users would expect.

          As Kleopatra mentioned, we don't use this API anywhere (it was introduced in 2.1, so most of the controls already used ObjectProperty<ObservableList<T>>), so the scope of the change is very limited.

          So I think we should do it ( + the same for SetProperty and MapProperty ! )
          Show
          msladecek Martin Sládeček added a comment - I'm not the original author of this API, so I don't know why it was done this way. It's in the tests, so it was probably designed this way. But Kleopatra is right that it violates the contract. In the change event, both old value and new value points to the same list, so listening on that event might be a pitfall as the old value does not represent the state of the list before the change. So the other options we have here is to create a new list with the list content before the change or do not send change events at all in this case. The first option might be expensive and it would look like the list object changed too (which is not true) so I would lean towards the latter, which corresponds to javadoc and therefore what users would expect. As Kleopatra mentioned, we don't use this API anywhere (it was introduced in 2.1, so most of the controls already used ObjectProperty<ObservableList<T>>), so the scope of the change is very limited. So I think we should do it ( + the same for SetProperty and MapProperty ! )
          Hide
          vadim Vadim Pakhnushev added a comment -
          Apparently users expect change events to be sent when the state of the collection is changed, see JDK-8088928 and JDK-8169052 for example.
          Show
          vadim Vadim Pakhnushev added a comment - Apparently users expect change events to be sent when the state of the collection is changed, see JDK-8088928 and JDK-8169052 for example.

            People

            • Assignee:
              Unassigned
              Reporter:
              fastegal Jeanette Winzenburg
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Imported: