Fix MappedBackedList handling of wasUpdated changes

[LoayGhreeb]

This PR addresses an issue related to EasyBind#mapBacked. The problem arises when using a SortedList with elements that are mapped using EasyBind#mapBacked. Specifically, updates to elements in the list do not propagate to the SortedList.

Minimal Example:

import javafx.beans.Observable;
import javafx.beans.property.IntegerProperty;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.collections.transformation.SortedList;

import com.tobiasdiez.easybind.EasyBind;

public class Main {
    public static void main(String[] args) {
        ObservableList<IntegerProperty> list = FXCollections.observableArrayList(
                number -> new Observable[] {number}
        );
        ObservableList<Integer> mappedList = EasyBind.mapBacked(list, IntegerProperty::get);
        SortedList<Integer> sortedList = new SortedList<>(mappedList);

        IntegerProperty number = new SimpleIntegerProperty(1);
        list.add(number);

        System.out.println("mappedList: " + mappedList);
        System.out.println("sortedList: " + sortedList);

        number.set(2);

        System.out.println("mappedList: " + mappedList);
        System.out.println("sortedList: " + sortedList);
    }
}

Output:

mappedList: [1]
sortedList: [1]
mappedList: [2]
sortedList: [1]

Expected Output:

mappedList: [1]
sortedList: [1]
mappedList: [2]
sortedList: [2]

The issue is due to the SortedList#sourceChanged method in JavaFX. If there is no custom comparator, SortedList#updateUnsorted will handle the updates, but it does not handle the wasUpdated change. This results in the SortedList not updating when an element's change type is wasUpdated.

Reference to the code:
SortedList#sourceChanged
SortedList#updateUnsorted

The fix involves using nextSet, which is equivalent to calling nextRemove() followed by nextAdd(). This ensures that updates to elements are correctly propagated through to the SortedList.

[tobiasdiez]

Thanks, could you please also add a test verifying that this is indeed fixed (and will not be reintroduced in the future).

[LoayGhreeb]

Thanks, could you please also add a test verifying that this is indeed fixed (and will not be reintroduced in the future).

Done in 5ee12b2 (#87)

[Siedlerchr]

It would be great if you could merge this and release a new version (olly is currently on holidays and I don't have admin rights on the namespace at sonatype so we cannot publish it under jabref namespace)

Edit// For releasing you can take a look at the afterburner gradle https://github.com/JabRef/afterburner.fx/blob/main/build.gradle

[LoayGhreeb]

@tobiasdiez, @Siedlerchr

In some cases, I don't want to create a new object on update because I already have bindings that reflect the updates in the mapped object.

How about introducing a new method:
mapBacked(ObservableList<A> source, Function<A, B> mapper, boolean mapOnUpdate)

The changes to MappedBackedList would be as follows:

if (change.wasUpdated()) {
    if (mapOnUpdate) {
        E old = backingList.set(change.getFrom(), mapper.apply(getSource().get(change.getFrom())));
        nextSet(change.getFrom(), old);
    } else {
        nextUpdate(change.getFrom());
    }
}

If you agree with this, should I do this in a separate PR?

---

Edit: The method should be like this, assuming the SortedList issue will be fixed by JavaFX:

if (change.wasUpdated()) {
  if(mapOnUpdate) {
      backingList.set(change.getFrom(), mapper.apply(getSource().get(change.getFrom())));
  }
  nextUpdate(change.getFrom());
}

[tobiasdiez]

I thought a bit more about this, but don't really have time right now to deep-dive into it. So please excuse any mistakes in the following and please do point them out.

The issue is due to the SortedList#sourceChanged method in JavaFX. If there is no custom comparator, SortedList#updateUnsorted will handle the updates, but it does not handle the wasUpdated change. This results in the SortedList not updating when an element's change type is wasUpdated.

So this is actually a bug in SortedList that we workaround here, right? Would it then not be "better" to create a PR to JavaFX adding the handling of updated in the unsorted case? I'm fine with merging this PR (as JabRef is the main consumer anyway) as a temporary hack, but the solution with removing and adding does have a greater overhead. Perhaps it would work to simply specifier a custom comparator in JabRef for the time being?

Or is there any other use case for calling nextSet instead of nextUpdate?

[Siedlerchr]

@LoayGhreeb You can now open this PR against https://github.com/JabRef/EasyBind I have setup the publishing

[LoayGhreeb]

So this is actually a bug in SortedList that we workaround here, right? Would it then not be "better" to create a PR to JavaFX adding the handling of updated in the unsorted case?

Initially, I thought the issue was that SortedList doesn't handle updates when there is no comparator. However, after more debugging, I realized this isn't the actual reason.

The actual issue is that SortedList treats wasUpdate changes as changes in the object's properties, not as the object itself being replaced.

Also, if the list has a comparator and an update change is fired, it resorts the list without considering the new object. Therefore, it might be better to create a PR for JavaFX because, as you mentioned, using nextSet will have a greater overhead.

I'm fine with merging this PR (as JabRef is the main consumer anyway) as a temporary hack, but the solution with removing and adding does have a greater overhead. Perhaps it would work to simply specifier a custom comparator in JabRef for the time being?

In the case of JabRef, we don't need to create a new object on change, so I suggested adding a new method: #87 (comment). If we add this method, JabRef will work fine.

The issue should be reported to JavaFX for cases where a new object is mapped on update.

[LoayGhreeb]

I see that ObservableList#set uses nextSet, and it is very similar to our case. Why did they not use nextUpdate in that case? Should we follow their approach and only use nextSet, or should we ask the JavaFX community?

I tried to fix the SortedList. See this commit: LoayGhreeb@c6d4088. It is working, tested with: LoayGhreeb@c15156e. But I'm not sure if this fix is really needed.

I think nextUpdate was added to handle changes in the object properties only, and it is not expected that nextUpdate changes the object itself.

From the observableArrayList documentation:

Creates a new empty ObservableList that is backed by an array list and listens to changes in observables of its items.
These observables are listened for changes and the user is notified of these through an ListChangeListener.Change#wasUpdated() change of an attached ListChangeListener. These changes are unrelated to the changes made to the observable list itself using methods such as add and remove.
For example, a list of Shapes can listen to changes in the shapes' fill property.