Vojtech Szocs has posted comments on this change.

Change subject: userportal, webadmin: convert all grids (Cells) to PatternFly 
tooltips
......................................................................


Patch Set 20:

(23 comments)

https://gerrit.ovirt.org/#/c/37935/20//COMMIT_MSG
Commit Message:

Line 17: 
Line 18: The Cell infrastructure has been slightly reworked. Highlights:
Line 19: 
Line 20: * All custom Cells that simply provided some variation of tooltip 
behavior were removed.
Line 21:   Tooltips are now provided for free by the root objects in the Cell 
and Column hierarchy.
+1
Line 22: * Various cells/columns that simply rendered an image with a tooltip 
were replaced by
Line 23:   AbstractImageResourceColumn and ImageResourceCell.
Line 24: * Almost all Columns use the template-method pattern for rendering a 
tooltip. Simply
Line 25:   override the getTooltip method with tooltip contents. The default 
implementation


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractCell.java:

Line 64:         if (tooltipContent == null) {
Line 65:             tooltipContent = getTooltip(value);
Line 66:         }
Line 67: 
Line 68:         if (BrowserEvents.MOUSEOVER.equals(event.getType())) {
At this point, assuming that "tooltipContent" is still null (likely scenario?) 
- does it still make sense to proceed with TooltipMixin stuff?
Line 69:             TooltipMixin.configureTooltip(parent, tooltipContent, 
event);
Line 70:         }
Line 71: 
Line 72:         if (BrowserEvents.MOUSEOUT.equals(event.getType())) {


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractImageButtonCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractImageButtonCell.java:

Line 32:     }
Line 33: 
Line 34:     @Override
Line 35:     public Set<String> getConsumedEvents() {
Line 36:         Set<String> set = new HashSet<String>();
Why not simply:

 Set<String> set = new HashSet<>(super.getConsumedEvents());
 set.add(BrowserEvents.CLICK);
 return set;

?
Line 37:         set.add(BrowserEvents.CLICK);
Line 38:         set.addAll(super.getConsumedEvents());
Line 39:         return set;
Line 40:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractInputCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractInputCell.java:

Line 62:         if (tooltipContent == null) {
Line 63:             tooltipContent = getTooltip(value);
Line 64:         }
Line 65: 
Line 66:         if (BrowserEvents.MOUSEOVER.equals(event.getType())) {
Same comment as in AbstractCell.java
Line 67:             TooltipMixin.configureTooltip(parent, tooltipContent, 
event);
Line 68:         }
Line 69: 
Line 70:         if (BrowserEvents.MOUSEOUT.equals(event.getType())) {


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/CompositeCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/CompositeCell.java:

Line 65:         if (tooltipContent == null) {
Line 66:             tooltipContent = getTooltip(value);
Line 67:         }
Line 68: 
Line 69:         if (BrowserEvents.MOUSEOVER.equals(event.getType())) {
Same comment as in AbstractCell.java
Line 70:             TooltipMixin.configureTooltip(parent, tooltipContent, 
event);
Line 71:         }
Line 72: 
Line 73:         if (BrowserEvents.MOUSEOUT.equals(event.getType())) {


Line 135:             sb.appendHtmlConstant("</span>"); //$NON-NLS-1$
Line 136:         }
Line 137:         else {
Line 138:             throw new IllegalStateException("CompositeCell cannot 
render Cells that do not implement " //$NON-NLS-1$
Line 139:                     + 
"org.ovirt.engine.ui.common.widget.table.cell.Cell"); //$NON-NLS-1$
AFAIK, you could use Class.getName() here to avoid hard-coding fully-qualified 
class name:

 "CompositeCell cannot render Cells that do not implement " + Cell.getName()

This is because GWT supports using (some) Java class metadata at runtime:

http://www.gwtproject.org/doc/latest/RefJreEmulation.html#Package_java_lang
Line 140:         }
Line 141:     }
Line 142: 
Line 143:     public void setElementIdPrefix(String elementIdPrefix) {


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/EditTextCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/EditTextCell.java:

Line 21:     private com.google.gwt.cell.client.EditTextCell delegate = new 
com.google.gwt.cell.client.EditTextCell();
Line 22: 
Line 23:     @Override
Line 24:     public Set<String> getConsumedEvents() {
Line 25:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 26:         set.add(BrowserEvents.CLICK);
Line 27:         set.add(BrowserEvents.KEYUP);
Line 28:         set.add(BrowserEvents.KEYDOWN);
Line 29:         set.add(BrowserEvents.BLUR);


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ImageButtonCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ImageButtonCell.java:

Line 22:  * Cell that renders ActionButtonDefinition-like image buttons. 
Supports tooltips.
Line 23:  *
Line 24:  * @param <T>
Line 25:  *            The data type of the cell (the model)
Line 26:  * TODO rename abstract?
It's possible, but developers can see it's "abstract" in their IDE anyway :-)

(Personally I'm not a big fan of Abstract* names for all abstract Java classes, 
the name should be meaningful enough I guess.)
Line 27:  */
Line 28: public abstract class ImageButtonCell<T> extends AbstractCell<T> {
Line 29: 
Line 30:     interface CellTemplate extends SafeHtmlTemplates {


Line 53:      * Events to sink.
Line 54:      */
Line 55:     @Override
Line 56:     public Set<String> getConsumedEvents() {
Line 57:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 58:         set.addAll(super.getConsumedEvents());
Line 59:         set.add(BrowserEvents.CLICK);
Line 60:         return set;
Line 61:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/LinkCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/LinkCell.java:

Line 23: public class LinkCell extends TextCell {
Line 24: 
Line 25:     @Override
Line 26:     public Set<String> getConsumedEvents() {
Line 27:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 28:         set.add(BrowserEvents.CLICK);
Line 29:         set.addAll(super.getConsumedEvents());
Line 30:         return set;
Line 31:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ListModelListBoxCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ListModelListBoxCell.java:

Line 50:     }
Line 51: 
Line 52:     @Override
Line 53:     public Set<String> getConsumedEvents() {
Line 54:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 55:         set.add(BrowserEvents.CHANGE);
Line 56:         set.addAll(super.getConsumedEvents());
Line 57:         return set;
Line 58:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ResizableCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ResizableCell.java:

Line 11:  */
Line 12: public class ResizableCell extends SafeHtmlCell {
Line 13:     @Override
Line 14:     public Set<String> getConsumedEvents() {
Line 15:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 16:         set.add(BrowserEvents.CLICK);
Line 17:         set.add(BrowserEvents.MOUSEDOWN);
Line 18:         set.add(BrowserEvents.MOUSEMOVE);
Line 19:         set.addAll(super.getConsumedEvents());


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ResizableCheckboxCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/ResizableCheckboxCell.java:

Line 11:  */
Line 12: public class ResizableCheckboxCell extends SafeHtmlCell {
Line 13:     @Override
Line 14:     public Set<String> getConsumedEvents() {
Line 15:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 16:         set.add(BrowserEvents.CLICK);
Line 17:         set.add(BrowserEvents.MOUSEDOWN);
Line 18:         set.add(BrowserEvents.MOUSEMOVE);
Line 19:         set.add(BrowserEvents.CHANGE);


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/SafeHtmlCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/SafeHtmlCell.java:

Line 10: 
Line 11:     @Override
Line 12:     public void render(Context context, SafeHtml value, 
SafeHtmlBuilder sb, String id) {
Line 13:         if (value != null) {
Line 14:             sb.appendHtmlConstant("<div id=\" " + id + "\" 
style='display:block'>"); //$NON-NLS-1$ //$NON-NLS-2$
Why the added space?
Line 15:             sb.append(value);
Line 16:             sb.appendHtmlConstant("</div>"); //$NON-NLS-1$
Line 17:         }
Line 18:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/StatusCompositeCellWithElementId.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/StatusCompositeCellWithElementId.java:

Line 70:             cell.render(context, hasCell.getValue(value), sb, id);
Line 71:         }
Line 72:         else {
Line 73:             throw new IllegalStateException("StatusCompositeCell 
cannot render Cells that do not implement " //$NON-NLS-1$
Line 74:                     + 
"org.ovirt.engine.ui.common.widget.table.cell.Cell"); //$NON-NLS-1$
Same comment as in CompositeCell.java
Line 75:         }
Line 76:     }
Line 77: 
Line 78:     @Override


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/TextCell.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/TextCell.java:

Line 18:  *
Line 19:  * If truncation is enabled, and if the text doesn't fit in the parent 
element, it is truncated.
Line 20:  *
Line 21:  * There are two types of truncation. You can specify a length in 
characters, or if you don't, overflow
Line 22:  * will be detected and truncated via CSS 'text-overflow: ellipse'.
ellipsis :-)

(I'm really nit-picky today, sorry.)
Line 23:  *
Line 24:  * Truncation can also be disabled.
Line 25:  *
Line 26:  * When text is truncated, the full text will be rendered in a 
tooltip, unless a different tooltip is manually


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/DiskContainersColumn.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/DiskContainersColumn.java:

Line 35:     }
Line 36: 
Line 37:     @Override
Line 38:     public SafeHtml getTooltip(Disk object) {
Line 39:         if (object.getNumberOfVms() < 2) {
Is this condition consistent with original behavior?

(I mean, original code didn't care about number of items in 
"object.getVmNames()" collection.)
Line 40:             return null;
Line 41:         }
Line 42:         return 
SafeHtmlUtils.fromString(StringUtils.join(object.getVmNames(), ", ")); 
//$NON-NLS-1$
Line 43:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/header/ResizableHeader.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/header/ResizableHeader.java:

Line 115:         this(text, column, table,
Line 116:                 new SafeHtmlCell() {
Line 117:                     @Override
Line 118:                     public Set<String> getConsumedEvents() {
Line 119:                         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 120:                         set.add(BrowserEvents.CLICK);
Line 121:                         set.addAll(super.getConsumedEvents());
Line 122:                         return set;
Line 123:                     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/header/ResizeableCheckboxHeader.java
File 
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/header/ResizeableCheckboxHeader.java:

Line 22:         super(checkboxHeader.getTitle(), column, table,
Line 23:                 new SafeHtmlCell() {
Line 24:                     @Override
Line 25:                     public Set<String> getConsumedEvents() {
Line 26:                         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 27:                         set.add(BrowserEvents.CLICK);
Line 28:                         set.add(BrowserEvents.CHANGE);
Line 29:                         set.add(BrowserEvents.KEYDOWN);
Line 30:                         set.addAll(super.getConsumedEvents());


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/widget/extended/vm/AbstractConsoleButtonCell.java
File 
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/widget/extended/vm/AbstractConsoleButtonCell.java:

Line 35:         @Template("<div id=\"{0}\" class=\"{1}\" 
data-class=\"consoleButton\" />")
Line 36:         SafeHtml consoleButton(String id, String className);
Line 37:     }
Line 38: 
Line 39:     private final CellTemplate template = 
GWT.create(CellTemplate.class);
Could be static & initialized on first use (but we can do that later).
Line 40: 
Line 41:     private final ConsoleButtonCommand command;
Line 42: 
Line 43:     private final String enabledCss;


Line 55:      * Events to sink.
Line 56:      */
Line 57:     @Override
Line 58:     public Set<String> getConsumedEvents() {
Line 59:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 60:         set.addAll(super.getConsumedEvents());
Line 61:         set.add(BrowserEvents.CLICK);
Line 62:         return set;
Line 63:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/cell/CustomSelectionCell.java
File 
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/cell/CustomSelectionCell.java:

Line 63:      * to additional events.
Line 64:      */
Line 65:     @Override
Line 66:     public Set<String> getConsumedEvents() {
Line 67:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 68:         set.add(BrowserEvents.CHANGE);
Line 69:         set.addAll(super.getConsumedEvents());
Line 70:         return set;
Line 71:     }


https://gerrit.ovirt.org/#/c/37935/20/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/cell/MenuCell.java
File 
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/cell/MenuCell.java:

Line 38:     }
Line 39: 
Line 40:     @Override
Line 41:     public Set<String> getConsumedEvents() {
Line 42:         Set<String> set = new HashSet<String>();
Same comment as in AbstractImageButtonCell.java
Line 43:         set.add(BrowserEvents.CLICK);
Line 44:         set.addAll(super.getConsumedEvents());
Line 45:         return set;
Line 46:     }


-- 
To view, visit https://gerrit.ovirt.org/37935
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54820570f3dd9cd8306ba63a263c480b33eeb65d
Gerrit-PatchSet: 20
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Martin Betak <mbe...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@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

Reply via email to