Skip to content

Commit

Permalink
Merge pull request #2427 from concord-consortium/185893761-drawing-lo…
Browse files Browse the repository at this point in the history 
…g-cleanup

Image logging cleanup
  • Loading branch information
@bgoldowsky
bgoldowsky authored Nov 4, 2024
2 parents 98baf71 + 09430af commit 8c24196
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 74 deletions.
18 changes: 12 additions & 6 deletions src/plugins/drawing/components/drawing-layer.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ describe("Drawing Layer Components", () => {
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("moves a freehand line", () => {
line.setPosition(5, 5);
line.setDragPosition(5, 5);
line.repositionObject();
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("deletes a freehand line", () => {
Expand All @@ -80,7 +81,8 @@ describe("Drawing Layer Components", () => {
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("moves a vector line", () => {
vector.setPosition(5,5);
vector.setDragPosition(5,5);
vector.repositionObject();
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("deletes a vector line", () => {
Expand Down Expand Up @@ -108,7 +110,8 @@ describe("Drawing Layer Components", () => {
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("moves a vector arrow", () => {
vector.setPosition(5,5);
vector.setDragPosition(5,5);
vector.repositionObject();
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("changes vector to single arrow", () => {
Expand Down Expand Up @@ -143,7 +146,8 @@ describe("Drawing Layer Components", () => {
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("moves a rectangle", () => {
rect.setPosition(5,5);
rect.setDragPosition(5,5);
rect.repositionObject();
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("deletes a rectangle", () => {
Expand All @@ -170,7 +174,8 @@ describe("Drawing Layer Components", () => {
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("moves a ellipse", () => {
ellipse.setPosition(5,5);
ellipse.setDragPosition(5,5);
ellipse.repositionObject();
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("deletes a ellipse", () => {
Expand Down Expand Up @@ -257,7 +262,8 @@ describe("Drawing Layer Components", () => {
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("moves a image", () => {
image.setPosition(5,5);
image.setDragPosition(5,5);
image.repositionObject();
expect(getDrawingObject(content)).toMatchSnapshot();
});
it("deletes a image", () => {
Expand Down
60 changes: 14 additions & 46 deletions src/plugins/drawing/model/drawing-content.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,9 @@ describe("DrawingContentModel", () => {
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "deleteObjects", change: { args: [ [] ], path: ""}, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(4,
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "setSelectedIds", change: { args: [ ["a", "b"] ], path: ""}, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(5,
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "deleteObjects", change: { args: [ ["a", "b"] ], path: ""}, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenCalledTimes(5);
expect(mockLogTileChangeEvent).toHaveBeenCalledTimes(4);
});

it("can update the properties of a set of selected drawing objects", () => {
Expand Down Expand Up @@ -277,45 +274,39 @@ describe("DrawingContentModel", () => {
expect(rect2.strokeWidth).toBe(2);

expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(1,
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "setSelectedIds", change: { args: [["a", "b"]], path: "" }, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(2,
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "setStroke", change: { args: ["#000000", ["a", "b"]], path: "" }, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(3,
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(2,
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "setStrokeWidth", change: { args: [2, ["a", "b"]], path: "" }, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(4,
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(3,
LogEventName.DRAWING_TOOL_CHANGE,
{ operation: "setStrokeDashArray", change: { args: ["3,3", ["a", "b"]], path: "" }, tileId: "drawing-1" });
expect(mockLogTileChangeEvent).toHaveBeenCalledTimes(4);
});
expect(mockLogTileChangeEvent).toHaveBeenCalledTimes(3);
});

it("can move objects", () => {
const model = createDrawingContentWithMetadata();

const rectSnapshot1: RectangleObjectSnapshotForAdd = {...baseRectangleSnapshot, id:"a", x:0, y:0};
model.addObject(rectSnapshot1);

const rectSnapshot2: RectangleObjectSnapshotForAdd = {...baseRectangleSnapshot, id:"b", x:10, y:10};
model.addObject(rectSnapshot2);
const rect1 = model.addObject(rectSnapshot1);

mockLogTileChangeEvent.mockReset();
model.moveObjects([
{id: "a", destination: {x: 20, y: 20}},
{id: "b", destination: {x: 30, y: 30}}
]);
rect1.setDragPosition(20, 20);
rect1.repositionObject();
expect(mockLogTileChangeEvent).toHaveBeenNthCalledWith(1,
LogEventName.DRAWING_TOOL_CHANGE, {
operation: "moveObjects",
operation: "repositionObject",
change: {
args: [[{id: "a", destination: {x: 20, y: 20}}, {id: "b", destination: {x: 30, y: 30}}]],
path: ""
args: [],
path: "/objects/0"
},
tileId: "drawing-1"
});
expect(mockLogTileChangeEvent).toHaveBeenCalledTimes(1);
});
expect(rect1.x).toBe(20);
expect(rect1.y).toBe(20);
});

it("can resize rectangle", () => {
mockLogTileChangeEvent.mockClear();
Expand Down Expand Up @@ -625,29 +616,6 @@ describe("DrawingContentModel", () => {
expect(model.currentStamp!.url).toBe("b.png");
});

it("can update image urls", () => {
const originalUrl = "my/image/url";
const image = ImageObject.create({
url: originalUrl, x: 0, y: 0, width: 10, height: 10
});
const model = createDrawingContentWithMetadata({
objects: [image]
});

model.updateImageUrl("", "");
expect(image.url).toEqual(originalUrl);

// Updates to a empty string are ignored
model.updateImageUrl("my/image/url", "");
expect(image.url).toEqual(originalUrl);

model.updateImageUrl("", "my/image/newUrl");
expect(image.url).toEqual(originalUrl);

model.updateImageUrl("my/image/url", "my/image/newUrl");
expect(image.url).toBe("my/image/newUrl");
});

test("addObject throws when an instance is passed to it", () => {
const model = createDrawingContentWithMetadata();
const rect = RectangleObject.create(baseRectangleSnapshot);
Expand Down
26 changes: 4 additions & 22 deletions src/plugins/drawing/model/drawing-content.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { StampModel, StampModelType } from "./stamp";
import { DrawingObjectMSTUnion } from "../components/drawing-object-manager";
import { DrawingObjectSnapshotForAdd, DrawingObjectType,
ObjectMap, ToolbarModalButton } from "../objects/drawing-object";
import { ImageObjectType, isImageObjectSnapshot } from "../objects/image";
import { isImageObjectSnapshot } from "../objects/image";
import { LogEventName } from "../../../lib/logger-types";
import { logTileChangeEvent } from "../../../models/tiles/log/log-tile-change-event";
import { ITileExportOptions, IDefaultContentOptions } from "../../../models/tiles/tile-content-info";
Expand Down Expand Up @@ -159,8 +159,9 @@ export const DrawingContentModel = NavigatableTileModel
const tileId = self.metadata?.id ?? "";
const {name: operation, ...change} = call;
// Ignore actions that don't need to be logged
const ignoredActions = ["setDisabledFeatures", "setDragPosition", "setDragBounds",
"setSelectedButton", "afterAttach"];
const ignoredActions = ["afterAttach", "afterCreate", "reset",
"setDisabledFeatures", "setDragPosition", "setDragBounds",
"setSelectedButton", "setSelectedIds", "setOpenPalette", "setEditing"];
if (ignoredActions.includes(operation)) return;

logTileChangeEvent(LogEventName.DRAWING_TOOL_CHANGE, { operation, change, tileId });
Expand Down Expand Up @@ -376,25 +377,6 @@ export const DrawingContentModel = NavigatableTileModel
self.setSelectedIds(newIds);
},

moveObjects(moves: DrawingObjectMove[]) {
moves.forEach(move => {
const object = self.objectMap[move.id];
object?.setPosition(move.destination.x, move.destination.y);
});
},

updateImageUrl(oldUrl: string, newUrl: string) {
if (!oldUrl || !newUrl || (oldUrl === newUrl)) return;
// Modify all images with this url
self.objects.forEach(object => {
if (object.type !== "image") return;
const image = object as ImageObjectType;
if (image.url === oldUrl) {
image.setUrl(newUrl);
}
});
},

createGroup(objectIds: string[]) {
const props: GroupObjectSnapshotForAdd = {
type: "group",
Expand Down

0 comments on commit 8c24196

Please sign in to comment.