Lior Vernia has posted comments on this change. Change subject: webadmin: Implement sorting by simple status image ......................................................................
Patch Set 10: (3 comments) Response to your general comment is also included inline in SimpleStatusImageComparator. Will wait on merging this until you voice your opinion concerning the static nature of comparators in NicActivateStatusColumn. http://gerrit.ovirt.org/#/c/28753/10/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/NicActivateStatusColumn.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/NicActivateStatusColumn.java: Line 37: Line 38: public void makeSortable() { Line 39: makeSortable(new Comparator<T>() { Line 40: Line 41: SimpleStatusImageComparator imageComparator = new SimpleStatusImageComparator(); > Small thing, this could be: Functionally it would be the same so I don't see the harm, but from a design point of view it made sense to me that each column instance would have its own comparator - after all, comparators don't generally have to be stateless so this is just an implementation detail that could theoretically change (e.g. have a caching comparator if the comparison is costly). It should definitely be private though, and I don't mind changing it to static as well :) Whatever makes more sense to you. Line 42: Line 43: @Override Line 44: public int compare(T o1, T o2) { Line 45: return imageComparator.compare(getImage(o1), getImage(o2)); http://gerrit.ovirt.org/#/c/28753/10/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SimpleStatusColumnComparator.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SimpleStatusColumnComparator.java: Line 11: private final SimpleStatusImageComparator imageComparator; Line 12: Line 13: public SimpleStatusColumnComparator(ImageResourceColumn<T> renderingColumn) { Line 14: this.renderingColumn = renderingColumn; Line 15: imageComparator = new SimpleStatusImageComparator(); > The "imageComparator" could be a singleton instance, what do you think? Same argument as in other patch. Line 16: } Line 17: Line 18: @Override Line 19: public int compare(T o1, T o2) { http://gerrit.ovirt.org/#/c/28753/10/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SimpleStatusImageComparator.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SimpleStatusImageComparator.java: Line 23: return (o1 == null) ? -1 : 1; Line 24: } else if (o1.equals(o2)) { Line 25: return 0; Line 26: } else { Line 27: return downImage.equals(o1) ? -1 : 1; > I see that this is useful for simple none/up/down status comparison. Please take a look at http://gerrit.ovirt.org/#/c/28755/. For status columns that had their own "specialized" status enums I indeed suggest counting on their underlying numerical values. But there are several status columns that are represented by different enums, which essentially mean the same thing (none/up/down) and should probably be treated in a consistent manner in the same sorting order. That's why I think for these columns, comparing by image is better than comparing by their specific enums. Also, good comment concerning comparison - I think it's still better to use equals() here, in case implementation ever changes. Line 28: } Line 29: } Line 30: -- To view, visit http://gerrit.ovirt.org/28753 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ac043ef1d522f67678b5c70b461833a82d3d9f0 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@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
