Tomas Jelinek has posted comments on this change. Change subject: userportal, webadmin: [Fix] Disable per arch the Pool SPICE proxy ......................................................................
Patch Set 7: (2 comments) http://gerrit.ovirt.org/#/c/25753/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java: Line 775: spiceProxyEnabledCheckboxWithInfoIcon.setExplanation(applicationTemplates.italicText(explanation)); Line 776: } Line 777: Line 778: public void setSpiceProxyEnabled(boolean enabled) { Line 779: unitVmModel.getSpiceProxyEnabled().setIsChangable(enabled); This class is a view - it is not supposed to do any changing of the model (since we are trying to do MVP and not MVC.). Only the line: spiceProxyEnabledCheckboxWithInfoIcon.setLabelEnabled(enabled); is supposed to be here. The rest should be either on the UnitVmModel, some behavior class or on the presenter widget. Since the rest of the logic around enabling/disabling this fields is on the PoolModelBehaviorBase.initialize() I would just enrich it by the new conditions. (btw the logic related to the messages and calling the setSpiceProxyEnabled() are correctly on the BasePoolPopupPresenterWidget). Line 780: spiceProxyEnabledCheckboxWithInfoIcon.setLabelEnabled(enabled); Line 781: Line 782: if (!enabled) { Line 783: unitVmModel.getSpiceProxyEnabled().setEntity(enabled); http://gerrit.ovirt.org/#/c/25753/7/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/pool/BasePoolPopupPresenterWidget.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/pool/BasePoolPopupPresenterWidget.java: Line 31: model.getDataCenterWithClustersList().getSelectedItemChangedEvent().addListener(new IEventListener() { Line 32: @Override Line 33: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 34: if (model.getSelectedCluster() != null) { Line 35: setSpiceProxyOverrideExplanation(model.getSelectedCluster(), model.getOSType()); Since the enabling / disabling depends also on the OS type, you need to also listen to OS type changes and call the setSpiceProxyOverrideExplanation also when the osType changes. And since you are anyway touching this code I'd like to ask you to also rename the setSpiceProxyOverrideExplanation to updateSpiceProxyOverrideExplanation Thanx. Line 36: } Line 37: Line 38: } Line 39: }); -- To view, visit http://gerrit.ovirt.org/25753 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifb2c22954a411206ac39dee05f08432e21989b16 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gustavo Frederico Temple Pedrosa <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Leonardo Bianconi <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Vitor de Lima <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
