Lior Vernia has posted comments on this change. Change subject: webadmin: Network dialog- displays default MTU if 'has MTU' is unchecked ......................................................................
Patch Set 4: (7 comments) http://gerrit.ovirt.org/#/c/28099/4/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkModel.java: Line 38: getVLanTag().setEntity((getNetwork().getVlanId() == null ? Integer.valueOf(0) : getNetwork().getVlanId())); Line 39: initIsVm(); Line 40: getExport().setEntity(getNetwork().isExternal()); Line 41: getExport().setIsChangable(false); Line 42: initMtu(); > The init of the 'mtu' depends on the 'export' value. Notice that export isn't changeable. There's (at least currently) only the init process. I see no harm in calling initMtu() and then setting export, letting it modify MTU in case the network is external. Line 43: getExternalProviders().setIsChangable(false); Line 44: getNetworkLabel().setSelectedItem(getNetwork().getLabel()); Line 45: toggleProfilesAvailability(); Line 46: } http://gerrit.ovirt.org/#/c/28099/4/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NetworkModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NetworkModel.java: Line 125 Line 126 Line 127 Line 128 Line 129 > why? it is set to false on initMtu. But setting export happens before initMtu() at the moment (you changed it to be like that). And then events are triggered that count on hasMtu. I may be wrong, but please verify this case. Line 101: Line 102: @Override Line 103: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 104: onExportChanged(); Line 105: updateMtu(); > I want updateMtu() to be called just when export really changes. As far as I can see, the reason for these calls is to update the QoS/label fields. So that code can be extracted into corresponding methods, and have those called instead. onExportChanged() will need to call these methods as well, of course. Line 106: } Line 107: }); Line 108: Line 109: setNetworkLabel(new ListModel<String>()); Line 367: } else { Line 368: if (this.mtuOverrideSupported != mtuOverrideSupported) { Line 369: initMtu(); Line 370: } Line 371: getHasMtu().setIsChangable(true); > It is not related to this patch. In the case of EditNetwork, onExportChanged is called before syncWithBackend. Line 372: } Line 373: this.mtuOverrideSupported = mtuOverrideSupported; Line 374: } Line 375: Line 722: private void updateVlanTagChangeability() { Line 723: getVLanTag().setIsChangable(getHasVLanTag().getEntity()); Line 724: } Line 725: Line 726: private void updateMtu() { > updateMtu is called when hasMtu is changed. So hasMtu related code can't be hasMtu related code can be here. As I mentioned, this method doesn't have to be triggered on the entityChangedEvent of hasMtu, it can be called from various locations (those that may affect it) explicitly, i.e. syncWithBackend() and onExportChanged(). And maybe from construction/initialization. These are not changed that are unrelated to this patch, they should be part of this. Line 727: getMtu().setIsChangable(getHasMtu().getEntity() && !getExport().getEntity()); Line 728: Line 729: if (getExport().getEntity() || getHasMtu().getEntity()) { Line 730: getMtu().setEntity(null); http://gerrit.ovirt.org/#/c/28099/4/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NewNetworkModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NewNetworkModel.java: Line 104: protected void onExportChanged() { Line 105: boolean externalNetwork = (Boolean) getExport().getEntity(); Line 106: getExternalProviders().setIsChangable(externalNetwork); Line 107: getIsVmNetwork().setIsChangable(!externalNetwork && isSupportBridgesReportByVDSM()); Line 108: getHasMtu().setIsChangable(!externalNetwork && isMTUOverrideSupported()); > The fix here is not really related to this patch. Just a bug I found by acc Okay. Line 109: if (externalNetwork) { Line 110: getIsVmNetwork().setEntity(true); Line 111: getHasMtu().setEntity(false); Line 112: } http://gerrit.ovirt.org/#/c/28099/4/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/renderer/MtuRenderer.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/renderer/MtuRenderer.java: Line 15: private final ApplicationMessages messages = GWT.create(ApplicationMessages.class); Line 16: Line 17: @Override Line 18: public String render(Integer mtu) { Line 19: return mtu == null || mtu == 0 ? messages.defaultMtu(defaultMtu) : mtu.toString(); > Currently it shouldn't. But I don't see the disadvantage of checking it. Then you should check it in other places as well, e.g. in ItemInfoPanel. Line 20: } Line 21: -- To view, visit http://gerrit.ovirt.org/28099 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f813fd292b56407c97ae69c1580061bbc0eef04 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
