Lior Vernia has posted comments on this change. Change subject: webadmin,userportal: Consolidate SortedListModel vs. SearchableListModel ......................................................................
Patch Set 1: (13 comments) http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java: Line 837: lastSelectedItems.add(item); Line 838: } Line 839: } Line 840: Line 841: if (useComparator(value)) { This means: "perform sorting if there's something to sort by". The previous condition meant: "perform sorting if there's something to sort by AND if the collection isn't already sorted by that ordering". I think the previous condition more accurately represents what should happen here. Line 842: Collection<T> sortedItems = sortItems(value); Line 843: itemsChanging(sortedItems, items); Line 844: items = sortedItems; Line 845: } else { http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java: Line 82: // Sort items after updating comparator Line 83: Collection<T> items = getItems(); Line 84: if (items != null) { Line 85: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : new ArrayList<T>(items); Line 86: super.setItems(maybeSortedItems); > Instead of the two lines above: I agree. Plus if this is a SearchableListModel, you'll want the overridden method to be triggered rather than that of ListModel. Line 87: } Line 88: } Line 89: Line 90: @Override Line 88: } Line 89: Line 90: @Override Line 91: public void setItems(Collection<T> items) { Line 92: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; I think this should be called without the useComparator() condition. Maybe just check for null instead. Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<T> items) { Line 92: Collection<T> maybeSortedItems = useComparator(items) ? sortItems(items) : items; Line 93: super.setItems(maybeSortedItems); Line 94: } Line 95: Line 96: protected boolean useComparator(Collection<T> items) { Considering comments here and in SearchableListModel, I think this method isn't needed. Line 97: return items != null && comparator != null; Line 98: } Line 99: Line 100: protected Collection<T> sortItems(Collection<T> items) { Line 97: return items != null && comparator != null; Line 98: } Line 99: Line 100: protected Collection<T> sortItems(Collection<T> items) { Line 101: if (comparator == null) { I don't think this is an erroneous case - if no comparator is supplied, usually the semantics are to use the objects' "natural" ordering. Line 102: throw new IllegalStateException("comparator cannot be null"); //$NON-NLS-1$ Line 103: } Line 104: Line 105: List<T> sortedList = new ArrayList<T>(items); http://gerrit.ovirt.org/#/c/31101/1/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModelTest.java: Line 93: }; Line 94: } Line 95: Line 96: @Test Line 97: public void testUseComparator_nullItems() { I don't really care about conventions, but this underscore thing won't be liked by true Java people. Line 98: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 99: assertThat(testedModel.useComparator(null), is(false)); Line 100: } Line 101: Line 117: testedModel.sortItems(null); Line 118: } Line 119: Line 120: @Test(expected = IllegalStateException.class) Line 121: public void testSortItems_nullComparator() { As per my comment in SortedListModel, I think this is a valid case that shouldn't result in exception. Line 122: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); Line 123: testedModel.sortItems(SOME_ITEMS); Line 124: } Line 125: Line 123: testedModel.sortItems(SOME_ITEMS); Line 124: } Line 125: Line 126: @Test Line 127: public void testSortItems_noItems() { Common functionality between sortItems() test methods (the assertions, basically) could be extracted to a method that receives only initial items, comparator and expected ordering. Line 128: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 129: Collection<TestItem> initial = Collections.emptyList(); Line 130: Collection<TestItem> sorted = testedModel.sortItems(initial); Line 131: assertNotSame(sorted, initial); Line 156: assertArrayEquals(sorted.toArray(), expected.toArray()); Line 157: } Line 158: Line 159: @Test Line 160: public void testSetItems_nullItems() { Same comment as with the sortItems() test methods. Line 161: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 162: testedModel.setItems(null); Line 163: assertNull(testedModel.getItems()); Line 164: } Line 168: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); Line 169: Collection<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 170: testedModel.setItems(initial); Line 171: Collection<TestItem> sorted = testedModel.getItems(); Line 172: assertSame(sorted, initial); Not sure this is what I would expect. My expectation would depend on whether the class implements Comparable or not. If it does, I would expect it to be sorted by that. Otherwise, I would expect it to either keep the same ordering or be sorted by hashCode() or whatever Java does by default, either would be fine. But I don't think either of these will currently happen - won't you throw an exception for the null comparator? Line 173: } Line 174: Line 175: @Test Line 176: public void testSetItems_noItems() { Line 207: public void testSetComparator_nullToValueComparator() { Line 208: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(); Line 209: Collection<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 210: Collection<TestItem> expected = Arrays.asList(ITEM_1_2, ITEM_1_1, ITEM_2_1, ITEM_2_2); Line 211: testedModel.setItems(initial); Here I would plug another line to test the ordering, and then again after the comparator is set. But again, this should probably happen in the extracted method. Line 212: testedModel.setComparator(makeValueComparator()); Line 213: Collection<TestItem> sorted = testedModel.getItems(); Line 214: assertNotSame(sorted, initial); Line 215: assertThat(sorted.isEmpty(), is(false)); Line 221: public void testSetComparator_valueToNullComparator() { Line 222: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 223: Collection<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 224: Collection<TestItem> expected = Arrays.asList(ITEM_1_2, ITEM_1_1, ITEM_2_1, ITEM_2_2); Line 225: testedModel.setItems(initial); Same comment, according to what we define to be the behavior with a null comparator. Line 226: testedModel.setComparator(null); Line 227: Collection<TestItem> sorted = testedModel.getItems(); Line 228: assertNotSame(sorted, initial); Line 229: assertThat(sorted.isEmpty(), is(false)); Line 235: public void testSetComparator_valueToSaltComparator() { Line 236: SortedListModel<TestItem> testedModel = new SortedListModel<TestItem>(makeValueComparator()); Line 237: Collection<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 238: Collection<TestItem> expected = Arrays.asList(ITEM_1_1, ITEM_2_1, ITEM_1_2, ITEM_2_2); Line 239: testedModel.setItems(initial); Same comment, make one check here and one after the comparator is set. Line 240: testedModel.setComparator(makeSaltComparator()); Line 241: Collection<TestItem> sorted = testedModel.getItems(); Line 242: assertNotSame(sorted, initial); Line 243: assertThat(sorted.isEmpty(), is(false)); -- To view, visit http://gerrit.ovirt.org/31101 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id10b84c2fb476ed2240c33d72f6432335650df33 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
