Omer Frenkel has posted comments on this change. Change subject: core: Graphics Device CRUD ......................................................................
Patch Set 8: (6 comments) http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractGraphicsDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractGraphicsDeviceCommand.java: Line 11: public abstract class AbstractGraphicsDeviceCommand<T extends GraphicsParameters> extends CommandBase<T> { Line 12: Line 13: protected AbstractGraphicsDeviceCommand(T parameters) { Line 14: super(parameters); Line 15: setVmId(parameters.getDev().getVmId()); maybe you can do something like if (parameters.isVm()) { setVmId(..) } else { setTemplateId(..) } and then in canDoAction you can do: if (parameters.isVm() && getVm() == null) { // fail on vm not found } if (!parameters.isVm() && getVmTemplate() == null) { // fail on template not found } Line 16: } Line 17: Line 18: @Override Line 19: protected boolean canDoAction() { http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetGraphicsDevicesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetGraphicsDevicesQuery.java: Line 17: @Override Line 18: protected void executeQueryCommand() { Line 19: List<GraphicsDevice> result = new LinkedList<>(); Line 20: Line 21: // we must use getVmDeviceByVmIdTypeAndDevice since it supports user filtering thats ok, but we can also create a query for getVmDeviceByVmIdAndType to support filtering, shouldn't be complicated, up to you Line 22: List<VmDevice> spiceDevs = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdTypeAndDevice( Line 23: getParameters().getId(), Line 24: VmDeviceGeneralType.GRAPHICS, Line 25: VmDeviceType.SPICE.getName(), Line 25: VmDeviceType.SPICE.getName(), Line 26: getUserID(), Line 27: getParameters().isFiltered()); Line 28: if (spiceDevs != null && !spiceDevs.isEmpty()) { Line 29: result.add(GraphicsDevice.fromVmDevice(spiceDevs.get(0))); wil we have GRAPHICS device per type? if user want both he will have 2 devices? how will it work with multi monitors? Line 30: } Line 31: Line 32: List<VmDevice> vncDevs = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdTypeAndDevice( Line 33: getParameters().getId(), http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateGraphicsDeviceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateGraphicsDeviceCommand.java: Line 27: } Line 28: return result; Line 29: } Line 30: Line 31: boolean entityExists() { i think this can be saved with my suggestion in the abstract command, no? Line 32: if (getParameters().isVm()) { Line 33: Guid vmId = getParameters().getDev().getVmId(); Line 34: return vmId != null && getVmDAO().get(vmId) != null; Line 35: } else { http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsDevice.java: Line 12: Line 13: static { Line 14: typeToId = new HashMap<VmDeviceType, Guid>(); Line 15: typeToId.put(VmDeviceType.SPICE, Guid.createGuidFromString("00000000-0000-0000-0000-000000000000")); // $NON-NLS-1$ Line 16: typeToId.put(VmDeviceType.VNC, Guid.createGuidFromString("00000000-0000-0000-0000-000000000001")); // $NON-NLS-1$ once you hard code a string, you know something is not right.. why is this needed? why not use new id for each device? Line 17: } Line 18: Line 19: Line 20: private GraphicsDevice() { } http://gerrit.ovirt.org/#/c/25409/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsType.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import org.ovirt.engine.core.common.utils.VmDeviceType; Line 4: Line 5: public enum GraphicsType { this is not overlapping with DisplayType enum? are we going to remove the display type field from vm? Line 6: Line 7: SPICE(VmDeviceType.SPICE), Line 8: VNC(VmDeviceType.VNC); Line 9: -- To view, visit http://gerrit.ovirt.org/25409 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If9eed1ddb4aa8e8376ba5eff662f1bdf49fda800 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[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
