Alona Kaplan has posted comments on this change. Change subject: webadmin: adding label tag to Host->Interface subtab ......................................................................
Patch Set 3: (6 comments) http://gerrit.ovirt.org/#/c/27898/3/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/host/SubTabHostInterfaceView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/host/SubTabHostInterfaceView.java: Line 157: hostInterfaceForm = new HostInterfaceForm(getDetailModel()); Line 158: contentPanel.remove(contentPanel.getWidgetCount() - 1); Line 159: contentPanel.add(hostInterfaceForm); Line 160: Line 161: contentPanel.getParent().getElement().getStyle().setOverflowX(Overflow.AUTO); > This should work if you put it in the constructor after initWidget(), shoul It doesn't work. contentPanel.getParent() is null at this stage. Moved it to presenter's onReveal(). Line 162: } Line 163: Line 164: @Override Line 165: public void setRefreshButtonVisibility(boolean visible) { http://gerrit.ovirt.org/#/c/27898/3/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/HostInterfaceForm.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/HostInterfaceForm.java: Line 123: grid.setWidget(row, col, widget); Line 124: grid.getCellFormatter().setHeight(row, col, "100%"); //$NON-NLS-1$ Line 125: } Line 126: Line 127: static LabelWithToolTip createInterfaceLabel(VdsNetworkInterface iface) { > It might be nicer to create a class extending LabelWithTooltip and put this Done Line 128: boolean hasLabels = iface.getLabels() != null Line 129: && !iface.getLabels().isEmpty(); Line 130: LabelWithToolTip interfaceNameWithLabel = Line 131: new LabelWithToolTip(hasLabels ? templates.textImageLabels(iface.getName(), labelImage) Line 137: } Line 138: Line 139: private static SafeHtml createLabelToolTip(Set<String> labels) { Line 140: SafeHtmlBuilder tooltip = new SafeHtmlBuilder(); //$NON-NLS-1$ Line 141: boolean isFirst = true; > Please consider removing the boolean variable, always add the first label a I like it as is:) Line 142: Line 143: if (labels == null) { Line 144: return null; Line 145: } http://gerrit.ovirt.org/#/c/27898/3/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/InterfacePanel.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/InterfacePanel.java: Line 51: row.getColumnFormatter().setWidth(0, "30px"); //$NON-NLS-1$ Line 52: row.getColumnFormatter().setWidth(1, "210px"); //$NON-NLS-1$ Line 53: Line 54: // Check box and interface status icon Line 55: row.setWidget(0, 0, new HorizontalPanel() { > Why was this necessary? The only thing that changed is the addition of the With FlowPanel the status image is under the checkbox (which makes sense cause this is how flow panel works). I"m not sure how it worked before but after all the changes in this patch it is the result... Line 56: { Line 57: if (isSelectionAvailable) { Line 58: add(getCheckBox()); Line 59: } http://gerrit.ovirt.org/#/c/27898/3/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 23: Line 24: public LabelWithToolTip(String text, int length) { Line 25: this(text); Line 26: Line 27: if (length > -1 && text.length() > length) { > I think you can get rid of the first condition now, due to the change above Change the code according to your second comment. now this condition is relevant again. Line 28: setText(text.substring(0, length - 3) + "..."); //$NON-NLS-1$ Line 29: } Line 30: } Line 31: Line 32: public LabelWithToolTip(String text) { Line 33: this(SafeHtmlUtils.fromTrustedString(text)); Line 34: } Line 35: Line 36: public LabelWithToolTip(SafeHtml text) { > I think a lot of the changes in this file can be avoided. Why not keep the Done. Still overloading setTitle(SafeHtml title), IMO it is better api for this class. Line 37: super(text); Line 38: Line 39: tooltipPanel.setWidget(tooltip); Line 40: tooltipPanel.getElement().getStyle().setZIndex(1); -- 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: 3 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
