Hello Lior Vernia,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/28392
to review the following change.
Change subject: webadmin,userportal: Fix client-side sort (TreeSet approach)
......................................................................
webadmin,userportal: Fix client-side sort (TreeSet approach)
Change-Id: I437e9dd7ff2afdb2d87fb0ba6c17dcb081cd965d
Signed-off-by: Vojtech Szocs <[email protected]>
Signed-off-by: Lior Vernia <[email protected]>
---
A
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedCollection.java
M
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java
A
frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java
M
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java
4 files changed, 361 insertions(+), 10 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/92/28392/1
diff --git
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedCollection.java
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedCollection.java
new file mode 100644
index 0000000..30f0cef
--- /dev/null
+++
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedCollection.java
@@ -0,0 +1,193 @@
+package org.ovirt.engine.ui.uicommonweb.models;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.SortedSet;
+import java.util.TreeSet;
+
+/**
+ * Collection implementation that uses {@link TreeSet} to maintain items
ordered by
+ * means of provided comparator with the ability to track logical position of
items.
+ *
+ * @param <E>
+ * The type of element maintained by this collection.
+ */
+public class SortedCollection<E> implements Collection<E>, Serializable {
+
+ public interface PositionCallback<T> {
+ int getPosition(T item);
+ }
+
+ public interface ComparatorWithPositionCallback<T> extends Comparator<T> {
+ void setPositionCallback(PositionCallback<T> positionCallback);
+ }
+
+ private static final long serialVersionUID = 6150830451408750485L;
+
+ private final SortedSet<E> items;
+ private final Map<E, Integer> positions = new HashMap<E, Integer>();
+
+ private int nextPos = 0;
+
+ SortedCollection(ComparatorWithPositionCallback<E> comparator,
+ Collection<? extends E> initialValues) {
+ this.items = new TreeSet<E>(comparator);
+
+ initComparator(comparator);
+ initPositions(initialValues);
+ addAll(initialValues);
+ }
+
+ SortedCollection(ComparatorWithPositionCallback<E> comparator) {
+ this(comparator, Collections.<E> emptyList());
+ }
+
+ void initComparator(ComparatorWithPositionCallback<E> comparator) {
+ comparator.setPositionCallback(new PositionCallback<E>() {
+ @Override
+ public int getPosition(E item) {
+ Integer pos = positions.get(item);
+ return (pos != null) ? pos.intValue() : Integer.MAX_VALUE;
+ }
+ });
+ }
+
+ void initPositions(Collection<? extends E> c) {
+ Iterator<? extends E> i = c.iterator();
+ while (i.hasNext()) {
+ positions.put(i.next(), nextPos++);
+ }
+ }
+
+ @Override
+ public int size() {
+ return items.size();
+ }
+
+ @Override
+ public boolean isEmpty() {
+ return items.isEmpty();
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return items.contains(o);
+ }
+
+ @Override
+ public Iterator<E> iterator() {
+ return new Iterator<E>() {
+
+ private final Iterator<? extends E> i = items.iterator();
+ private E lastRet;
+
+ @Override
+ public boolean hasNext() {
+ boolean hasNext = i.hasNext();
+ if (!hasNext) {
+ // Reference cleanup
+ lastRet = null;
+ }
+ return hasNext;
+ }
+
+ @Override
+ public E next() {
+ lastRet = i.next();
+ return lastRet;
+ }
+
+ @Override
+ public void remove() {
+ i.remove();
+ // In the edge case when user called next() and hasNext()
returned false,
+ // lastRet was cleaned up so we can't remove the corresponding
mapping from
+ // positions. This isn't very likely to happen, we prefer to
avoid keeping
+ // references to items within the iterator after hasNext()
returned false.
+ if (lastRet != null) {
+ SortedCollection.this.positions.remove(lastRet);
+ lastRet = null;
+ }
+ }
+
+ };
+ }
+
+ @Override
+ public Object[] toArray() {
+ return items.toArray();
+ }
+
+ @Override
+ public <T> T[] toArray(T[] a) {
+ return items.toArray(a);
+ }
+
+ @Override
+ public boolean add(E e) {
+ if (!positions.containsKey(e)) {
+ // Make new item's logical position higher than that of other items
+ positions.put(e, nextPos++);
+ }
+ return items.add(e);
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ positions.remove(o);
+ return items.remove(o);
+ }
+
+ @Override
+ public boolean containsAll(Collection<?> c) {
+ return items.containsAll(c);
+ }
+
+ /**
+ * Retain original AbstractCollection.addAll method
+ * implementation that adds elements one by one.
+ * <p>
+ * {@inheritDoc}
+ */
+ @Override
+ public boolean addAll(Collection<? extends E> c) {
+ boolean ret = false;
+ for (E e : c) {
+ if (add(e)) {
+ ret = true;
+ }
+ }
+ return ret;
+ }
+
+ @Override
+ public boolean removeAll(Collection<?> c) {
+ // TreeSet delegates to AbstractCollection.removeAll method
+ // which removes elements one by one via collection's iterator
+ return items.removeAll(c);
+ }
+
+ @Override
+ public boolean retainAll(Collection<?> c) {
+ // TreeSet delegates to AbstractCollection.retainAll method
+ // which removes elements one by one via collection's iterator
+ return items.retainAll(c);
+ }
+
+ @Override
+ public void clear() {
+ positions.clear();
+ items.clear();
+ }
+
+ @Override
+ public String toString() {
+ return items.toString();
+ }
+
+}
diff --git
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java
index 3244218..07a51f3 100644
---
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java
+++
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java
@@ -2,27 +2,45 @@
import java.util.Collection;
import java.util.Comparator;
-import java.util.SortedSet;
-import java.util.TreeSet;
import org.ovirt.engine.core.common.utils.ObjectUtils;
+import
org.ovirt.engine.ui.uicommonweb.models.SortedCollection.ComparatorWithPositionCallback;
+import
org.ovirt.engine.ui.uicommonweb.models.SortedCollection.PositionCallback;
public class SortedListModel<T> extends ListModel<T> {
- private static class SortSensitiveComparator<T> implements Comparator<T> {
+ public static class SortSensitiveComparator<T> implements
ComparatorWithPositionCallback<T> {
private final Comparator<? super T> comparator;
private final boolean sortAscending;
- public SortSensitiveComparator(Comparator<? super T> comparator,
boolean sortAscending) {
+ private PositionCallback<T> positionCallback;
+
+ SortSensitiveComparator(Comparator<? super T> comparator, boolean
sortAscending) {
assert comparator != null : "comparator cannot be null";
//$NON-NLS-1$
this.comparator = comparator;
this.sortAscending = sortAscending;
}
@Override
+ public void setPositionCallback(PositionCallback<T> positionCallback) {
+ this.positionCallback = positionCallback;
+ }
+
+ @Override
public int compare(T a, T b) {
- return sortAscending ? comparator.compare(a, b) :
comparator.compare(b, a);
+ int ret = comparator.compare(a, b);
+
+ if (ret == 0 && positionCallback != null) {
+ ret = positionCallback.getPosition(a) -
positionCallback.getPosition(b);
+ }
+
+ if (ret == 0) {
+ ret = a.hashCode() - b.hashCode();
+ }
+
+ ret = Integer.signum(ret);
+ return sortAscending ? ret : -ret;
}
@Override
@@ -54,7 +72,7 @@
}
- protected Comparator<? super T> comparator;
+ protected SortSensitiveComparator<T> comparator;
public SortedListModel(Comparator<? super T> comparator) {
super();
@@ -83,10 +101,7 @@
if (items == null || comparator == null) {
return items;
}
-
- SortedSet<T> sortedItems = new TreeSet<T>(comparator);
- sortedItems.addAll(items);
- return sortedItems;
+ return new SortedCollection<T>(comparator, items);
}
}
diff --git
a/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java
b/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java
new file mode 100644
index 0000000..4b6fc30
--- /dev/null
+++
b/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java
@@ -0,0 +1,136 @@
+package org.ovirt.engine.ui.uicommonweb.models;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.ovirt.engine.ui.uicommonweb.Configurator;
+import org.ovirt.engine.ui.uicommonweb.ILogger;
+import
org.ovirt.engine.ui.uicommonweb.models.SortedListModel.SortSensitiveComparator;
+
+public class SortedListModelTest {
+
+ static class TestItem {
+
+ private final int value;
+ private final int salt;
+
+ TestItem(int value, int salt) {
+ this.value = value;
+ this.salt = salt;
+ }
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + salt;
+ result = prime * result + value;
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null) {
+ return false;
+ }
+ if (getClass() != obj.getClass()) {
+ return false;
+ }
+ TestItem other = (TestItem) obj;
+ if (salt != other.salt) {
+ return false;
+ }
+ if (value != other.value) {
+ return false;
+ }
+ return true;
+ }
+
+ @Override
+ public String toString() {
+ return "Item " + value + "_" + salt;
+ }
+
+ }
+
+ private static final TestItem ITEM_1_1 = new TestItem(1, 1);
+ private static final TestItem ITEM_1_2 = new TestItem(1, 2);
+ private static final TestItem ITEM_2_1 = new TestItem(2, 1);
+ private static final TestItem ITEM_2_2 = new TestItem(2, 2);
+
+ private SortedListModel<TestItem> model;
+
+ SortSensitiveComparator<TestItem> makeValueAscComparator() {
+ return new SortSensitiveComparator<TestItem>(new
Comparator<TestItem>() {
+ @Override
+ public int compare(TestItem a, TestItem b) {
+ return a.value - b.value;
+ }
+ }, true);
+ }
+
+ @Before
+ public void setUp() {
+ this.model = new SortedListModel<TestItem>() {
+ @Override
+ protected Configurator lookupConfigurator() {
+ return null;
+ }
+
+ @Override
+ protected ILogger lookupLogger() {
+ return null;
+ }
+ };
+ model.comparator = makeValueAscComparator();
+ }
+
+ @Test
+ public void testSortItems_nullComparator() {
+ model.comparator = null;
+
+ List<TestItem> initial = Arrays.asList(ITEM_1_2, ITEM_1_1);
+ Collection<TestItem> sorted = model.sortItems(initial);
+
+ assertEquals(sorted, initial);
+ }
+
+ @Test
+ public void testSortItems_nullItems() {
+ Collection<TestItem> sorted = model.sortItems(null);
+
+ assertNull(sorted);
+ }
+
+ @Test
+ public void testSortItems_retainOrder() {
+ List<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2,
ITEM_1_1);
+ List<TestItem> expected = Arrays.asList(ITEM_1_2, ITEM_1_1, ITEM_2_1,
ITEM_2_2);
+ Collection<TestItem> sorted = model.sortItems(initial);
+
+ assertArrayEquals(sorted.toArray(), expected.toArray());
+
+ sorted.remove(ITEM_1_2);
+ sorted.add(ITEM_1_2);
+ expected = Arrays.asList(ITEM_1_1, ITEM_1_2, ITEM_2_1, ITEM_2_2);
+
+ assertArrayEquals(sorted.toArray(), expected.toArray());
+
+ sorted.removeAll(Arrays.asList(ITEM_1_2, ITEM_2_2));
+ sorted.addAll(Arrays.asList(ITEM_2_2, ITEM_1_2));
+
+ assertArrayEquals(sorted.toArray(), expected.toArray());
+ }
+
+}
diff --git
a/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java
b/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java
index 13bb2a3..fa91bce 100644
---
a/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java
+++
b/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java
@@ -1,5 +1,6 @@
package org.ovirt.engine.ui.webadmin.section.main.view.tab;
+import java.util.Comparator;
import java.util.List;
import org.ovirt.engine.core.common.businessentities.StoragePool;
@@ -54,6 +55,12 @@
return object.getName();
}
};
+ nameColumn.makeSortable(new Comparator<StoragePool>() {
+ @Override
+ public int compare(StoragePool o1, StoragePool o2) {
+ return 0; // TODO TEST
+ }
+ });
getTable().addColumn(nameColumn, constants.nameDc(), "150px");
//$NON-NLS-1$
CommentColumn<StoragePool> commentColumn = new
CommentColumn<StoragePool>();
--
To view, visit http://gerrit.ovirt.org/28392
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I437e9dd7ff2afdb2d87fb0ba6c17dcb081cd965d
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
