Lior Vernia has posted comments on this change.
Change subject: webadmin: Introducing AddRemoveRowWidget
......................................................................
Patch Set 1:
(4 comments)
I figured setting element IDs should be the responsibility of the derived
class, for two reasons:
1. It knows what values are backing it and so can decide better how to set IDs
(prime candidate for where to do this is createWidget()).
2. Tried to keep this base class as simple as possible, just the essentials for
orchestrating adding and removing rows, so oblivious to technical details such
as element IDs.
Could be changed if you think otherwise.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AddRemoveRowWidget.java
Line 84: addEntry(createGhostValue());
Line 85: }
Line 86:
Line 87: private void addEntry(T value) {
Line 88: final HorizontalPanel entry = new HorizontalPanel();
Alright, wasn't even aware of that, I'll try to achieve this using FlowPanel,
should be possible.
Line 89: entry.addStyleName(style.entryStyle());
Line 90: contentPanel.add(entry);
Line 91:
Line 92: final V widget = createWidget(value);
Line 96:
Line 97: PushButton button = createButton(item);
Line 98: entry.add(button);
Line 99:
Line 100: final boolean ghost = isGhost(value);
I'm also passing it to toggleGhost(), so the choice is either make two calls to
the same method or use a variable. I don't mind changing to two method calls.
Now, toggleGhost() doesn't really have to be passed the third parameter
becomingGhost because it can just do isGhost(value) itself, but the assumption
is that the derived class will always want to check whether it's becoming ghost
or non-ghost to decide how to render the widget, so I might as well pass it as
another argument and make the job easier on the derived class implementation.
Line 101: toggleGhost(value, widget, ghost);
Line 102: widget.addValueChangeHandler(new ValueChangeHandler<T>() {
Line 103:
Line 104: private boolean wasGhost = ghost;
Line 107: public void onValueChange(ValueChangeEvent<T> event) {
Line 108: T value = event.getValue();
Line 109: boolean becomingGhost = isGhost(value);
Line 110: if (becomingGhost != wasGhost) {
Line 111: ((PushButton)
entry.getWidget(entry.getWidgetCount() - 1)).setEnabled(!becomingGhost);
I completely agree and was actually contemplating doing it as you suggested in
3, just seemed too verbose so I opted for this. I'll fix it for the next
patchset.
Line 112: toggleGhost(value, widget, becomingGhost);
Line 113: wasGhost = becomingGhost;
Line 114: }
Line 115: }
Line 134: boolean ghostItem = isGhost(item.getFirst());
Line 135: final PushButton button =
Line 136: new PushButton(new Image(ghostItem ?
resources.increaseIcon() : resources.decreaseIcon()));
Line 137: button.addStyleName(style.buttonStyle());
Line 138: button.setEnabled(!ghostItem);
Enable for non-ghost, disable for ghost.
Line 139:
Line 140: button.addClickHandler(ghostItem ?
Line 141: new ClickHandler() {
Line 142:
--
To view, visit http://gerrit.ovirt.org/19530
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I15958e4b7c1fcf0ec08ea35eb32c690964f2a366
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
