Lior Vernia 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
Yes, originally put the verified checkmark only after checking that tooltips
are still displayed correctly, and now also verified that it's the additional
condition from 68ac3b1 that addresses the need for tooltip in cells.
....................................................
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;
If I understand correctly what you described then I did more or less the same
thing, except for using a delegate for the CheckboxCell behavior instead of
duplicating some of CheckboxCell's code inside the proposed
SafeHtmlCheckboxCellWithTooltip (which as you said in the end is indeed more
error prone, less maintainable and more cumbersome).
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) {
At the moment I don't think it matters, because ResizableHeader and
CheckboxHeader consume disjoint sets of events.
Moving to a theoretical discussion then, I'm not sure what the right thing to
do would be if there were some shared event. Should the two cells handle it? I
imagine that could result in some ill-defined behavior.
Also, the order might be important, so which cell should get to handle it
first? (that's actually something that's already apparent here, when I give
CheckboxHeader precedence over ResizableHeader, but again I didn't think it
mattered because the sets of consumed events are disjoint)
So to summarize, I don't mind changing if you think that's more correct.
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;
No problem, was actually contemplating that when I wrote the code (decided
against because I didn't wanna pass on the header AND the column to another
method). Will fix in next patchset.
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
