Tomas Jelinek has posted comments on this change. Change subject: userportal\webadmin: Editing of template version for stateless VMs and pools ......................................................................
Patch Set 3: (13 comments) http://gerrit.ovirt.org/#/c/36510/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/dialog/tab/DialogTab.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/dialog/tab/DialogTab.java: Line 125: public void disableContent() { Line 126: disable(getContent()); Line 127: } Line 128: Line 129: public void setContentState(HashMap<Widget, Boolean> originalState) { - please don't use specific implementations of maps - rather use Map<Widget, Boolean> Line 130: setState(getContent(), originalState); Line 131: } Line 132: Line 133: public void getContentState(HashMap<Widget, Boolean> originalState) { Line 129: public void setContentState(HashMap<Widget, Boolean> originalState) { Line 130: setState(getContent(), originalState); Line 131: } Line 132: Line 133: public void getContentState(HashMap<Widget, Boolean> originalState) { - the getter in java is typically something which returns the value and not fills them into the argument. Please rename this method to "fillContentStateInto(...). - same, please don't use specific implementations of maps - rather use Map<Widget, Boolean> Line 134: getState(getContent(), originalState); Line 135: } Line 136: Line 137: private void getState(Widget content, HashMap<Widget, Boolean> originalState) { Line 133: public void getContentState(HashMap<Widget, Boolean> originalState) { Line 134: getState(getContent(), originalState); Line 135: } Line 136: Line 137: private void getState(Widget content, HashMap<Widget, Boolean> originalState) { - same, please rename to "fillStateInto" - same, please don't use specific implementations of maps - rather use Map<Widget, Boolean> Line 138: if (content instanceof IndexedPanel) { Line 139: for (int i = 0; i < ((IndexedPanel) content).getWidgetCount(); i++) { Line 140: getState(((IndexedPanel) content).getWidget(i), originalState); Line 141: } http://gerrit.ovirt.org/#/c/36510/3/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 867: private final CommonApplicationConstants constants; Line 868: Line 869: private final Map<TabName, DialogTab> tabMap = new HashMap<TabName, DialogTab>(); Line 870: Line 871: private HashMap<Widget, Boolean> tabsState = null; same, please use Map<...> Line 872: Line 873: @SuppressWarnings({ "unchecked", "rawtypes" }) Line 874: public AbstractVmPopupWidget(CommonApplicationConstants constants, Line 875: CommonApplicationResources resources, Line 1644: Line 1645: object.getTemplateChanged().getEntityChangedEvent().addListener(new IEventListener<EventArgs>() { Line 1646: @Override Line 1647: public void eventRaised(Event<? extends EventArgs> ev, Object sender, EventArgs args) { Line 1648: Boolean enabled = object.getTemplateChanged().getEntity(); > We load the model with the new template data, which may be totally differen OK Line 1649: if (Boolean.TRUE.equals(enabled)) { Line 1650: tabsState = getAllTabsState(); Line 1651: disableAllTabs(); Line 1652: templateEditor.setEnabled(tabsState.get(templateEditor)); // if template editing was allowed -> keep it enabled Line 2133: rngDeviceTab Line 2134: ); Line 2135: } Line 2136: Line 2137: private HashMap<Widget, Boolean> getAllTabsState() { same, please use Map<> Line 2138: HashMap<Widget, Boolean> originalState = new HashMap<Widget, Boolean>(); Line 2139: for (DialogTab dialogTab : allDialogTabs()) { Line 2140: dialogTab.getContentState(originalState); Line 2141: } http://gerrit.ovirt.org/#/c/36510/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingPoolModelBehavior.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingPoolModelBehavior.java: Line 93: } Line 94: updateRngDevice(template.getId()); Line 95: getModel().getCustomPropertySheet().deserialize(template.getCustomProperties()); Line 96: Line 97: if (!pool.getVmtGuid().equals(template.getId())) { > it's based on the logic in NewPoolModelBehavior::template_SelectedItemChang OK Line 98: if (!templateVersionWasChanged) { Line 99: deactivateInstanceTypeManager(new InstanceTypeManager.ActivatedListener() { Line 100: @Override Line 101: public void activated() { Line 95: getModel().getCustomPropertySheet().deserialize(template.getCustomProperties()); Line 96: Line 97: if (!pool.getVmtGuid().equals(template.getId())) { Line 98: if (!templateVersionWasChanged) { Line 99: deactivateInstanceTypeManager(new InstanceTypeManager.ActivatedListener() { If I understand the purpose of this templateVersionChanged properly it is like this: - load the dialog - change the template - remember it was changed - change it back - updateAll() to get to the original state If I'm right than you don't need the activation and deactivation around the updateAll() - you just need to call the getInstanceTypeManager().updateAll() on line 112. Line 100: @Override Line 101: public void activated() { Line 102: getInstanceTypeManager().updateAll(); Line 103: } Line 102: getInstanceTypeManager().updateAll(); Line 103: } Line 104: }); Line 105: } Line 106: templateVersionWasChanged = true; I'd say you need to handle also the getModel().getTemplateChanged() in this method otherwise the dialog will not get disabled. Line 107: doChangeDefautlHost(template.getDedicatedVmForVds()); Line 108: setupWindowModelFrom(template); Line 109: Line 110: } else { http://gerrit.ovirt.org/#/c/36510/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingVmModelBehavior.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingVmModelBehavior.java: Line 50: boolean templateVersionWasChanged; Line 51: Line 52: public ExistingVmModelBehavior(VM vm) Line 53: { Line 54: this.vm = vm; please delete this constructor - the template_SelectedItemChanged expects the oldVm to be non-null and fails on NPE without having it set - e.g. by having this constructor you expose a constructor which creates an object in an invalid state. Line 55: } Line 56: Line 57: public ExistingVmModelBehavior(VM vm, VM oldVm) Line 58: { Line 183: getModel().getTemplateChanged().setEntity(true); Line 184: Line 185: if (!templateVersionWasChanged) { Line 186: templateVersionWasChanged = true; Line 187: deactivateInstanceTypeManager(new InstanceTypeManager.ActivatedListener() { same as in existing pool Line 188: @Override Line 189: public void activated() { Line 190: getInstanceTypeManager().updateAll(); Line 191: } Line 256: } Line 257: Line 258: @Override Line 259: protected void buildModel(VmBase vm) { Line 260: if (templateVersionWasChanged) { // instanceManager is disabled and the regular buildModel copies unwanted fields could this happen? because it is called only from line 204 (using the loadModelFromVM() ) and: - if templateVersionWasChanged == true when entering the else branch on line 197 than on line 198 it turns false - if templateVersionWasChanged == false when entering the else branch on line 197 than it will stay false Line 261: BuilderExecutor.build(vm, getModel(), Line 262: new CoreVmBaseToUnitBuilder(), Line 263: new HwOnlyVmBaseToUnitBuilder()); Line 264: } else { http://gerrit.ovirt.org/#/c/36510/3/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2493: Line 2494: @DefaultStringValue("This field is not a valid Guid (use 0-9,A-F format: 00000000-0000-0000-0000-000000000000)") Line 2495: String invalidGuidMsg(); Line 2496: Line 2497: @DefaultStringValue("Template version was changed, confirm change for further editing") Not sure what you mean by "confirm". You mean just to save the dialog and open again and than start editing? Line 2498: String templateChangeMessage(); Line 2499: Line 2500: @DefaultStringValue("New template version will be applied on the next vm start, no more changes are possible") Line 2501: String templateChangeRunningMessage(); -- To view, visit http://gerrit.ovirt.org/36510 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53686dae694f6ff826bdbeaada1e28fc55fd8d30 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
