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(); If the correct flow of things depends on the order of calls, then something here is wrong. Why did you need to change it? 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 Removing this probably causes a null pointer exception when editing an external network. Line 101: Line 102: @Override Line 103: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 104: onExportChanged(); Line 105: updateMtu(); I think it's better to call updateMtu() from onExportChanged() rather than from here, all other logic depending on changes in the value of export is there. 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); I suspect there's a bug here. Since this is triggered only after the model is constructed, in the EditNetwork case it could override the non-changeability of an external network. 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() { If I correctly understand your purpose, you were trying to concentrate all code related to MTU value in one place to reduce complexity. However, the hasMtu code is still complex and buggy. I think it should also be put in this method, and be called both from syncWithBackend() and from onExportChanged(), and be computed according to the entire relevant state - whether the version supports it and whether the network is external. 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()); Both of these lines should be moved into corresponding methods calculating the desired state according to the two dependencies, as I described in another file. 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(); Null shouldn't be possible, should it? 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 <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Lior Vernia <[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
