Vojtech Szocs has posted comments on this change. Change subject: webadmin: Implement sorting by simple status image ......................................................................
Patch Set 10: Code-Review+2 (3 comments) > Before giving +2 I would like other opinions on the matter. For simple cases like none/up/down status, it seems OK to me. For anything more complex (having more than two status images), I'd suggest to take enum-to-ImageResource info into account. For example, VmStatusColumn / VmStatusCell works with VMStatus enum to determine corresponding ImageResource. If we knew the enum, we could do following: * if image type enum doesn't implement Comparable<SpecificImageEnumType>, use ordinal() of that enum * if image type enum does implement Comparable<SpecificImageEnumType>, use Comparable.compareTo() Giving +2 - in this particular case, proposed (simple) solution looks OK to me. 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: private static final SimpleStatusImageComparator imageComparator = new SimpleStatusImageComparator(); in scope of NicActivateStatusColumn (outside this anonymous class declaration), comparators should be stateless anyway so we could just use a single instance. Same thing for existing TextColumnWithTooltip / LexoNumericComparator. What do you think? 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? 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. Looking at GWT-generated code for ClientBundle / ImageResource methods (like CommonApplicationResources) - there is no explicit equals() override for ImageResource implementations (ImageResourcePrototype), so equals() comparison falls back to comparing instances, but I assume you are aware of this. Typically, there should be a mapping of enum-to-ImageResource, if we had the enum info here, we could do a slightly better comparison. 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: 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
