From 50abc2b410ee3dba6edc50326037f368d126c2cf Mon Sep 17 00:00:00 2001 From: Lukas Lieb Date: Wed, 20 Nov 2024 13:34:14 +0100 Subject: [PATCH 1/3] GH-1423 Add MovementBehavior to control invalid multi element movements --- .../change-bounds-tool-move-feedback.ts | 16 ++++++++++++---- .../tools/change-bounds/change-bounds-tool.ts | 11 ++++++++++- packages/glsp-sprotty/src/types.ts | 1 + 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts b/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts index c9467411..32440747 100644 --- a/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts +++ b/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts @@ -185,10 +185,18 @@ export class FeedbackMoveMouseListener extends DragAwareMouseListener { if (!this.tracker.isTracking()) { return []; } - // only reset the move of invalid elements, the others will be handled by the change bounds tool itself - this.getElementsToMove(target) - .filter(element => this.tool.changeBoundsManager.isValid(element)) - .forEach(element => this.elementId2startPos.delete(element.id)); + const elementToMove = this.getElementsToMove(target); + if (!this.tool.movementBehavior.allElementsNeedToBeValid) { + // only reset the move of invalid elements, the others will be handled by the change bounds tool itself + elementToMove + .filter(element => this.tool.changeBoundsManager.isValid(element)) + .forEach(element => this.elementId2startPos.delete(element.id)); + } else { + if (elementToMove.find(element => !this.tool.changeBoundsManager.isValid(element)) === undefined) { + // do not reset any element as all are valid + this.elementId2startPos.clear(); + } + } this.dispose(); return []; } diff --git a/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts b/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts index 4237e667..4f00497b 100644 --- a/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts +++ b/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts @@ -63,6 +63,10 @@ import { import { FeedbackMoveMouseListener } from './change-bounds-tool-move-feedback'; import { ChangeBoundsTracker, TrackedElementResize, TrackedResize } from './change-bounds-tracker'; +export interface IMovementBehavior { + readonly allElementsNeedToBeValid: boolean; +} + /** * The change bounds tool has the license to move multiple elements or resize a single element by implementing the ChangeBounds operation. * In contrast to Sprotty's implementation this tool only sends a `ChangeBoundsOperationAction` when an operation has finished and does not @@ -84,6 +88,7 @@ export class ChangeBoundsTool extends BaseEditTool { @inject(EdgeRouterRegistry) @optional() readonly edgeRouterRegistry?: EdgeRouterRegistry; @inject(TYPES.IMovementRestrictor) @optional() readonly movementRestrictor?: IMovementRestrictor; @inject(TYPES.IChangeBoundsManager) readonly changeBoundsManager: IChangeBoundsManager; + @inject(TYPES.IMovementBehavior) @optional() readonly movementBehavior: IMovementBehavior = { allElementsNeedToBeValid: true }; get id(): string { return ChangeBoundsTool.ID; @@ -260,7 +265,11 @@ export class ChangeBoundsListener extends DragAwareMouseListener implements ISel protected getElementsToMove(target: GModelElement): SelectableBoundsAware[] { const selectedElements = getMatchingElements(target.index, isNonRoutableSelectedMovableBoundsAware); const selectionSet: Set = new Set(selectedElements); - return selectedElements.filter(element => this.isValidMove(element, selectionSet)); + const elementsToMove = selectedElements.filter(element => this.isValidMove(element, selectionSet)); + if (this.tool.movementBehavior.allElementsNeedToBeValid && elementsToMove.length !== selectionSet.size) { + return []; + } + return elementsToMove; } protected handleMoveElementsOnServer(elementsToMove: SelectableBoundsAware[]): Operation[] { diff --git a/packages/glsp-sprotty/src/types.ts b/packages/glsp-sprotty/src/types.ts index 2054a8a3..0de6d576 100644 --- a/packages/glsp-sprotty/src/types.ts +++ b/packages/glsp-sprotty/src/types.ts @@ -30,6 +30,7 @@ export const TYPES = { IToolFactory: Symbol('Factory'), ITypeHintProvider: Symbol('ITypeHintProvider'), IMovementRestrictor: Symbol('IMovementRestrictor'), + IMovementBehavior: Symbol('IMovementBehavior'), ISelectionListener: Symbol('ISelectionListener'), /** @deprecated Use {@link TYPES.IGModelRootListener} instead */ // eslint-disable-next-line deprecation/deprecation From fd6cc8ba1464ba9c93434185f732f844563a2365 Mon Sep 17 00:00:00 2001 From: Lukas Lieb Date: Thu, 21 Nov 2024 16:08:59 +0100 Subject: [PATCH 2/3] Review: - Rename to IMovementOptions - Add JsDoc to `allElementsNeedToBeValid` option --- .../tools/change-bounds/change-bounds-tool-move-feedback.ts | 2 +- .../src/features/tools/change-bounds/change-bounds-tool.ts | 5 +++-- packages/glsp-sprotty/src/types.ts | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts b/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts index 32440747..a52d5a2b 100644 --- a/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts +++ b/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts @@ -192,7 +192,7 @@ export class FeedbackMoveMouseListener extends DragAwareMouseListener { .filter(element => this.tool.changeBoundsManager.isValid(element)) .forEach(element => this.elementId2startPos.delete(element.id)); } else { - if (elementToMove.find(element => !this.tool.changeBoundsManager.isValid(element)) === undefined) { + if (elementToMove.every(element => this.tool.changeBoundsManager.isValid(element))) { // do not reset any element as all are valid this.elementId2startPos.clear(); } diff --git a/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts b/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts index 4f00497b..aaaaf8cc 100644 --- a/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts +++ b/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts @@ -63,7 +63,8 @@ import { import { FeedbackMoveMouseListener } from './change-bounds-tool-move-feedback'; import { ChangeBoundsTracker, TrackedElementResize, TrackedResize } from './change-bounds-tracker'; -export interface IMovementBehavior { +export interface IMovementOptions { + /** If set to true, a move with multiple elements is only performed if each individual move is valid. */ readonly allElementsNeedToBeValid: boolean; } @@ -88,7 +89,7 @@ export class ChangeBoundsTool extends BaseEditTool { @inject(EdgeRouterRegistry) @optional() readonly edgeRouterRegistry?: EdgeRouterRegistry; @inject(TYPES.IMovementRestrictor) @optional() readonly movementRestrictor?: IMovementRestrictor; @inject(TYPES.IChangeBoundsManager) readonly changeBoundsManager: IChangeBoundsManager; - @inject(TYPES.IMovementBehavior) @optional() readonly movementBehavior: IMovementBehavior = { allElementsNeedToBeValid: true }; + @inject(TYPES.IMovementOptions) @optional() readonly movementBehavior: IMovementOptions = { allElementsNeedToBeValid: true }; get id(): string { return ChangeBoundsTool.ID; diff --git a/packages/glsp-sprotty/src/types.ts b/packages/glsp-sprotty/src/types.ts index 0de6d576..27ac94cc 100644 --- a/packages/glsp-sprotty/src/types.ts +++ b/packages/glsp-sprotty/src/types.ts @@ -30,7 +30,7 @@ export const TYPES = { IToolFactory: Symbol('Factory'), ITypeHintProvider: Symbol('ITypeHintProvider'), IMovementRestrictor: Symbol('IMovementRestrictor'), - IMovementBehavior: Symbol('IMovementBehavior'), + IMovementOptions: Symbol('IMovementOptions'), ISelectionListener: Symbol('ISelectionListener'), /** @deprecated Use {@link TYPES.IGModelRootListener} instead */ // eslint-disable-next-line deprecation/deprecation From 522c336e6fee6a8ef3913ae4c2d88ed1b2c3e437 Mon Sep 17 00:00:00 2001 From: Lukas Lieb Date: Fri, 22 Nov 2024 08:43:11 +0100 Subject: [PATCH 3/3] Review: - Variable renamings --- .../change-bounds/change-bounds-tool-move-feedback.ts | 8 ++++---- .../features/tools/change-bounds/change-bounds-tool.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts b/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts index a52d5a2b..68433595 100644 --- a/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts +++ b/packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts @@ -185,14 +185,14 @@ export class FeedbackMoveMouseListener extends DragAwareMouseListener { if (!this.tracker.isTracking()) { return []; } - const elementToMove = this.getElementsToMove(target); - if (!this.tool.movementBehavior.allElementsNeedToBeValid) { + const elementsToMove = this.getElementsToMove(target); + if (!this.tool.movementOptions.allElementsNeedToBeValid) { // only reset the move of invalid elements, the others will be handled by the change bounds tool itself - elementToMove + elementsToMove .filter(element => this.tool.changeBoundsManager.isValid(element)) .forEach(element => this.elementId2startPos.delete(element.id)); } else { - if (elementToMove.every(element => this.tool.changeBoundsManager.isValid(element))) { + if (elementsToMove.every(element => this.tool.changeBoundsManager.isValid(element))) { // do not reset any element as all are valid this.elementId2startPos.clear(); } diff --git a/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts b/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts index aaaaf8cc..cafebcb0 100644 --- a/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts +++ b/packages/client/src/features/tools/change-bounds/change-bounds-tool.ts @@ -89,7 +89,7 @@ export class ChangeBoundsTool extends BaseEditTool { @inject(EdgeRouterRegistry) @optional() readonly edgeRouterRegistry?: EdgeRouterRegistry; @inject(TYPES.IMovementRestrictor) @optional() readonly movementRestrictor?: IMovementRestrictor; @inject(TYPES.IChangeBoundsManager) readonly changeBoundsManager: IChangeBoundsManager; - @inject(TYPES.IMovementOptions) @optional() readonly movementBehavior: IMovementOptions = { allElementsNeedToBeValid: true }; + @inject(TYPES.IMovementOptions) @optional() readonly movementOptions: IMovementOptions = { allElementsNeedToBeValid: true }; get id(): string { return ChangeBoundsTool.ID; @@ -267,7 +267,7 @@ export class ChangeBoundsListener extends DragAwareMouseListener implements ISel const selectedElements = getMatchingElements(target.index, isNonRoutableSelectedMovableBoundsAware); const selectionSet: Set = new Set(selectedElements); const elementsToMove = selectedElements.filter(element => this.isValidMove(element, selectionSet)); - if (this.tool.movementBehavior.allElementsNeedToBeValid && elementsToMove.length !== selectionSet.size) { + if (this.tool.movementOptions.allElementsNeedToBeValid && elementsToMove.length !== selectionSet.size) { return []; } return elementsToMove;