Vojtech Szocs has posted comments on this change. Change subject: webadmin: Support column sorting ......................................................................
Patch Set 1: (5 comments) Thanks guys for your reviews. I'll submit one more patchset once everything is ready for final review. http://gerrit.ovirt.org/#/c/25910/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/AbstractActionTable.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/AbstractActionTable.java: Line 394: sortBy = sortableColumn.getSortBy(); Line 395: } Line 396: Line 397: // Apply sort options to model Line 398: searchableModel.setSortOptions(sortBy, event.isSortAscending()); > See my comment on SortableColumn - if you decide to follow it, the sorting I agree with your comment, I'll introduce the change you mentioned in upcoming patchset. Line 399: Line 400: // Synchronize column sort info Line 401: ColumnSortInfo columnSortInfo = event.getColumnSortList().get(0); Line 402: tableHeader.getColumnSortList().push(columnSortInfo); http://gerrit.ovirt.org/#/c/25910/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SortableColumn.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SortableColumn.java: Line 20: Line 21: /** Line 22: * Makes this column sortable, passing the name of the field to sort by. Line 23: */ Line 24: public void makeSortable(String sortBy) { > Consider adding another override, that accepts a Comparator<T> as argument. Good idea. I'll fix this in upcoming patchset. I think that Comparator<? super T> is better here (and also in SearchableListModel's setComparator method) because of generic type signature consistency with Java collection classes working with Comparator, for example: java.util.TreeSet(Comparator<? super T> comparator) Comparator type's contravariance will ensure that one comparator could be used for multiple types, avoiding the need to implement one Comparator per each type. Line 25: setSortable(true); Line 26: this.sortBy = sortBy; Line 27: } Line 28: http://gerrit.ovirt.org/#/c/25910/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 730: /** Line 731: * Updates current sort options, performing {@link #refresh} if necessary. Line 732: */ Line 733: public void setSortOptions(String sortBy, boolean sortAscending) { Line 734: boolean shouldRefresh = (this.sortBy == null && sortBy != null) > You could use !Objects.equals() instead of these three lines. AFAIK, Objects.equals() is Java 7 and our current GWT version (2.5.1) doesn't support Java 7 yet. Note that GWT 2.6 adds initial support for Java 7, but we're not there yet. As a temporary solution, I'll use our shared helper class instead: org.ovirt.engine.core.common.utils.ObjectUtils.objectsEqual Line 735: || (this.sortBy != null && sortBy == null) Line 736: || (!this.sortBy.equals(sortBy)); Line 737: shouldRefresh = shouldRefresh || this.sortAscending != sortAscending; Line 738: Line 733: public void setSortOptions(String sortBy, boolean sortAscending) { Line 734: boolean shouldRefresh = (this.sortBy == null && sortBy != null) Line 735: || (this.sortBy != null && sortBy == null) Line 736: || (!this.sortBy.equals(sortBy)); Line 737: shouldRefresh = shouldRefresh || this.sortAscending != sortAscending; > And then this could be part of the same line, because it would be much shor Indeed. Line 738: Line 739: this.sortBy = sortBy; Line 740: this.sortAscending = sortAscending; Line 741: http://gerrit.ovirt.org/#/c/25910/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDataCenterView.java: Line 53: public String getValue(StoragePool object) { Line 54: return object.getName(); Line 55: } Line 56: }; Line 57: nameColumn.makeSortable("name"); //$NON-NLS-1$ > Exactly, similar to what is done in the searchbackend project (take a look Greg is right, magic strings are not good, I'll fix this in upcoming patchset. The "name" string gets passed into search query, for example: ... SORTBY name ASC Engine backend maps the query into SQL command, in this case "name" field will be mapped to "name" DB column. Line 58: getTable().addColumn(nameColumn, constants.nameDc(), "150px"); //$NON-NLS-1$ Line 59: Line 60: CommentColumn<StoragePool> commentColumn = new CommentColumn<StoragePool>(); Line 61: getTable().addColumnWithHtmlHeader(commentColumn, commentColumn.getHeaderHtml(), "30px"); //$NON-NLS-1$ -- To view, visit http://gerrit.ovirt.org/25910 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I141ea068fe90409852d34bea6fedb45d0d8a07ae Gerrit-PatchSet: 1 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: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
