Alexander Wels has posted comments on this change. Change subject: webadmin: combine datacenter/cluster in host popup ......................................................................
Patch Set 3: (4 comments) https://gerrit.ovirt.org/#/c/41240/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/GroupedListModelListBox.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/GroupedListModelListBox.java: Line 22: import com.google.gwt.user.client.ui.Label; Line 23: import com.google.gwt.user.client.ui.ListBox; Line 24: import com.google.gwt.user.client.ui.Widget; Line 25: Line 26: public abstract class GroupedListModelListBox<T> extends ListModelListBox<T> { > please add some javadoc describing the widget What? it isn't obvious? Line 27: interface Style extends CssResource { Line 28: String container(); Line 29: String listBox(); Line 30: String labelContainer(); Line 62: } Line 63: Line 64: @Override Line 65: protected void initWidget(Widget widget) { Line 66: container = DataCenterClusterListUiBinder.uiBinder.createAndBindUi(this); > this seems off. Isn't this widget generic? It is, but it wasn't before and I missed renaming this. The interface is wrong too. Line 67: super.initWidget(container); Line 68: container.insert(widget, 0); Line 69: getListBox().addChangeHandler(new ChangeHandler() { Line 70: @Override Line 79: return container.getWidget(0); Line 80: } Line 81: Line 82: /** Line 83: * Set the acceptable values as well as generating the appropriate OptionGroups based on the data center > shouldn't mention data centers since (if?) this is a generic widget It shouldn't Line 84: * names. Line 85: */ Line 86: @Override Line 87: public void setAcceptableValues(Collection<T> newValues) { https://gerrit.ovirt.org/#/c/41240/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java: Line 1861 Line 1862 Line 1863 Line 1864 Line 1865 > no idea what this does. Is this the "partial cluster" thing you were referr Yes, it would popuplate the clusters with partially filled cluster model, which never 'equal' the actual models. So it would mess with the list box checking if the model being added already exists in the list box. -- To view, visit https://gerrit.ovirt.org/41240 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If42ada78c0b5e72509f0a5a6a5d8cfcefc3ed7d5 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
