Vojtech Szocs has posted comments on this change. Change subject: userportal, webadmin: Tooltip infrastructure ......................................................................
Patch Set 4: (16 comments) Nice patch, some comments inline. http://gerrit.ovirt.org/#/c/27325/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/TooltipPanel.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/TooltipPanel.java: Line 15: import com.google.gwt.user.client.ui.HTML; Line 16: import com.google.gwt.user.client.ui.Widget; Line 17: Line 18: /** Line 19: * @param <W> Widget that controls whether to show/hide the tool-tip panel. Is the <W> type parameter still relevant here? Line 20: */ Line 21: public class TooltipPanel extends ElementAwareDecoratedPopupPanel { Line 22: Line 23: private HandlerRegistration tooltipNativePreviewHandlerRegistration; Line 52: * Set the tool-tip text, this will override any existing widget on the panel. Don't use if you provide your Line 53: * own widget Line 54: * @param text The tool-tip text. Line 55: */ Line 56: public void setText(String text) { There is also another way, up to you to consider: SafeHtml safeText = SafeHtmlUtils.fromSafeConstant(text); setText(safeText); Both setText methods are practically the same, the only difference is how they instantiate HTML widget, above code should reduce the complexity. Line 57: if (text != null && !"".equals(text)) { Line 58: setWidget(new HTML(text)); Line 59: } else { Line 60: setWidget(null); Line 65: * Set the tool-tip text, if the owner hasn't provided its own widget. If you pass null the text will be a Line 66: * blank string Line 67: * @param text The tool-tip text. Line 68: */ Line 69: public void setText(SafeHtml text) { Minor thing, it would be nice to unify both setText Javadocs, the only practical difference is first one accepting text as HTML string, second one accepting text as SafeHtml. Line 70: if(!isTextEmpty(text)) { Line 71: setWidget(new HTML(text)); Line 72: } else { Line 73: setWidget(null); Line 66: * blank string Line 67: * @param text The tool-tip text. Line 68: */ Line 69: public void setText(SafeHtml text) { Line 70: if(!isTextEmpty(text)) { Minor thing, please put space between "if" and "(" to make it not appear as method invocation. Line 71: setWidget(new HTML(text)); Line 72: } else { Line 73: setWidget(null); Line 74: } Line 90: public void applyTo(final Widget tooltipSource) { Line 91: tooltipSource.addAttachHandler(new AttachEvent.Handler() { Line 92: Line 93: @Override Line 94: public void onAttachOrDetach(AttachEvent event) { +1 on dealing with source widget's DOM attach/detach. Line 95: //Only attach the preview handlers if the widget is attached to the DOM. Line 96: if (event.isAttached()) { Line 97: registerPreviewHandler(tooltipSource); Line 98: } else { Line 114: } Line 115: tooltipNativePreviewHandlerRegistration = Event.addNativePreviewHandler(new NativePreviewHandler() { Line 116: @Override Line 117: public void onPreviewNativeEvent(NativePreviewEvent event) { Line 118: if(tooltipSource.isVisible()) { Minor thing, please put space between "if" and "(" to make it not appear as method invocation. Line 119: Element tooltipSourceElement = tooltipSource.getElement(); Line 120: handlePreviewEvent(event, tooltipSourceElement); Line 121: } Line 122: } Line 128: * {@code Widget} to pass into the tool-tip. Line 129: * @param tooltipSource The {@code Element} that the panel should be applied to. Line 130: */ Line 131: public void applyTo(final Element tooltipSource) { Line 132: if (tooltipNativePreviewHandlerRegistration != null) { For this variant of applyTo method, once it's called for given Element, it can't be called again because tooltipNativePreviewHandlerRegistration will be always != null from that point on, is that correct? If so, once that Element gets detached (as part of detach of its owner Widget), we can't call applyTo again. Do you think this would be problematic? Line 133: return; Line 134: } Line 135: tooltipNativePreviewHandlerRegistration = Event.addNativePreviewHandler(new NativePreviewHandler() { Line 136: @Override Line 158: } Line 159: Line 160: /** Line 161: * Use handleNativeBrowserEvent, if you don't even have an {@code Element} to use as the source Line 162: * of the tool-tip. For instance in GWT Grid cell. I think you mean CellTable, not the Grid widget. Grid works by manipulating DOM elements, like other "classic" GWT widgets. CellTable works by constructing HTML string and then setting it into <table> element as innerHTML, which causes re-creation of child DOM elements (such as cells) and therefore implies that such DOM elements are "non-persistent" across widget redraws. Line 163: * @param tooltipSource The element that is the source of the event. Line 164: * @param event The native event. Line 165: */ Line 166: public void handleNativeBrowserEvent(final Element tooltipSource, NativeEvent event) { Line 167: if (BrowserEvents.MOUSEOUT.equals(event.getType())) { Line 168: onTooltipSourceMouseOut(); Line 169: TooltipPanel.this.hide(true); Line 170: } else if (BrowserEvents.MOUSEOVER.equals(event.getType())) { Line 171: displayTooltipPanel(tooltipSource); Would it make sense to have onTooltipSourceMouseOver called here, in consistency with onTooltipSourceMouseOut called above? In other words, does the fact that we have / don't have source Element affect if onTooltipSourceMouseOver should / should not be called? Line 172: } Line 173: } Line 174: Line 175: /** Line 206: NodeList<Node> children = element.getChildNodes(); Line 207: for (int i = 0; i < children.getLength(); i++) { Line 208: Node child = children.getItem(i); Line 209: if (child instanceof Element) { Line 210: if(child.equals(tooltipSourceElement)) { Minor thing, please put space between "if" and "(" to make it not appear as method invocation. Line 211: displayTooltipPanel(tooltipSourceElement); Line 212: return; Line 213: } Line 214: } Line 212: return; Line 213: } Line 214: } Line 215: } Line 216: if(element.equals(tooltipSourceElement)) { Minor thing, please put space between "if" and "(" to make it not appear as method invocation. Line 217: displayTooltipPanel(tooltipSourceElement); Line 218: } Line 219: } Line 220: } else { Line 218: } Line 219: } Line 220: } else { Line 221: onTooltipSourceMouseOut(); Line 222: TooltipPanel.this.hide(true); The same two lines also exist for "mouseout" handling in handleNativeBrowserEvent method. Can we extract these two lines into some private method to reduce code duplication? Line 223: } Line 224: break; Line 225: } Line 226: } http://gerrit.ovirt.org/#/c/27325/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/generic/EntityModelTextBox.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/generic/EntityModelTextBox.java: Line 12: public class EntityModelTextBox<T> extends ValueBox<T> implements EditorWidget<T, ValueBoxEditor<T>> { Line 13: Line 14: private ObservableValueBoxEditor<T> editor; Line 15: Line 16: private final TooltipPanel tooltipPanel = new TooltipPanel(true, this); Do we really need to provide tooltipSource ("this") here? In setTitle method below, we replace tooltip content widget anyway via tooltipPanel.setText method. In other words, it could simply be like this: private final TooltipPanel tooltipPanel = new TooltipPanel(true); Line 17: Line 18: public EntityModelTextBox(Renderer<T> renderer, Parser<T> parser) { Line 19: super(Document.get().createTextInputElement(), renderer, parser); Line 20: } http://gerrit.ovirt.org/#/c/27325/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/label/TextBoxLabelBase.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/label/TextBoxLabelBase.java: Line 29: public abstract class TextBoxLabelBase<T> extends ValueBoxBase<T> { Line 30: Line 31: private T value; Line 32: private String tooltipCaption; Line 33: private final TooltipPanel tooltipPanel = new TooltipPanel(true, this); Same comment like in EntityModelTextBox.java, it could be like this: private final TooltipPanel tooltipPanel = new TooltipPanel(true); Line 34: Line 35: Line 36: private TextBoxLabelBase(Renderer<T> renderer, Parser<T> parser) { Line 37: super(Document.get().createTextInputElement(), renderer, parser); http://gerrit.ovirt.org/#/c/27325/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/AbstractCellWithTooltip.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/AbstractCellWithTooltip.java: Line 44: /** Line 45: * Returns {@code true} if tool-tip should be shown for the given {@code parent} element. Line 46: */ Line 47: protected boolean showTooltip(Element parent, C value) { Line 48: tooltipPanel.applyTo(parent); Hm, is tooltipPanel.applyTo call necessary for computation within contentOverflows method? If not, we could move tooltipPanel.applyTo call into onBrowserEvent: ... tooltipPanel.applyTo(parent); tooltipPanel.setText(getTooltip(value)); ... Line 49: return contentOverflows(parent); Line 50: } Line 51: Line 52: /** http://gerrit.ovirt.org/#/c/27325/4/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 5: import com.google.gwt.user.client.ui.Label; Line 6: Line 7: public class LabelWithToolTip extends Label { Line 8: Line 9: private final TooltipPanel tooltipPanel = new TooltipPanel(true, this); Same comment like in EntityModelTextBox.java, it could be like this: private final TooltipPanel tooltipPanel = new TooltipPanel(true); Line 10: Line 11: public LabelWithToolTip() { Line 12: this("", -1); //$NON-NLS-1$ Line 13: } -- To view, visit http://gerrit.ovirt.org/27325 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4fc79a6aa132c65d3050499e5f00facbb5c358d3 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@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
