Fix a bug with different-flag with sets

sialcasa · Nov 7, 2018 · 6dddbe1 · 6dddbe1
1 parent 0065413
commit 6dddbe1
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 12 deletions.
diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/mapping/BeanSetPropertyField.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/mapping/BeanSetPropertyField.java
@@ -97,6 +97,6 @@ public boolean isDifferent(M wrappedObject) {
 		final Set<E> modelValue = getter.apply(wrappedObject);
 		final Set<E> wrapperValue = targetProperty;
 
-		return !Objects.equals(modelValue, wrappedObject);
+		return !Objects.equals(modelValue, wrapperValue);
 	}
 }
diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/mapping/FxSetPropertyField.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/mapping/FxSetPropertyField.java
@@ -90,6 +90,6 @@ public boolean isDifferent(M wrappedObject) {
 		final Set<E> modelValue = accessor.apply(wrappedObject).getValue();
 		final Set<E> wrapperValue = targetProperty;
 
-		return !Objects.equals(modelValue, wrappedObject);
+		return !Objects.equals(modelValue, wrapperValue);
 	}
 }
diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/mapping/ModelWrapper.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/mapping/ModelWrapper.java
@@ -52,7 +52,9 @@
 import de.saxsys.mvvmfx.utils.mapping.accessorfunctions.StringPropertyAccessor;
 import de.saxsys.mvvmfx.utils.mapping.accessorfunctions.StringSetter;
 
+import java.util.Collection;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -82,6 +84,7 @@
 import javafx.beans.property.SimpleStringProperty;
 import javafx.beans.property.StringProperty;
 import javafx.collections.FXCollections;
+import javafx.collections.ObservableSet;
 
 
 /**
@@ -1462,31 +1465,44 @@ public <E> ListProperty<E> field(String identifier, ListPropertyAccessor<M, E> a
 
 	/* Field type set */
 
+	/**
+	 * This helper method is needed because there is no equivalent of {@link FXCollections#observableArrayList(Collection)}
+	 * for {@link Set}.
+	 * The only factory method available for sets is {@link FXCollections#observableSet(Set)} which creates an observable set
+	 * that is backed by the given set. However, this would mean that changes to the observable set are directly synchronized back to the
+	 * source set. In contrast to this {@link FXCollections#observableArrayList(Collection)} creates an observable list that has
+	 * initially all values of the source list but changes aren't synchronized back because internally a new ArrayList is created.
+	 * We need exactly this behaviour for Sets and therefore are using this helper method.
+	 */
+	private static <T> ObservableSet<T> observableHashSet(Set<T> source) {
+		return FXCollections.observableSet(new HashSet<>(source));
+	}
+
 	public <E> SetProperty<E> field(SetGetter<M, E> getter, SetSetter<M, E> setter) {
 		return add(new BeanSetPropertyField<>(this::propertyWasChanged, getter,
-				(m, set) -> setter.accept(m, FXCollections.observableSet(set)), SimpleSetProperty::new));
+				(m, set) -> setter.accept(m, observableHashSet(set)), SimpleSetProperty::new));
 	}
 
 	public <E> SetProperty<E> immutableField(SetGetter<M, E> getter, SetImmutableSetter<M, E> immutableSetter) {
 		return addImmutable(new ImmutableSetPropertyField<>(
 				this::propertyWasChanged,
 				getter,
-				(m, set) -> immutableSetter.apply(m, FXCollections.observableSet(set)),
+				(m, set) -> immutableSetter.apply(m, observableHashSet(set)),
 				SimpleSetProperty::new
 		));
 	}
 
 	public <E> SetProperty<E> field(SetGetter<M, E> getter, SetSetter<M, E> setter, Set<E> defaultValue) {
 		return add(new BeanSetPropertyField<>(this::propertyWasChanged, getter,
-				(m, set) -> setter.accept(m, FXCollections.observableSet(set)), SimpleSetProperty::new,
+				(m, set) -> setter.accept(m, observableHashSet(set)), SimpleSetProperty::new,
 				defaultValue));
 	}
 
 	public <E> SetProperty<E> immutableField(SetGetter<M, E> getter, SetImmutableSetter<M, E> immutableSetter, Set<E> defaultValue) {
 		return addImmutable(new ImmutableSetPropertyField<>(
 				this::propertyWasChanged,
 				getter,
-				(m, set) -> immutableSetter.apply(m, FXCollections.observableSet(set)),
+				(m, set) -> immutableSetter.apply(m, observableHashSet(set)),
 				SimpleSetProperty::new,
 				defaultValue));
 	}
@@ -1502,22 +1518,22 @@ public <E> SetProperty<E> field(SetPropertyAccessor<M, E> accessor, Set<E> defau
 
 	public <E> SetProperty<E> field(String identifier, SetGetter<M, E> getter, SetSetter<M, E> setter) {
 		return addIdentified(identifier, new BeanSetPropertyField<>(this::propertyWasChanged, getter,
-				(m, set) -> setter.accept(m, FXCollections.observableSet(set)),
+				(m, set) -> setter.accept(m, observableHashSet(set)),
 				() -> new SimpleSetProperty<>(null, identifier)));
 	}
 
 	public <E> SetProperty<E> field(String identifier, SetGetter<M, E> getter, SetSetter<M, E> setter,
 			Set<E> defaultValue) {
 		return addIdentified(identifier, new BeanSetPropertyField<>(this::propertyWasChanged, getter,
-				(m, set) -> setter.accept(m, FXCollections.observableSet(set)),
+				(m, set) -> setter.accept(m, observableHashSet(set)),
 				() -> new SimpleSetProperty<>(null, identifier), defaultValue));
 	}
 
 	public <E> SetProperty<E> immutableField(String identifier, SetGetter<M, E> getter, SetImmutableSetter<M, E> immutableSetter) {
 		return addIdentifiedImmutable(identifier, new ImmutableSetPropertyField<>(
 				this::propertyWasChanged,
 				getter,
-				(m, set) -> immutableSetter.apply(m, FXCollections.observableSet(set)),
+				(m, set) -> immutableSetter.apply(m, observableHashSet(set)),
 				() -> new SimpleSetProperty<>(null, identifier)));
 	}
 
@@ -1526,7 +1542,7 @@ public <E> SetProperty<E> immutableField(String identifier, SetGetter<M, E> gett
 		return addIdentifiedImmutable(identifier, new ImmutableSetPropertyField<>(
 				this::propertyWasChanged,
 				getter,
-				(m, set) -> immutableSetter.apply(m, FXCollections.observableSet(set)),
+				(m, set) -> immutableSetter.apply(m, observableHashSet(set)),
 				() -> new SimpleSetProperty<>(null, identifier),
 				defaultValue));
 	}

diff --git a/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/mapping/ModelWrapperTest.java b/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/mapping/ModelWrapperTest.java
@@ -17,9 +17,15 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 
+import javafx.collections.ObservableList;
+import javafx.collections.ObservableSet;
 import org.junit.jupiter.api.Test;
 
 import javafx.beans.property.IntegerProperty;
@@ -618,6 +624,7 @@ public void testDifferentFlag() {
 		person.setName("horst");
 		person.setAge(32);
 		person.setNicknames(Arrays.asList("captain"));
+		person.setEmailAddresses(Collections.singleton("test@example.org"));
 
 		ModelWrapper<Person> personWrapper = new ModelWrapper<>(person);
 
@@ -626,6 +633,9 @@ public void testDifferentFlag() {
 		final StringProperty name = personWrapper.field(Person::getName, Person::setName);
 		final IntegerProperty age = personWrapper.field(Person::getAge, Person::setAge);
 		final ListProperty<String> nicknames = personWrapper.field(Person::getNicknames, Person::setNicknames);
+		final SetProperty<String> emailAddresses = personWrapper.field(Person::getEmailAddresses, Person::setEmailAddresses);
+
+		assertThat(personWrapper.isDifferent()).isFalse();
 
 		name.set("hugo");
 		assertThat(personWrapper.isDifferent()).isTrue();
@@ -679,25 +689,40 @@ public void testDifferentFlag() {
 		nicknames.setValue(FXCollections.observableArrayList("spectator"));
 		assertThat(personWrapper.isDifferent()).isTrue();
 
+		name.setValue("hans");
+		assertThat(personWrapper.isDifferent()).isTrue();
+
 		personWrapper.reload();
 		assertThat(personWrapper.isDifferent()).isFalse();
 
-		name.setValue("hans");
+		emailAddresses.remove("test@example.org");
+
 		assertThat(personWrapper.isDifferent()).isTrue();
 
-		personWrapper.reload();
+		emailAddresses.add("test@example.org");
 		assertThat(personWrapper.isDifferent()).isFalse();
 
+		emailAddresses.add("horst@example.org");
+		assertThat(personWrapper.isDifferent()).isTrue();
+
+		emailAddresses.remove("horst@example.org");
+		assertThat(personWrapper.isDifferent()).isFalse();
+
+		emailAddresses.setValue(FXCollections.observableSet("horst@example.org"));
+		assertThat(personWrapper.isDifferent()).isTrue();
+
 		personWrapper.reset();
 		assertThat(personWrapper.isDifferent()).isTrue();
 	}
 
+
 	@Test
 	public void testDifferentFlagWithFxProperties() {
 		PersonFX person = new PersonFX();
 		person.setName("horst");
 		person.setAge(32);
 		person.setNicknames(Arrays.asList("captain"));
+		person.setEmailAddresses(Collections.singleton("test@example.org"));
 
 		ModelWrapper<PersonFX> personWrapper = new ModelWrapper<>(person);
 
@@ -706,6 +731,7 @@ public void testDifferentFlagWithFxProperties() {
 		final StringProperty name = personWrapper.field(PersonFX::nameProperty);
 		final IntegerProperty age = personWrapper.field(PersonFX::ageProperty);
 		final ListProperty<String> nicknames = personWrapper.field(PersonFX::nicknamesProperty);
+		final SetProperty<String> emailAddresses = personWrapper.field(PersonFX::emailAddressesProperty);
 
 		name.set("hugo");
 		assertThat(personWrapper.isDifferent()).isTrue();
@@ -751,6 +777,22 @@ public void testDifferentFlagWithFxProperties() {
 		personWrapper.reload();
 		assertThat(personWrapper.isDifferent()).isFalse();
 
+		emailAddresses.remove("test@example.org");
+
+		assertThat(personWrapper.isDifferent()).isTrue();
+
+		emailAddresses.add("test@example.org");
+		assertThat(personWrapper.isDifferent()).isFalse();
+
+		emailAddresses.add("horst@example.org");
+		assertThat(personWrapper.isDifferent()).isTrue();
+
+		emailAddresses.remove("horst@example.org");
+		assertThat(personWrapper.isDifferent()).isFalse();
+
+		emailAddresses.setValue(FXCollections.observableSet("horst@example.org"));
+		assertThat(personWrapper.isDifferent()).isTrue();
+
 		personWrapper.reset();
 		assertThat(personWrapper.isDifferent()).isTrue();
 	}
@@ -1070,4 +1112,31 @@ public void testDefaultValues() {
 		assertThat(nameWithoutDefault.get()).isNull();
 		assertThat(ageWithoutDefault.get()).isEqualTo(0);
 	}
+
+
+	/**
+	 * This test case reproduces an error resulting from the internal usage of {@link FXCollections#observableSet(Set)}
+	 * that lead to the behavior that when a value was added to a mapped SetProperty then this value was initially also
+	 * added to the wrapped object.
+	 */
+	@Test
+	public void testSet() {
+		Person person = new Person();
+
+		person.setEmailAddresses(Collections.singleton("test@example.org"));
+
+		ModelWrapper<Person> personWrapper = new ModelWrapper<>(person);
+		SetProperty<String> emailAddresses = personWrapper.field(Person::getEmailAddresses, Person::setEmailAddresses);
+
+		personWrapper.commit();
+
+		// given
+		assertThat(person.getEmailAddresses()).containsOnly("test@example.org");
+
+		// when
+		emailAddresses.add("horst@example.org");
+
+		// then
+		assertThat(person.getEmailAddresses()).containsOnly("test@example.org");
+	}
 }