Tomas Jelinek has posted comments on this change. Change subject: webadmin: Create VM from Templates list view ......................................................................
Patch Set 1: (7 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 129: systemTreeSelectedItem = value; Line 130: } Line 131: } Line 132: Line 133: VmInterfaceCreatingManager addVmFromBlankTemplateNetworkManager = you could either extract this two anonymous classes to normal public classes or to parent as protected vars so you can re-use them on both list models Line 134: new VmInterfaceCreatingManager(new VmInterfaceCreatingManager.PostVnicCreatedCallback() { Line 135: @Override Line 136: public void vnicCreated(Guid vmId) { Line 137: // do nothing 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 called like this: setupNewVmModel(new UnitVmModle(new NewVmFromTemplateModelBehavior(template)); and than some specific setups, but not too much... 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 669: setcurrentVm(new VM()); Line 670: validateVM(model, name); Line 671: } Line 672: Line 673: protected UnitVmModelNetworkAsyncCallback createUnitVmModelNetworkAsyncCallback(VM vm, UnitVmModel model) { you could extract also this to the parent - the same method is in VmListModel Line 674: if (vm.getVmtGuid().equals(Guid.Empty)) { Line 675: return new UnitVmModelNetworkAsyncCallback(model, addVmFromBlankTemplateNetworkManager) { Line 676: @Override Line 677: public void executed(FrontendActionAsyncResult result) { 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: if (selectedTemplate.isBaseTemplate()) { baseTemplate = selectedTemplate; } than you will not need the boolean isSelectedTemplateBase flag and also the rest of the code will be simpler and faster - see next comments 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 IDs? 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 next check after the base template has been found. 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 == selectedTemplate in case it is the base - so instead of this if-else you can do directly: templateCouple = new TemplateWithVersion(baseTemplate, selectedTemplate); 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
