Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(react-components) Unit test for UndoManager #5028

Merged
merged 7 commits into from
Mar 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,6 @@ export class RevealRenderTarget {
options?: RevealRenderTargetOptions
) {
this._viewer = viewer;

const cameraManager = this.cameraManager;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed and will give problems in the unit test

if (!isFlexibleCameraManager(cameraManager)) {
throw new Error('Can not use RevealRenderTarget without the FlexibleCameraManager');
}

const coreDmOnly = options?.coreDmOnly ?? false;
this._cdfCaches = new CdfCaches(sdk, viewer, { coreDmOnly });
this._commandsController = new CommandsController(this.domElement);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { type RevealRenderTarget } from '../renderTarget/RevealRenderTarget';
/**
* Represents an generic transaction used for undo.
* It implements undo transaction for added, deleted, name, color, geometry
* and renderstyle changes.
* and render style changes.
*/
export class DomainObjectTransaction extends Transaction {
// ==================================================
Expand Down
14 changes: 8 additions & 6 deletions react-components/src/architecture/base/undo/Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ export abstract class Transaction {
// INSTANCE METHODS
// ==================================================

public readonly uniqueId: number;
public readonly changed: symbol;
protected readonly parentUniqueId: number;
public readonly uniqueId: number;
private readonly _parentUniqueId?: number;
public readonly timeStamp = Date.now();

// ==================================================
Expand All @@ -25,10 +25,9 @@ export abstract class Transaction {
this.changed = changed;

const parent = domainObject.parent;
if (parent === undefined) {
throw new Error('Parent is undefined');
if (parent !== undefined) {
this._parentUniqueId = parent.uniqueId;
}
this.parentUniqueId = parent.uniqueId;
}

// ==================================================
Expand All @@ -46,7 +45,10 @@ export abstract class Transaction {

public undo(renderTarget: RevealRenderTarget): boolean {
const root = renderTarget.rootDomainObject;
const uniqueId = this.changed === Changes.deleted ? this.parentUniqueId : this.uniqueId;
const uniqueId = this.changed === Changes.deleted ? this._parentUniqueId : this.uniqueId;
if (uniqueId === undefined) {
return false;
}
const domainObject = root.getThisOrDescendantByUniqueId(uniqueId);
if (domainObject === undefined) {
return false;
Expand Down
123 changes: 123 additions & 0 deletions react-components/src/architecture/base/undo/UndoManager.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/*!
* Copyright 2024 Cognite AS
*/

import { beforeEach, describe, expect, test } from 'vitest';
import { Changes } from '../domainObjectsHelpers/Changes';
import { UndoManager } from './UndoManager';
import { MeasureBoxDomainObject } from '../../concrete/measurements/MeasureBoxDomainObject';
import { PrimitiveType } from '../utilities/primitives/PrimitiveType';
import { expectEqualVector3 } from '../../../../tests/tests-utilities/primitives/primitiveTestUtil';
import { type BoxDomainObject } from '../../concrete/primitives/box/BoxDomainObject';
import { RevealRenderTarget } from '../renderTarget/RevealRenderTarget';
import { type RootDomainObject } from '../domainObjects/RootDomainObject';
import { Box } from '../utilities/primitives/Box';
import { viewerMock } from '../../../../tests/tests-utilities/fixtures/viewer';
import { sdkMock } from '../../../../tests/tests-utilities/fixtures/sdk';

describe('UndoManager', () => {
let renderTarget: RevealRenderTarget;
let root: RootDomainObject;
let manager: UndoManager;
beforeEach(() => {
renderTarget = createRenderTargetMock();
root = renderTarget.rootDomainObject;
manager = new UndoManager();
});

test('Should updated the state of the UndoManager', () => {
const domainObject = createBoxDomainObject();
root.addChildInteractive(domainObject);

// Change name
const transaction = domainObject.createTransaction(Changes.active); // Just fake a change
expect(manager.canUndo).toBe(false);
expect(manager.addTransaction(transaction)).toBe(true);

// Undo
expect(manager.hasUniqueId(domainObject.uniqueId)).toBe(true);
expect(manager.canUndo).toBe(true);
manager.undo(renderTarget);
expect(manager.hasUniqueId(domainObject.uniqueId)).toBe(false);
expect(manager.canUndo).toBe(false);
});

test('Should undo geometry and name change', () => {
const domainObject = createBoxDomainObject();
root.addChildInteractive(domainObject);
const oldBox = new Box().copy(domainObject.box);
const oldName = domainObject.name;

{
// Change some geometry
const transaction = domainObject.createTransaction(Changes.geometry);
manager.addTransaction(transaction);
domainObject.box.size.multiplyScalar(3);
domainObject.box.center.addScalar(3);
domainObject.notify(Changes.geometry);
expect(domainObject.box.size).not.toStrictEqual(oldBox.size);
expect(domainObject.box.center).not.toStrictEqual(oldBox.center);
}
{
// Change name
const transaction = domainObject.createTransaction(Changes.naming);
expect(manager.addTransaction(transaction)).toBe(false);
domainObject.name = 'new name';
domainObject.notify(Changes.naming);
expect(domainObject.name).not.toBe(oldName);
}
// Undo
manager.undo(renderTarget);
expectEqualVector3(domainObject.box.size, oldBox.size);
expectEqualVector3(domainObject.box.center, oldBox.center);
expect(domainObject.name).toBe(oldName);
expect(manager.canUndo).toBe(false);
});

test('Should undo add', () => {
// Add the new domainObject
const domainObject = createBoxDomainObject();
const transaction = domainObject.createTransaction(Changes.added);
expect(manager.addTransaction(transaction)).toBe(true);
root.addChildInteractive(domainObject);
expect(root.getThisOrDescendantByUniqueId(domainObject.uniqueId)).toBeDefined();
expect(domainObject.hasParent).toBe(true);

// Undo
manager.undo(renderTarget);
expect(root.getThisOrDescendantByUniqueId(domainObject.uniqueId)).toBeUndefined();
expect(domainObject.hasParent).toBe(false);
expect(manager.canUndo).toBe(false);
});

test('Should undo deleted', () => {
const domainObject = createBoxDomainObject();
root.addChildInteractive(domainObject);
const uniqueId = domainObject.uniqueId;

// Delete the domain object
const transaction = domainObject.createTransaction(Changes.deleted);
expect(manager.addTransaction(transaction)).toBe(true);
domainObject.removeInteractive();
expect(root.getThisOrDescendantByUniqueId(uniqueId)).toBeUndefined();

manager.undo(renderTarget);
const restoredDomainObject = root.getThisOrDescendantByUniqueId(uniqueId);
expect(restoredDomainObject).toBeDefined();
expect(restoredDomainObject).not.toBe(domainObject);
expect(manager.canUndo).toBe(false);
});
});

function createBoxDomainObject(): BoxDomainObject {
const domainObject = new MeasureBoxDomainObject(PrimitiveType.Box);
const { box } = domainObject;
box.size.set(1, 2, 3);
box.center.set(4, 5, 6);
return domainObject;
}

function createRenderTargetMock(): RevealRenderTarget {
const renderTarget = new RevealRenderTarget(viewerMock, sdkMock);
return renderTarget;
}
12 changes: 10 additions & 2 deletions react-components/src/architecture/base/undo/UndoManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,19 @@ export class UndoManager {
// INSTANCE METHODS
// =================================================

public addTransaction(transaction: Transaction | undefined): void {
/**
* Adds a transaction to the undo manager.
*
* @param transaction - The transaction to be added. If undefined, the transaction will not be added.
* @returns A boolean indicating whether the ability to undo has changed as a result of adding the transaction.
*/
public addTransaction(transaction: Transaction | undefined): boolean {
if (transaction === undefined) {
return;
return false;
}
const couldUndo = this.canUndo;
this._transactions.push(transaction);
return couldUndo !== this.canUndo;
}

public undo(renderTarget: RevealRenderTarget): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ import { type BaseDragger } from '../../base/domainObjectsHelpers/BaseDragger';
import { ExampleDragger } from './ExampleDragger';
import { Quantity } from '../../base/domainObjectsHelpers/Quantity';
import { type TranslationInput } from '../../base/utilities/TranslateInput';
import { type Transaction } from '../../base/undo/Transaction';
import { type DomainObject } from '../../base/domainObjects/DomainObject';
import { Changes } from '../../base/domainObjectsHelpers/Changes';
import { DomainObjectTransaction } from '../../base/undo/DomainObjectTransaction';
import { type IconName } from '../../base/utilities/IconName';
import { type RevealRenderTarget } from '../../base/renderTarget/RevealRenderTarget';
import { type Transaction } from '../../base/undo/Transaction';
import { DomainObjectTransaction } from '../../base/undo/DomainObjectTransaction';

export class ExampleDomainObject extends VisualDomainObject {
// ==================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@ import { type PrimitivePickInfo } from '../common/PrimitivePickInfo';
import { type BaseDragger } from '../../../base/domainObjectsHelpers/BaseDragger';
import { BoxDragger } from './BoxDragger';
import { type CreateDraggerProps } from '../../../base/domainObjects/VisualDomainObject';
import { getIconByPrimitiveType } from '../../../base/utilities/primitives/getIconByPrimitiveType';
import { type TranslationInput } from '../../../base/utilities/TranslateInput';
import { Quantity } from '../../../base/domainObjectsHelpers/Quantity';
import { PanelInfo } from '../../../base/domainObjectsHelpers/PanelInfo';
import { type IconName } from '../../../base/utilities/IconName';
import { SolidDomainObject } from '../common/SolidDomainObject';
import { SolidPrimitiveRenderStyle } from '../common/SolidPrimitiveRenderStyle';
import { Box } from '../../../base/utilities/primitives/Box';
Expand Down Expand Up @@ -42,10 +40,6 @@ export abstract class BoxDomainObject extends SolidDomainObject {
// OVERRIDES of DomainObject
// ==================================================

public override get icon(): IconName {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, implementation is already in the base class

return getIconByPrimitiveType(this.primitiveType);
}

public override get typeName(): TranslationInput {
switch (this.primitiveType) {
case PrimitiveType.HorizontalArea:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import { FocusType } from '../../../base/domainObjectsHelpers/FocusType';
import { type PrimitiveType } from '../../../base/utilities/primitives/PrimitiveType';
import { VisualDomainObject } from '../../../base/domainObjects/VisualDomainObject';
import { getIconByPrimitiveType } from '../../../base/utilities/primitives/getIconByPrimitiveType';
import { type IconName } from '../../../base/utilities/IconName';
import { DomainObjectTransaction } from '../../../base/undo/DomainObjectTransaction';
import { type Transaction } from '../../../base/undo/Transaction';
import { type IconName } from '../../../base/utilities/IconName';

export abstract class SolidDomainObject extends VisualDomainObject {
// For focus when edit in 3D (Used when isSelected is true only)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { type PrimitivePickInfo } from '../common/PrimitivePickInfo';
import { type BaseDragger } from '../../../base/domainObjectsHelpers/BaseDragger';
import { CylinderDragger } from './CylinderDragger';
import { type CreateDraggerProps } from '../../../base/domainObjects/VisualDomainObject';
import { getIconByPrimitiveType } from '../../../base/utilities/primitives/getIconByPrimitiveType';
import { type TranslationInput } from '../../../base/utilities/TranslateInput';
import { Quantity } from '../../../base/domainObjectsHelpers/Quantity';
import { PanelInfo } from '../../../base/domainObjectsHelpers/PanelInfo';
Expand All @@ -19,7 +18,6 @@ import { SolidPrimitiveRenderStyle } from '../common/SolidPrimitiveRenderStyle';
import { Cylinder } from '../../../base/utilities/primitives/Cylinder';
import { Line3, Vector3 } from 'three';
import { type RevealRenderTarget } from '../../../base/renderTarget/RevealRenderTarget';
import { type IconName } from '../../../base/utilities/IconName';

export abstract class CylinderDomainObject extends SolidDomainObject {
// ==================================================
Expand All @@ -32,10 +30,6 @@ export abstract class CylinderDomainObject extends SolidDomainObject {
// OVERRIDES of DomainObject
// ==================================================

public override get icon(): IconName {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, implementation is already in the base class

return getIconByPrimitiveType(this.primitiveType);
}

public override get typeName(): TranslationInput {
return { key: 'CYLINDER' };
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ import { VisualDomainObject } from '../../../base/domainObjects/VisualDomainObje
import { getIconByPrimitiveType } from '../../../base/utilities/primitives/getIconByPrimitiveType';
import { type TranslationInput } from '../../../base/utilities/TranslateInput';
import { clear } from '../../../base/utilities/extensions/arrayExtensions';
import { type Transaction } from '../../../base/undo/Transaction';
import { DomainObjectTransaction } from '../../../base/undo/DomainObjectTransaction';
import { type IconName } from '../../../base/utilities/IconName';
import { Vector3ArrayUtils } from '../../../base/utilities/primitives/PointsUtils';
import { DomainObjectTransaction } from '../../../base/undo/DomainObjectTransaction';
import { type Transaction } from '../../../base/undo/Transaction';

export abstract class LineDomainObject extends VisualDomainObject {
// ==================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ import { PanelInfo } from '../../../base/domainObjectsHelpers/PanelInfo';
import { Quantity } from '../../../base/domainObjectsHelpers/Quantity';
import { radToDeg } from 'three/src/math/MathUtils.js';
import { type DomainObjectChange } from '../../../base/domainObjectsHelpers/DomainObjectChange';
import { DomainObjectTransaction } from '../../../base/undo/DomainObjectTransaction';
import { type Transaction } from '../../../base/undo/Transaction';
import { type IconName } from '../../../base/utilities/IconName';
import { SolidPrimitiveRenderStyle } from '../common/SolidPrimitiveRenderStyle';
import { type RevealRenderTarget } from '../../../base/renderTarget/RevealRenderTarget';
import { getRoot } from '../../../base/domainObjects/getRoot';
import { DomainObjectTransaction } from '../../../base/undo/DomainObjectTransaction';
import { type Transaction } from '../../../base/undo/Transaction';

const ORIGIN = new Vector3(0, 0, 0);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { type CameraManager, type CameraManagerEventType, type CameraState } from '@cognite/reveal';
import { remove } from 'lodash';
import { Mock } from 'moq.ts';
import { PerspectiveCamera } from 'three';

import { vi, type Mock as viMock } from 'vitest';

Expand Down Expand Up @@ -36,6 +37,8 @@ export const cameraManagerMock = new Mock<CameraManager>()
})
.setup((p) => p.getCameraState())
.returns(cameraManagerGlobalCurrentCameraState as Required<CameraState>)
.setup((p) => p.getCamera)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to the ,mock. It is needed.

.returns(() => new PerspectiveCamera())
.setup((p) => p.fitCameraToBoundingBox)
.returns(fitCameraToBoundingBoxMock)
.object();
12 changes: 12 additions & 0 deletions react-components/tests/tests-utilities/fixtures/viewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,16 @@ export const viewerMock = new Mock<Cognite3DViewer<DataSourceType>>()
.returns(fitCameraToModelsMock)
.setup((p) => p.requestRedraw)
.returns(vi.fn())
.setup((p) => p.on)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needed in the unit tests

.returns(vi.fn())
.setup((p) => p.off)
.returns(vi.fn())
.setup((p) => p.addObject3D)
.returns(vi.fn())
.setup((p) => p.removeObject3D)
.returns(vi.fn())
.setup((p) => p.addCustomObject)
.returns(vi.fn())
.setup((p) => p.removeCustomObject)
.returns(vi.fn())
.object();
Loading