Lior Vernia has posted comments on this change. Change subject: webadmin: adding label tag to Host->Interface subtab ......................................................................
Patch Set 6: Code-Review+2 (3 comments) Please note the inline comments, including one in patchset 3. I don't see anything wrong with the code but there were some things I would do differently, but that's up to you. http://gerrit.ovirt.org/#/c/27898/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/InterfaceLabelWithToolTip.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/InterfaceLabelWithToolTip.java: Line 18: private final static ApplicationTemplates templates = GWT.create(ApplicationTemplates.class); Line 19: private final static SafeHtml labelImage = Line 20: SafeHtmlUtils.fromTrustedString(AbstractImagePrototype.create(resources.tagImage()).getHTML()); Line 21: Line 22: private final LabelWithToolTip label; I think inheritance is more correct here than composition, and would also allow you to drop the getLabel() method - I find it very strange to have to call getLabel() on a class that is in fact a label. Line 23: Line 24: public InterfaceLabelWithToolTip(VdsNetworkInterface iface) { Line 25: label = createInterfaceLabel(iface); Line 26: } http://gerrit.ovirt.org/#/c/27898/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/label/LabelWithToolTip.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/label/LabelWithToolTip.java: Line 21: this(text.asString()); Line 22: } Line 23: Line 24: public LabelWithToolTip(String text, int length) { Line 25: super(text); I think what you did originally was nicer, having this constructor called the one accepting Strings. It's kinda ugly to pass "-1" to signal not to pay attention to the length argument. So if you do decide to revert to your previous approach, the first condition below should be removed as well. Line 26: Line 27: if (length > -1 && text.length() > length) { Line 28: setText(text.substring(0, length - 3) + "..."); //$NON-NLS-1$ Line 29: } Line 35: tooltipPanel.setText(text); Line 36: } Line 37: Line 38: public void setTitle(SafeHtml text) { Line 39: tooltipPanel.setText(text); Consider calling setTitle(text.asString()) instead, it would guarantee consistent results in this class between the two overloads without relying on the consistency of the overloads in TooltipPanel. Line 40: } -- To view, visit http://gerrit.ovirt.org/27898 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I997d85d894d79f14402157fcc51a0cdbfe2ff056 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
