Open LoayGhreeb opened 2 months ago
Thanks, could you please also add a test verifying that this is indeed fixed (and will not be reintroduced in the future).
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)
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
@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());
}
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
?
@LoayGhreeb You can now open this PR against https://github.com/JabRef/EasyBind I have setup the publishing
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 ofupdated
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: https://github.com/tobiasdiez/EasyBind/pull/87#issuecomment-2242391083. 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.
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: https://github.com/LoayGhreeb/EasyBind/commit/c6d40889144479c0f1d2b4ae0bd1842365071d86. It is working, tested with: https://github.com/LoayGhreeb/EasyBind/commit/c15156e2856c6ce00ae350cf4b81c70828aa4cd9. 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 anListChangeListener.Change#wasUpdated()
change of an attachedListChangeListener
. These changes are unrelated to the changes made to the observable list itself using methods such asadd
andremove
. For example, a list ofShape
s can listen to changes in the shapes'fill
property.
This PR addresses an issue related to
EasyBind#mapBacked
. The problem arises when using aSortedList
with elements that are mapped usingEasyBind#mapBacked
. Specifically, updates to elements in the list do not propagate to theSortedList
.Minimal Example:
Output:
Expected Output:
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 thewasUpdated
change. This results in theSortedList
not updating when an element's change type iswasUpdated
.Reference to the code: SortedList#sourceChanged SortedList#updateUnsorted
The fix involves using
nextSet
, which is equivalent to callingnextRemove()
followed bynextAdd()
. This ensures that updates to elements are correctly propagated through to theSortedList
.