Reviewers: rdayal,
Description:
Issue 7038: CompositeEditor and ListEditor optimizations
Initializer and Refresher visitors cause CompositeEditors' sub-editors
to be visited twice: once by the EditorChain#attach() they should be
calling in their setValue, and then once again by the visitor stepping
into sub-editors.
When a CompositeEditor has a ListEditor as a sub-editor, that ListEditor
then would create a ListEditorWrapper twice in a row, leading in
creating
sub-editors to dispose them right away, which is obviously bad for
performance.
Having the Initializer and Refresher avoid visiting CompositeEditors'
sub-editors (because setValue would have already done so) fixes the
issue.
Additionally, ListEditor unconditionally recreate its ListEditorWrapper
in setValue even when setting the same value as the one being edited.
This
leads to recreating all sub-editors instead of reusing them (even though
the EditorSource should take care of this already for best performance
in
all cases).
Please review this at https://gwt-code-reviews.appspot.com/1664803/
Affected files:
M user/src/com/google/gwt/editor/client/adapters/ListEditor.java
M user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java
M user/src/com/google/gwt/editor/client/impl/Refresher.java
M user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java
Index: user/src/com/google/gwt/editor/client/adapters/ListEditor.java
diff --git a/user/src/com/google/gwt/editor/client/adapters/ListEditor.java
b/user/src/com/google/gwt/editor/client/adapters/ListEditor.java
index
592a188cde90a0c996e3c382345e1689ba4cb9af..40f46cb26e32fde25bb3eeb19d84a2a8c862a772
100644
--- a/user/src/com/google/gwt/editor/client/adapters/ListEditor.java
+++ b/user/src/com/google/gwt/editor/client/adapters/ListEditor.java
@@ -18,6 +18,7 @@ package com.google.gwt.editor.client.adapters;
import com.google.gwt.editor.client.CompositeEditor;
import com.google.gwt.editor.client.Editor;
import com.google.gwt.editor.client.EditorDelegate;
+import com.google.gwt.editor.client.impl.Refresher;
import java.util.Collections;
import java.util.List;
@@ -124,6 +125,11 @@ public class ListEditor<T, E extends Editor<T>>
implements
* @param value a List of data objects of type T
*/
public void setValue(List<T> value) {
+ if ((list == null && value == null) || (list != null &&
list.isSameValue(value))) {
+ // setting the same value as the one being edited
+ list.refresh();
+ return;
+ }
if (list != null) {
// Having entire value reset, so dump the wrapper gracefully
list.detach();
Index: user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java
diff --git
a/user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java
b/user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java
index
b028a7983bba67432a0e8198baf31ce1e6b2a72b..968de45ded2b7b7ee21be85ef2bf28f250cdaa38
100644
--- a/user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java
+++ b/user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java
@@ -122,4 +122,41 @@ class ListEditorWrapper<T, E extends Editor<T>>
extends AbstractList<T> {
List<? extends E> getEditors() {
return editors;
}
+
+ /**
+ * Checks whether that ListEditorWrapper can be reused for the passed
list.
+ * <p>
+ * The ListEditorWrapper can be reused if and only if the backing list
is the
+ * same instance as the passed list.
+ */
+ boolean isSameValue(List<T> value) {
+ // identity check intentional
+ return (backing == value);
+ }
+
+ /**
+ * Refresh the editors in case the backing list has been modified from
+ * outside the ListEditorWrapper list.
+ * <p>
+ * This is basically the opposite from {@link #flush()}. It's used to
+ * reuse sub-editors instead of recreating a ListEditorWrapper from
+ * scratch.
+ */
+ void refresh() {
+ int i = 0;
+ for (T item : backing) {
+ if (i < size()) {
+ this.set(i, item);
+ } else {
+ assert i == size();
+ this.add(i, item);
+ }
+ i++;
+ }
+ while (backing.size() < size()) {
+ remove(size() - 1);
+ }
+ assert backing.size() == size();
+ assert backing.equals(workingCopy);
+ }
}
\ No newline at end of file
Index: user/src/com/google/gwt/editor/client/impl/Refresher.java
diff --git a/user/src/com/google/gwt/editor/client/impl/Refresher.java
b/user/src/com/google/gwt/editor/client/impl/Refresher.java
index
dee6f1877cba4f658fd5c18b9adb6dcbb15d5ac2..6043899d8ed75faa718abcce30e86abcf9c90dce
100644
--- a/user/src/com/google/gwt/editor/client/impl/Refresher.java
+++ b/user/src/com/google/gwt/editor/client/impl/Refresher.java
@@ -45,6 +45,9 @@ public class Refresher extends EditorVisitor {
asLeaf.setValue(toSet);
}
}
- return true;
+ // CompositeEditor's setValue should create sub-editors and attach
them to
+ // the EditorChain, which will traverse them. Returning true here for a
+ // CompositeEditor would then traverse it twice. See issue 7038.
+ return ctx.asCompositeEditor() == null;
}
}
\ No newline at end of file
Index: user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java
diff --git
a/user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java
b/user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java
index
b6cd3cb9f7e4575e6e78d1d57bdbd9fe9192ec37..08ad8a758c4ef10737f0a38a88b9bcd39155d446
100644
--- a/user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java
+++ b/user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java
@@ -210,11 +210,11 @@ public class SimpleBeanEditorTest extends GWTTestCase
{
class PersonWithListEditor implements Editor<PersonWithList> {
SimpleEditor<String> nameEditor = SimpleEditor.of(UNINITIALIZED);
- ListEditor<Address, AddressEditor> addressesEditor = ListEditor
- .of(new EditorSource<AddressEditor>() {
+ ListEditor<Address, ValueAwareAddressEditor> addressesEditor =
ListEditor
+ .of(new EditorSource<ValueAwareAddressEditor>() {
@Override
- public AddressEditor create(int index) {
- return new AddressEditor();
+ public ValueAwareAddressEditor create(int index) {
+ return new ValueAwareAddressEditor();
}
});
}
@@ -526,6 +526,30 @@ public class SimpleBeanEditorTest extends GWTTestCase {
assertEquals(3, editors.size());
assertEquals("quux", editors.get(2).getValue());
assertEquals(editors, new
ArrayList<SimpleEditor<String>>(positionMap.values()));
+
+ // Change list outside editor: shouldn't impact editors
+ rawData.clear();
+ rawData.addAll(Arrays.asList("able", "baker"));
+ List<String> expectedList = Arrays.asList("Hello", "World", "quux");
+ assertEquals(expectedList, editor.getList());
+ assertEquals(expectedList.size(), editors.size());
+ assertEquals(expectedList, Arrays.asList(editors.get(0).getValue(),
editors.get(1).getValue(),
+ editors.get(2).getValue()));
+ assertEquals(editors, new
ArrayList<SimpleEditor<String>>(positionMap.values()));
+
+ // Edit again: should reuse sub-editors and dispose unneeded ones
+ disposed[0] = null;
+ expectedDisposed = editors.get(2);
+ @SuppressWarnings("unchecked")
+ List<SimpleEditor<String>> expectedEditors =
Arrays.asList(editors.get(0), editors.get(1));
+ driver.edit(rawData);
+ assertEquals(expectedEditors, editors);
+ assertEquals(expectedEditors, editor.getEditors());
+ assertEquals(rawData, editor.getList());
+ assertEquals(rawData.size(), editors.size());
+ assertEquals(rawData, Arrays.asList(editors.get(0).getValue(),
editors.get(1).getValue()));
+ assertEquals(editors, new
ArrayList<SimpleEditor<String>>(positionMap.values()));
+ assertEquals(expectedDisposed, disposed[0]);
}
/**
@@ -556,13 +580,23 @@ public class SimpleBeanEditorTest extends GWTTestCase
{
// Edit
driver.edit(person);
- AddressEditor addressEditor =
personEditor.addressesEditor.getEditors().get(1);
+ ValueAwareAddressEditor addressEditor =
personEditor.addressesEditor.getEditors().get(1);
+ // Check that setValue is only called once on sub-editors (issue 7038)
+ assertEquals(1, addressEditor.setValueCalled);
+
assertEquals("a2City", addressEditor.city.getValue());
addressEditor.city.setValue("edited");
// Flush
driver.flush();
assertEquals("edited", person.addresses.get(1).getCity());
+
+ // Verify that setting the same list reuses sub-editors
+ addressEditor.setValueCalled = 0;
+ driver.edit(person);
+ assertSame(addressEditor,
personEditor.addressesEditor.getEditors().get(1));
+ // Check that setValue has correctly been called on the sub-editor
anyway
+ assertEquals(1, addressEditor.setValueCalled);
}
public void testMultipleBinding() {
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
