JavaFX List binding #11

Merged
merged 13 commits into from Feb 1, 2016

Projects

None yet

3 participants

@hendrikebbers
Member

Adds the support for a unidirectional List binding between DP and JavaFX lists with converter support.

Review on Reviewable

hendrikebbers added some commits Dec 14, 2015
@hendrikebbers hendrikebbers JavaFX list binding be88dbb
@hendrikebbers hendrikebbers Tests c866b18
@hendrikebbers hendrikebbers ListCompare & Tests
9e1934d
@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...lphin/client/javafx/impl/DefaultJavaFXListBinder.java
+
+ private javafx.collections.ObservableList<S> list;
+
+ private static List<javafx.collections.ObservableList> boundLists = new CopyOnWriteArrayList<>();
+
+ public DefaultJavaFXListBinder(javafx.collections.ObservableList<S> list) {
+ if (list == null) {
+ throw new IllegalArgumentException("list must not be null");
+ }
+ this.list = list;
+ }
+
+ @Override
+ public <T> Binding to(ObservableList<T> dolphinList, Converter<? super T, ? extends S> converter) {
+ boundLists.forEach(addedList -> {
+ if(addedList == list) {
@netopyr
netopyr Jan 8, 2016 Collaborator

Shouldn't boundLists be a Set?

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

We can't use a Set here since the default implementations compare content by using equals. In this case we need to use == otherwise 2 lists with the same content ("A", "B", "C") would be equal and a wrong list would be removed...

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...lphin/client/javafx/impl/DefaultJavaFXListBinder.java
+ this.list = list;
+ }
+
+ @Override
+ public <T> Binding to(ObservableList<T> dolphinList, Converter<? super T, ? extends S> converter) {
+ boundLists.forEach(addedList -> {
+ if(addedList == list) {
+ throw new UnsupportedOperationException("A JavaFX list can only be bound to one Dolphin Platform list!");
+ }
+ });
+ boundLists.add(list);
+ InternalListChangeListener listChangeListener = new InternalListChangeListener(list, converter);
+ Subscription subscription = dolphinList.onChanged(listChangeListener);
+
+ dolphinList.forEach(item -> list.add(converter.convert(item)));
+ list.clear();
@netopyr
netopyr Jan 8, 2016 Collaborator

I am confused. First you add all elements and then you clear the list?

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

I'm confused, too 😄
I think it's the wrong order: first clear and then add. Maybe I don't have a unitTests with non empty lists. Will add this

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...lphin/client/javafx/impl/DefaultJavaFXListBinder.java
+ public <T> Binding to(ObservableList<T> dolphinList, Converter<? super T, ? extends S> converter) {
+ boundLists.forEach(addedList -> {
+ if(addedList == list) {
+ throw new UnsupportedOperationException("A JavaFX list can only be bound to one Dolphin Platform list!");
+ }
+ });
+ boundLists.add(list);
+ InternalListChangeListener listChangeListener = new InternalListChangeListener(list, converter);
+ Subscription subscription = dolphinList.onChanged(listChangeListener);
+
+ dolphinList.forEach(item -> list.add(converter.convert(item)));
+ list.clear();
+
+ ListChangeListener readOnlyListener = c -> {
+ if (!listChangeListener.isOnChange()) {
+ throw new UnsupportedOperationException("A JavaFX list that is bound to a dolphin list can only be modified by the binding!");
@netopyr
netopyr Jan 8, 2016 Collaborator

This would leave the list in an invalid state. I wonder if we should initialize the list at this point and set all elements from the dolphinList again.

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

Sounds like an idea. Sadly the exception is catched by the JavaFX List and therefore it makes sense to provide a better handling. I will try this.

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...in/client/javafx/impl/InternalListChangeListener.java
@@ -0,0 +1,43 @@
+package com.canoo.dolphin.client.javafx.impl;
+
+import com.canoo.dolphin.client.javafx.Converter;
+import com.canoo.dolphin.collections.ListChangeEvent;
+import com.canoo.dolphin.collections.ListChangeListener;
+
+public class InternalListChangeListener<S, T> implements ListChangeListener<T> {
@netopyr
netopyr Jan 8, 2016 Collaborator

I think this is better done as an internal class of DefaultJavaFXListBinder, because both classes access each others members anyhow.

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...in/client/javafx/impl/InternalListChangeListener.java
+ public InternalListChangeListener(javafx.collections.ObservableList<S> javaFXList, Converter<? super T, ? extends S> converter) {
+ this.converter = converter;
+ this.javaFXList = javaFXList;
+ onChange = false;
+ }
+
+ @Override
+ public void listChanged(ListChangeEvent<? extends T> e) {
+ onChange = true;
+ try {
+ for (ListChangeEvent.Change<? extends T> c : e.getChanges()) {
+ if (c.isAdded()) {
+ for (int i = c.getFrom(); i < c.getTo(); i++) {
+ javaFXList.add(i, converter.convert(e.getSource().get(i)));
+ }
+ } else if (c.isRemoved()) {
@netopyr
netopyr Jan 8, 2016 Collaborator

You could also have a replace, in which case you need to remove the old elements and replace them with the new ones.

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

Oh, I thought this is handled by add AND remove

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...in/client/javafx/impl/InternalListChangeListener.java
+ private boolean onChange;
+
+ public InternalListChangeListener(javafx.collections.ObservableList<S> javaFXList, Converter<? super T, ? extends S> converter) {
+ this.converter = converter;
+ this.javaFXList = javaFXList;
+ onChange = false;
+ }
+
+ @Override
+ public void listChanged(ListChangeEvent<? extends T> e) {
+ onChange = true;
+ try {
+ for (ListChangeEvent.Change<? extends T> c : e.getChanges()) {
+ if (c.isAdded()) {
+ for (int i = c.getFrom(); i < c.getTo(); i++) {
+ javaFXList.add(i, converter.convert(e.getSource().get(i)));
@netopyr
netopyr Jan 8, 2016 Collaborator

It is unfortunate that we cannot use addAll() here, because it would be much more efficient. Maybe we should not mix the implementation with and without converter.

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

Converter: Because of performance issues or do you see any other problem?

@netopyr netopyr commented on the diff Jan 8, 2016
...ava/com/canoo/dolphin/client/javafx/FXBinderTest.java
@@ -513,4 +516,150 @@ public void testBidirectionalChain() {
binding1.unbind();
binding3.unbind();
}
+
+ @Test
+ public void testListBinding() {
+ ObservableList<String> dolphinList = new ObservableArrayList<>();
+ javafx.collections.ObservableList<String> javaFXList = FXCollections.observableArrayList();
+
+ Binding binding = FXBinder.bind(javaFXList).to(dolphinList);
+
+ assertEquals(dolphinList.size(), 0);
+ assertEquals(javaFXList.size(), 0);
+
+ dolphinList.add("Hello");
+
+ assertEquals(dolphinList.size(), 1);
+ assertEquals(javaFXList.size(), 1);
@netopyr
netopyr Jan 8, 2016 Collaborator

I think you should check the actual content of both lists.

@netopyr
netopyr Jan 8, 2016 Collaborator

Hmmm, Hamcrest würde das einfacher machen. Oder wir probieren mal meine Assertion Library Caj. Wenn ich Zeit habe, probiere ich das Sonntag mal. :)

@netopyr netopyr commented on an outdated diff Jan 8, 2016
...ava/com/canoo/dolphin/client/javafx/FXBinderTest.java
+
+ dolphinList.add("Java");
+
+ assertEquals(dolphinList.size(), 1);
+ assertEquals(javaFXList.size(), 1);
+
+ binding.unbind();
+
+ assertEquals(dolphinList.size(), 1);
+ assertEquals(javaFXList.size(), 1);
+
+ dolphinList.add("Duke");
+
+ assertEquals(dolphinList.size(), 2);
+ assertEquals(javaFXList.size(), 1);
+ }
@netopyr
netopyr Jan 8, 2016 Collaborator

I do not think that this test is sufficient. You need to make sure that you get the indexes right. I usually do as a minimum add/remove/replace in the beginning, middle, and end of the lists. Plus everything should be tested with one and with more than one element in the change.

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...ava/com/canoo/dolphin/client/javafx/FXBinderTest.java
+ binding2.unbind();
+ binding3.unbind();
+ binding4.unbind();
+ }
+
+
+ @Test
+ public void testErrorOnMultipleListBinding() {
+ ObservableList<String> dolphinList = new ObservableArrayList<>();
+ ObservableList<String> dolphinList2 = new ObservableArrayList<>();
+ javafx.collections.ObservableList<String> javaFXList = FXCollections.observableArrayList();
+
+ FXBinder.bind(javaFXList).to(dolphinList);
+ try {
+ FXBinder.bind(javaFXList).to(dolphinList2);
+ fail("A JavaFX list can only be bound to one Dolphin list");
@netopyr
netopyr Jan 8, 2016 Collaborator

You should work with an expected Exception instead.

@netopyr netopyr commented on the diff Jan 8, 2016
...ava/com/canoo/dolphin/client/javafx/FXBinderTest.java
+
+ @Test
+ public void testConvertedListBinding() {
+ ObservableList<Boolean> dolphinList = new ObservableArrayList<>();
+ javafx.collections.ObservableList<String> javaFXList = FXCollections.observableArrayList();
+
+ Binding binding = FXBinder.bind(javaFXList).to(dolphinList, value -> value.toString());
+
+ dolphinList.add(true);
+
+ assertEquals(dolphinList.size(), 1);
+ assertEquals(javaFXList.size(), 1);
+
+ assertEquals(javaFXList.get(0), "true");
+
+ }
@netopyr
netopyr Jan 8, 2016 Collaborator

I think we should also add tests to check what happens if one tries to modify a bound JavaFX-list and a test for the case that the converter throws an Exception.

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

I think that JavaFx catches this exception but I will have a look.

@netopyr netopyr and 1 other commented on an outdated diff Jan 8, 2016
...lphin/client/javafx/impl/DefaultJavaFXListBinder.java
+ }
+
+ @Override
+ public void listChanged(ListChangeEvent<? extends T> e) {
+ onChange = true;
+ try {
+ for (ListChangeEvent.Change<? extends T> c : e.getChanges()) {
+ if (c.isAdded()) {
+ for (int i = c.getFrom(); i < c.getTo(); i++) {
+ javaFXList.add(i, converter.convert(e.getSource().get(i)));
+ }
+ } else if (c.isRemoved()) {
+ final int index = c.getFrom();
+ javaFXList.remove(index, index + c.getRemovedElements().size());
+ }
+ }
@netopyr
netopyr Jan 8, 2016 Collaborator

Unfortunately GitHub swallowed your last comment. Replace can be handled by add and remove. But because of the "else", you will not do the remove part here. Also the order would be wrong, I think. You need to make sure that you do not remove the new elements accidentally.

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

make sense :)

@hendrikebbers
hendrikebbers Jan 8, 2016 Member

This means that the right order would be like this?
if(c.isReplaced()) {
...
} else if (c.isAdded()) {
...
} else if (c.isRemoved()) {
...
}

@netopyr
netopyr Jan 8, 2016 Collaborator

You can probably also do:
if (c.isRemoved()) {
...
}
if (c.isAdded()) {
...
}
i.e. if you remove the else and change the order, it should be fine. But to be sure, I would start with the unit-test.

@netopyr netopyr commented on the diff Jan 8, 2016
...lphin/client/javafx/impl/DefaultJavaFXListBinder.java
+ List boundList = boundLists.get(i);
+ if(boundList == list) {
+ boundLists.remove(i);
+ break;
+ }
+ }
+ };
+ }
+
+ private class InternalListChangeListener<S, T> implements com.canoo.dolphin.collections.ListChangeListener<T> {
+
+ private final javafx.collections.ObservableList<S> javaFXList;
+
+ private final Converter<? super T, ? extends S> converter;
+
+ private boolean onChange;
@netopyr
netopyr Jan 8, 2016 Collaborator

I think if you turn this into a member of the outer class, you can define the whole listener as a lambda.

@hendrikebbers hendrikebbers IdentityHashMap for Listeners
d3af6f4
@aalmiray
Contributor

Review status: 0 of 4 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/FXBinder.java, line 29 [r8] (raw file):
should this class be made final?


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/FXBinder.java, line 34 [r8] (raw file):
none all of these methods check their arguments might be null


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/impl/DefaultJavaFXListBinder.java, line 17 [r8] (raw file):
why make this field static? Also, this class can keep references forever unless explicitly removed; consider using WeakReference.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/JavaFXListBinder.java, line 8 [r8] (raw file):
consider using Objects.requireNonNull or create an utility class that can handle this an other assertions


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: 0 of 4 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/impl/DefaultJavaFXListBinder.java, line 17 [r8] (raw file):
The static map is used to hold all lists that are currently bound. By doing so you can't bind a JavaFX list to more than one Dolphin list. Since you can have many instances of DefaultJavaFXListBinder the field must be static.
About WeakRef: Yes, you are right. It was a WeakHashMap some time ago and based on the equals Problem when working with lists I changed it and lost the weak feature. Will refactor this.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/JavaFXListBinder.java, line 8 [r8] (raw file):
I will create a Utility class and update the request.


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: 0 of 4 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/JavaFXListBinder.java, line 8 [r8] (raw file):
Done.


Comments from the review on Reviewable.io

@aalmiray
Contributor

Reviewed 2 of 5 files at r1, 1 of 2 files at r2, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/impl/DefaultJavaFXListBinder.java, line 17 [r8] (raw file):
About WeakRef:
That is not a problem. All lists that are in the map are bound to a model. By doing so they are referenced by the model. You can remove this reference only by unbinding the lists. By doing so the list will be removed from the Map. Maybe we can find a bette concept in future that work more weak in general but with the current concept the reference in the lists isn't a problem as far as I can see it.


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.


java-client-javafx/src/main/java/com/canoo/dolphin/client/javafx/impl/DefaultJavaFXListBinder.java, line 69 [r7] (raw file):
I thin this will make the code more complex


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers added this to the 0.8.0 milestone Jan 22, 2016
@hendrikebbers hendrikebbers Merge branch 'master' into list-binding
# Conflicts:
#	java-client-javafx/src/test/java/com/canoo/dolphin/client/javafx/FXBinderTest.java
cf3a96d
@hendrikebbers
Member

Review done. I think we can merge this request.

@hendrikebbers hendrikebbers Merge branch 'master' into list-binding
# Conflicts:
#	java-client-javafx/src/test/java/com/canoo/dolphin/client/javafx/FXBinderTest.java
89a4245
@hendrikebbers hendrikebbers merged commit 160374b into master Feb 1, 2016

3 of 4 checks passed

code-review/reviewable Review in progress: 1 of 5 files reviewed, 13 unresolved discussions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.5%) to 16.732%
Details
@hendrikebbers hendrikebbers deleted the list-binding branch Feb 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment