Lior Vernia has posted comments on this change. Change subject: webadmin,userportal: Fix client-side sort (ArrayList approach) ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/28392/2/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 84: if (items == null || comparator == null) { Line 85: return items; Line 86: } Line 87: Line 88: List<T> sortedList = new ArrayList<T>() { > > The contract of List isn't compatible with what SortedListModel is suppos Since we decided to go with the simplest implementation possible, pleas remove these overrides. They're not required if we assume that developers don't "misbehave". Line 89: Line 90: private static final long serialVersionUID = 499355412389856325L; Line 91: Line 92: @Override http://gerrit.ovirt.org/#/c/28392/2/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 83: } Line 84: Line 85: @Before Line 86: public void setUp() { Line 87: this.model = new SortedListModel<TestItem>() { If we keep this test class, consider using Mockito.spy() and mocking these methods instead of extending SortedListModel. It's less invasive with respect to the class you're testing - instead of modifying its behavior, you use a proxy to intercept some method invocations. Line 88: @Override Line 89: protected Configurator lookupConfigurator() { Line 90: return null; Line 91: } Line 115: assertNull(sorted); Line 116: } Line 117: Line 118: @Test Line 119: public void testSortItems_retainOrder() { This test is not required anymore, and I'm not sure there's much else to test. Maybe add a test for switching comparators which makes sure that the order changes accordingly. Line 120: List<TestItem> initial = Arrays.asList(ITEM_2_1, ITEM_2_2, ITEM_1_2, ITEM_1_1); Line 121: List<TestItem> expected = Arrays.asList(ITEM_1_2, ITEM_1_1, ITEM_2_1, ITEM_2_2); Line 122: Collection<TestItem> sorted = model.sortItems(initial); Line 123: -- To view, visit http://gerrit.ovirt.org/28392 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437e9dd7ff2afdb2d87fb0ba6c17dcb081cd965d Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
