Vojtech Szocs has posted comments on this change.
Change subject: webadmin: Rendered checkbox headers resizeable
......................................................................
Patch Set 2:
(4 comments)
Proposed one more suggestion in ResizeableCheckboxHeader.java - let me know
what you think, if it requires significant change to existing patch, I'm OK
with current delegate approach.
....................................................
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
Thanks!
....................................................
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;
Exactly, you understood it perfectly :-)
One more idea that comes to my mind is modifying ResizableHeader to become
ResizableHeader<T, C> extends Header<C>
so you could use Cell<Boolean> (CheckboxCell) or Cell<SafeHtml>
(SafeHtmlCellWithTooltip).
We would replace ResizableHeader usage with SafeHtmlResizableHeader that uses
SafeHtmlCellWithTooltip
SafeHtmlResizableHeader<T> extends ResizableHeader<T, SafeHtml>
ResizeableCheckboxHeader could then use CheckboxCell and become
ResizeableCheckboxHeader<T> extends ResizableHeader<T, Boolean>
What do you think? (Alternatively, we can stay with current delegate approach,
I just wanted to find solution that attempts to make best use of class
inheritance, before resorting to use delegate approach.)
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) {
This reminds me of com.google.gwt.cell.client.CompositeCell - you pass ordered
list of cells and composite cell iterates over them and does
Set<String> events = cell.getConsumedEvents();
...
theConsumedEvents.addAll(events);
CompositeCell's onBrowserEvent eventually delegates to onBrowserEvent for each
wrapped cell.
So theoretically speaking, I see nothing wrong with passing same event via
onBrowserEvent to different cells. If the given cell says it handles event X,
it should have the possibility to handle it via onBrowserEvent, and possibly do
stuff like event stop propagation and/or event cancelling.
Regarding the order, since ResizeableCheckboxHeader is meant to be a wrapper
around CheckboxHeader, I think that wrapped cell should receive the event first.
In my original comment, I wanted to say something like "if
ResizeableCheckboxHeader's cell says it handles same event(s) as wrapped
CheckboxHeader's cell, ResizeableCheckboxHeader's cell should also have the
possibility to handle it via onBrowserEvent."
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;
Thanks! I've meant my original comment mainly from code readability
perspective, having a separate method to contain possible header replacement
logic.
Passing column argument might seem weird, but it's inevitable because
ResizableHeader needs column so that ColumnResizeHandler can resize it via
HasResizableColumns interface.
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
