Frank Kobzik has posted comments on this change. Change subject: engine: Change DisplayType semantics ......................................................................
Patch Set 32: (5 comments) http://gerrit.ovirt.org/#/c/30837/32/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java: Line 499: ? null Line 500: : entity.getGraphicsInfos().get(graphicsType); Line 501: Line 502: model.setDisplay(new Display()); Line 503: model.getDisplay().setType(graphicsTypeString); > The values of a backend enum (GraphicsType) should never be directly copied I changed it so now DisplayType is used. I wrote small private static function to do it. I don't know if I should have used new map(Graphics x, Display y) for that. I didn't did it that way because I see no potential public usage of it. Line 504: model.getDisplay().setAddress(graphicsInfo == null ? null : graphicsInfo.getIp()); Line 505: Integer displayPort = graphicsInfo == null ? null : graphicsInfo.getPort(); Line 506: model.getDisplay().setPort(displayPort == null || displayPort.equals(-1) ? null : displayPort); Line 507: Integer displaySecurePort = graphicsInfo == null ? null : graphicsInfo.getTlsPort(); Line 516: } Line 517: if (entity.getDefaultDisplayType() != null) { Line 518: model.setDisplay(new Display()); Line 519: GraphicsType graphicsType = deriveGraphicsType(entity.getGraphicsInfos(), null); Line 520: model.getDisplay().setType(graphicsType == null ? null : graphicsType.name().toLowerCase()); I believe i did this wrong. This branch applies for stopped VM and graphics infos don't have to exist at this moment. Line 521: } Line 522: } Line 523: if (entity.getLastStopTime() != null) { Line 524: model.setStopTime(DateMapper.map(entity.getLastStopTime(), null)); Line 790: } Line 791: Line 792: // for backwards compatibility Line 793: // if vm has multiple framebuffers, returns SPICE Line 794: @Mapping(from = org.ovirt.engine.core.common.businessentities.DisplayType.class, to = String.class) > If this method used outside of this class? If not please change it to "priv This is something I forgot to delete. Thanks. Line 795: public static GraphicsType deriveGraphicsType(Map<GraphicsType, GraphicsInfo> graphicsInfos, String incoming) { Line 796: if (graphicsInfos != null) { Line 797: if (graphicsInfos.containsKey(GraphicsType.SPICE)) { Line 798: return GraphicsType.SPICE; Line 791: Line 792: // for backwards compatibility Line 793: // if vm has multiple framebuffers, returns SPICE Line 794: @Mapping(from = org.ovirt.engine.core.common.businessentities.DisplayType.class, to = String.class) Line 795: public static GraphicsType deriveGraphicsType(Map<GraphicsType, GraphicsInfo> graphicsInfos, String incoming) { > The "incoming" parameter is never used, remove it. Done Line 796: if (graphicsInfos != null) { Line 797: if (graphicsInfos.containsKey(GraphicsType.SPICE)) { Line 798: return GraphicsType.SPICE; Line 799: } Line 800: if (graphicsInfos.containsKey(GraphicsType.VNC)) { Line 801: return GraphicsType.VNC; Line 802: } Line 803: } Line 804: return null; > This looks like business logic. Can this be moved to the backend? If you ar Hmm, I don't think this belongs to backend. GraphicsInfos is the new representation of dynamic graphics data in backend. Isn't it a client's (in this case restapi) responsibility to adapt to the new representation? Line 805: } Line 806: Line 807: @Mapping(from = String.class, to = OriginType.class) Line 808: public static OriginType map(String type, OriginType incoming) { -- To view, visit http://gerrit.ovirt.org/30837 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0907c9086f49f909250d9956356a0251954f1427 Gerrit-PatchSet: 32 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@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
