Alona Kaplan 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 The init of the 'mtu' depends on the 'export' value. Update the 'mtu' value in 'onExportChanged()' is problematic cause we need to distinguish whether it is part in the init process or not. If export is changed to false- 1. If part of the init process- the 'mtu' should be set to the getNetwork().getMtu() value. 2. If not part of the init- 'mtu' should be set to 'default value'. Passing the isInit value to 'onExportChanged()' complicates the code. The easiest way to make sure 'onExportChanged()' is not called as part of the init is to init the 'export' before the 'mtu'. 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 exte why? it is set to false on initMtu. 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 I want updateMtu() to be called just when export really changes. Unfortunately, despite of its name there are other cases when onExportChanged() is called. For example, updateDcLabels() calls onExportChanged() on a query success. As I wrote in the previous comment- updateMtu() shouldn't be called as part of the init process after initMtu. Moving updateMtu() to onExportChanged() can cause this problematic scenario. 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 It is not related to this patch. But there is no bug here, setMTUOverrideSupported is called from syncWithBackend before onExportChanged() is called. 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 updateMtu is called when hasMtu is changed. So hasMtu related code can't be here. But I agree, this class is very buggy and it is hard to maintain it. I send cleanup to this class in a separate patch. 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 fix here is not really related to this patch. Just a bug I found by accident. I don't want to add to this patch more unrelated changes. As I wrote in a previous comment. I will send cleanup to this class in a separate patch. 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? Currently it shouldn't. But I don't see the disadvantage of checking 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
