Fred Rolland has posted comments on this change. Change subject: webadmin: Create VM from Templates list view ......................................................................
Patch Set 1: (6 comments) https://gerrit.ovirt.org/#/c/38286/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java: Line 635: } Line 636: Line 637: private void createVMFromTemplate() Line 638: { Line 639: VmTemplate template = getSelectedItem(); > most part of this method could be extracted to a parent class. It could be Done Line 640: UnitVmModel model = new UnitVmModel(new NewVmFromTemplateModelBehavior(template)); Line 641: model.setIsAdvancedModeLocalStorageKey(getEditTemplateAdvancedModelKey()); Line 642: setWindow(model); Line 643: model.setTitle(ConstantsManager.getInstance().getConstants().newVmTitle()); Line 639: VmTemplate template = getSelectedItem(); Line 640: UnitVmModel model = new UnitVmModel(new NewVmFromTemplateModelBehavior(template)); Line 641: model.setIsAdvancedModeLocalStorageKey(getEditTemplateAdvancedModelKey()); Line 642: setWindow(model); Line 643: model.setTitle(ConstantsManager.getInstance().getConstants().newVmTitle()); > Can delete one of the rows :) Done Line 644: model.setTitle(ConstantsManager.getInstance().getConstants().newVmTitle()); Line 645: model.setHelpTag(HelpTag.new_vm); Line 646: model.setHashName("new_vm"); //$NON-NLS-1$ Line 647: model.setIsNew(true); https://gerrit.ovirt.org/#/c/38286/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmFromTemplateModelBehavior.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmFromTemplateModelBehavior.java: Line 16: Line 17: @Override Line 18: protected void postInitTemplate(List<VmTemplate> templates) { Line 19: boolean isSelectedTemplateBase = selectedTemplate.isBaseTemplate(); Line 20: VmTemplate baseTemplate = null; > you could init this variable like this: Done Line 21: List<VmTemplate> relatedTemplates = new ArrayList<>(); Line 22: for (VmTemplate template : templates) { Line 23: if (template.getName().equals(selectedTemplate.getName())) { Line 24: relatedTemplates.add(template); Line 19: boolean isSelectedTemplateBase = selectedTemplate.isBaseTemplate(); Line 20: VmTemplate baseTemplate = null; Line 21: List<VmTemplate> relatedTemplates = new ArrayList<>(); Line 22: for (VmTemplate template : templates) { Line 23: if (template.getName().equals(selectedTemplate.getName())) { > this seems a bit dangerous... could you please try to rewrite it to check I Done Line 24: relatedTemplates.add(template); Line 25: Line 26: if (!isSelectedTemplateBase) { Line 27: if (selectedTemplate.getBaseTemplateId().equals(template.getId())) { Line 22: for (VmTemplate template : templates) { Line 23: if (template.getName().equals(selectedTemplate.getName())) { Line 24: relatedTemplates.add(template); Line 25: Line 26: if (!isSelectedTemplateBase) { > here you could check if (baseTemplate == null) - than you will not do the n Done Line 27: if (selectedTemplate.getBaseTemplateId().equals(template.getId())) { Line 28: baseTemplate = template; Line 29: } Line 30: } Line 33: Line 34: initTemplateWithVersion(relatedTemplates); Line 35: Line 36: TemplateWithVersion templateCouple; Line 37: if (selectedTemplate.isBaseTemplate()) { > and this whole check is not needed since the baseTemplate == selectedTempla Done Line 38: templateCouple = new TemplateWithVersion(selectedTemplate, selectedTemplate); Line 39: } else { Line 40: templateCouple = new TemplateWithVersion(baseTemplate, selectedTemplate); Line 41: } -- To view, visit https://gerrit.ovirt.org/38286 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0e5fd7e84bbea9405bf7d9e087b4281866daa954 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Fred Rolland <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[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
