Vojtech Szocs has posted comments on this change.
Change subject: webadmin: Rendered checkbox headers resizeable
......................................................................
Patch Set 2:
(4 comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/column/SafeHtmlCellWithTooltip.java
Line 30
Line 31
Line 32
Line 33
Line 34
Commit msg for d84adad [webadmin,userportal: Support column resizing for all
tables]:
<quote>
AbstractCellWithTooltip does two kinds of content overflow detection:
(a) scrollWidth with temporary CSS 'overflow:auto'
(b) clientHeight with temporary CSS 'whiteSpace:normal'
In practice, (a) is used for both header and body cells. In Firefox,
header cells (SafeHtmlCellWithTooltip) use <div> with 'display:block'
to work around Firefox-specific issue with scrollWidth.
</quote>
So SafeHtmlCellWithTooltip.contentOverflows override was meant to address a bug
in Firefox (IIRC, Firefox reported incorrect scrollWidth value).
Later commit 68ac3b1 [frontend: Fix for showTooltip condition in
AbstractCellWithTooltip] modified original implementation of methods:
AbstractCellWithTooltip.detectOverflowUsingScrollWidth
AbstractCellWithTooltip.detectOverflowUsingClientHeight
So it's possible that SafeHtmlCellWithTooltip.contentOverflows override isn't
necessary anymore, i.e. it's possible that
AbstractCellWithTooltip.contentOverflows now deals properly with content
overflow also in case of SafeHtmlCellWithTooltip's HTML.
Lior, can you please review changes in commit 68ac3b1 and verify that even
after you remove SafeHtmlCellWithTooltip.contentOverflows override, content
overflow detection logic (which determines the tooltip availability) indeed
works in recent Firefox?
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/header/ResizeableCheckboxHeader.java
Line 12: import com.google.gwt.user.cellview.client.Column;
Line 13:
Line 14: public class ResizeableCheckboxHeader<T> extends ResizableHeader<T> {
Line 15:
Line 16: CheckboxHeader checkboxHeaderDelegate;
In general, a Header is basically a wrapper around Cell with methods to drive
wrapped Cell behavior.
I'm pretty sure we discussed this before, but instead of using delegate
pattern, can't we simply do this instead:
* modify constructors in ResizableHeader:
public ResizableHeader(SafeHtml text, Column<T, ?> column,
HasResizableColumns<T> table) {
this(new SafeHtmlCellWithTooltip("click", "mousedown", "mousemove",
"mouseover"));
}
public ResizableHeader(Cell<T> headerCell, SafeHtml text, Column<T, ?>
column, HasResizableColumns<T> table) {
super(headerCell);
this.text = text;
this.column = column;
this.table = table;
}
* create SafeHtmlCheckboxCellWithTooltip (extends
AbstractCellWithTooltip<SafeHtml>) i.e. custom version of
com.google.gwt.cell.client.CheckboxCell (extends AbstractEditableCell<Boolean,
Boolean>)
* ResizeableCheckboxHeader would use SafeHtmlCheckboxCellWithTooltip instead of
CheckboxHeader/CheckboxCell delegate
Thinking about it some more, com.google.gwt.cell.client.CheckboxCell is used on
lots of places and changes I've described above would introduce more potential
for regressions. So I guess the delegate pattern is more safe and requires
fewer changes to the current code.
Line 17:
Line 18: public ResizeableCheckboxHeader(CheckboxHeader checkboxHeader,
Line 19: Column<T, ?> column,
Line 20: HasResizableColumns<T> table) {
Line 29: checkboxHeaderDelegate = checkboxHeader;
Line 30: }
Line 31:
Line 32: @Override
Line 33: public void onBrowserEvent(Context context, Element target,
NativeEvent event) {
Shouldn't we do following instead?
// Delegate event to wrapped header first, if necessary
if
(checkboxHeaderDelegate.getCell().getConsumedEvents().contains(event.getType()))
{
checkboxHeaderDelegate.onBrowserEvent(context, target, event);
}
// Process event
super.onBrowserEvent(context, target, event);
Line 34: if
(checkboxHeaderDelegate.getCell().getConsumedEvents().contains(event.getType()))
{
Line 35: checkboxHeaderDelegate.onBrowserEvent(context, target,
event);
Line 36: } else {
Line 37: super.onBrowserEvent(context, target, event);
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/resize/ColumnResizeCellTable.java
Line 97: */
Line 98: @Override
Line 99: public void addColumn(Column<T, ?> column, Header<?> header) {
Line 100: Header<?> wrappedHeader = (columnResizingEnabled && header
instanceof CheckboxHeader)
Line 101: ? new ResizeableCheckboxHeader<T>((CheckboxHeader)
header, column, this) : header;
I'd suggest to move this code into separate method, for example:
Header<?> maybeWrapHeader(Column<T, ?> column, Header<?> header) ...
and use it in addColumn method:
super.addColumn(column, maybeWrapHeader(column, header));
Line 102:
Line 103: super.addColumn(column, wrappedHeader);
Line 104:
Line 105: if (columnResizingEnabled) {
--
To view, visit http://gerrit.ovirt.org/20324
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie442350893282e7838e5216d277718e654f1d884
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
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
