Vojtech Szocs has posted comments on this change. Change subject: ui: adding discover host integration to AddHost view ......................................................................
Patch Set 13: (7 comments) http://gerrit.ovirt.org/#/c/27623/13/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 100: { Line 101: return getHostId() == null; Line 102: } Line 103: Line 104: public boolean getIsProvioning() { return getExternalDiscoveredHosts().getSelectedItem() != null; } Small typo - should be getIsProvisioning. Line 105: Line 106: private Guid privateHostId; Line 107: Line 108: public Guid getHostId() http://gerrit.ovirt.org/#/c/27623/13/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.java: Line 136: Label fetchResult; Line 137: Line 138: @UiField(provided = true) Line 139: @Path(value = "providerStatus.entity") Line 140: @WithElementId We have a convention to not include "editor" in element IDs. Please use following: @WithElementId("providerStatus") Line 141: EntityModelLabelEditor<String> providerStatusEditor; Line 142: Line 143: @UiField Line 144: @Path(value = "comment.entity") Line 378: HorizontalPanel passwordSection; Line 379: Line 380: @UiField Line 381: @Path(value = "provisionedHostSection.entity") Line 382: @WithElementId("provisionedHostSection") By default, element ID will contain the name of the annotated field. In this case, you could simply use: @WithElementId Line 383: HorizontalPanel provisionedHostSection; Line 384: Line 385: @UiField Line 386: @Path(value = "discoveredHostSection.entity") Line 383: HorizontalPanel provisionedHostSection; Line 384: Line 385: @UiField Line 386: @Path(value = "discoveredHostSection.entity") Line 387: @WithElementId("discoveredHostSection") Same as above, you could simply use: @WithElementId Line 388: HorizontalPanel discoveredHostSection; Line 389: Line 390: @UiField(provided = true) Line 391: @Ignore Line 398: public RadioButton rbPassword; Line 399: Line 400: @UiField(provided = true) Line 401: @Ignore Line 402: @WithElementId("rbProvisionedHost") Same as above, you could simply use: @WithElementId Line 403: public RadioButton rbProvisionedHost; Line 404: Line 405: @UiField(provided = true) Line 406: @Ignore Line 403: public RadioButton rbProvisionedHost; Line 404: Line 405: @UiField(provided = true) Line 406: @Ignore Line 407: @WithElementId("rbDiscoveredHost") Same as above, you could simply use: @WithElementId Line 408: public RadioButton rbDiscoveredHost; Line 409: Line 410: @UiField Line 411: @Ignore Line 745: } Line 746: } Line 747: }); Line 748: Line 749: rbDiscoveredHost.addClickHandler(new ClickHandler() { In general, the View itself should not be adding click or other kinds of handlers on its own; this should be the responsibility of Presenter(Widget) instead. Please look at HostPopupPresenterWidget.init method - this one actually drives View.edit, any handler registration on View should be done there. For stuff like model.getWhateverEvent.addListener, it's not really a big deal (except that it violates MVP design pattern rule where Presenter should be the one performing handler registration, like I wrote above). It's because UiCommon models are one-time-use per login session. However, for stuff like widget.addClickHandler, it _IS_ a big deal, because we don't remove handler after View is no longer in use, which might result in memory leaks, especially for non-singleton popups. So instead of doing widget.addClickHandler here (in View.edit method), do this in HostPopupPresenterWidget.init method instead, for example: public interface ViewDef ... { ... existing stuff ... HasClickHandlers getDiscoveredHostRadio(); } @Override public void init(HostModel model) { ... existing stuff ... addYourOwnListeners(model); } void addYourOwnListeners(HostModel model) { registerHandler(getView().getDiscoveredHostRadio().addClickHandler(new ClickHandler() { ... }); } and in HostPopupView: @Override public HasClickHandlers getDiscoveredHostRadio() { return rbDiscoveredHost; } Same thing also for other widgets like rbProvisionedHost. Line 750: @Override Line 751: public void onClick(ClickEvent event) { Line 752: if (rbDiscoveredHost.getValue()) { Line 753: object.getIsDiscorveredHosts().setEntity(true); -- To view, visit http://gerrit.ovirt.org/27623 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0faf667019760326019368f044ee16265d41a620 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
