Eldan Shachar has posted comments on this change. Change subject: userportal\webadmin: Editing of template version for pools ......................................................................
Patch Set 3: (11 comments) https://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 change was removed - a lot of code was removed from this patch due to the decision not to support stateless VMs template-version changes. 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 Removed 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" Removed 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: } https://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<...> Removed Line 872: Line 873: @SuppressWarnings({ "unchecked", "rawtypes" }) Line 874: public AbstractVmPopupWidget(CommonApplicationConstants constants, Line 875: CommonApplicationResources resources, Line 2133: rngDeviceTab Line 2134: ); Line 2135: } Line 2136: Line 2137: private HashMap<Widget, Boolean> getAllTabsState() { > same, please use Map<> Removed Line 2138: HashMap<Widget, Boolean> originalState = new HashMap<Widget, Boolean>(); Line 2139: for (DialogTab dialogTab : allDialogTabs()) { Line 2140: dialogTab.getContentState(originalState); Line 2141: } https://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 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 l You're right about the purpose but the deactivation\activateion is still needed in order to allow the template values to be loaded without being overwritten by the original vm values. It's possible to call the updateAll() like you suggested instead of overriding the activated event (i.e. add it right after activateInstanceTypeManager()) but if someone else calls activateInst...() then updateAll() won't be called. 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 The editable fields in this dialog don't affect the VM so we don't need to disable it. Line 107: doChangeDefautlHost(template.getDedicatedVmForVds()); Line 108: setupWindowModelFrom(template); Line 109: Line 110: } else { https://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 t Change was removed 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 Removed 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 loadM Removed Line 261: BuilderExecutor.build(vm, getModel(), Line 262: new CoreVmBaseToUnitBuilder(), Line 263: new HwOnlyVmBaseToUnitBuilder()); Line 264: } else { https://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 o Removed 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 https://gerrit.ovirt.org/36510 To unsubscribe, visit https://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
