Skip to content

Conversation

@haydar-metin
Copy link
Contributor

What it does

Works on eclipse-glsp/glsp#1384.

This PR is the initial contribution to provide visual feedback that a container/parent wraps a child even while the child is currently moving. In subsequent PR's those containers will be extended and changing the parent itself of children will be implemented.

How to test

Move single/multiple selected child elements around or resize them (also with the keyboard). The parent will wrap it.

Peek.2025-10-02.11-22.mp4

I will revert the changes to the example file before merging.

Follow-ups

Changelog

  • This PR should be mentioned in the changelog

  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

  • tracker variables in the change-bounds files have been renamed to moveTracker and resizeTracker

  • ChangeBoundsTracker#startTracking requires the root node as argument

@haydar-metin haydar-metin self-assigned this Oct 2, 2025
@haydar-metin haydar-metin marked this pull request as draft October 31, 2025 15:13
@haydar-metin haydar-metin marked this pull request as ready for review November 27, 2025 10:52
@haydar-metin
Copy link
Contributor Author

@martin-fleck-at @tortmayr this PR is now ready to review. Just one question, how should we handle invalid arrow key movements? For example, if you use the arrow keys then you can move the elements to overlapping states (on main).

@martin-fleck-at
Copy link
Contributor

@haydar-metin Thank you for the great work! I think overall the mode of moving (mouse, keyboard, align command etc.) should not dictate the overall move/validation/restriction logic, i.e., they all should be treated the same. So my take on this is that moves with the keyboard should also be treated as invalid if the movement restrictor says so.

@tortmayr tortmayr requested a review from Copilot December 11, 2025 09:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces visual feedback for containers that wrap their children during move and resize operations. The implementation tracks initial bounds, detects affected ancestors, and applies wrapping behavior to resize containers as their children are moved or resized.

Key Changes

  • Added ancestor tracking and wrapping logic to ChangeBoundsTracker to automatically resize parent containers when children move or resize
  • Introduced InitialBoundsTracker to store element bounds before operations begin, enabling proper calculation of affected ancestors
  • Modified ChangeBoundsTracker#startTracking to require the root node as an argument (breaking change)

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/client/src/utils/gmodel-util.ts Added utility functions for ancestor/descendant traversal, bounds change detection, and ancestor ordering
packages/client/src/features/tools/edge-edit/edge-edit-tool-feedback.ts Updated to pass root node to tracker and disable wrapping for routing handles
packages/client/src/features/tools/change-bounds/change-bounds-tracker.ts Implemented wrapping logic, initial bounds tracking, and breaking changes to tracker API
packages/client/src/features/tools/change-bounds/change-bounds-tool.ts Refactored to use initial bounds tracker and support ancestor wrapping during resize operations
packages/client/src/features/tools/change-bounds/change-bounds-tool-move-feedback.ts Added wrap feedback actions and element wrap reset logic for move operations
packages/client/src/features/tools/change-bounds/change-bounds-manager.ts Extended to support wrap resizes and added options for size validation
packages/client/src/features/layout/layout-elements-action.ts Moved isBoundsAwareMoveable import from model to gmodel-util
packages/client/src/features/element-template/mouse-tracking-element-position-listener.ts Updated tracker initialization and disabled wrapping for ghost elements
packages/client/src/features/change-bounds/move-element-handler.ts Integrated wrapping logic for keyboard-based element movement
packages/client/src/features/change-bounds/model.ts Removed isBoundsAwareMoveable (moved to gmodel-util)
Comments suppressed due to low confidence (1)

packages/client/src/utils/gmodel-util.ts:1

  • The width should be calculated as maxX - minX and height as maxY - minY. Currently storing absolute coordinates instead of dimensions.
/********************************************************************************

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

this.moveInitialized();
this.pendingMoveInitialized = undefined;
}, 750);
}, this.moveInitializationTimeout());
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The method moveInitializationTimeout() is called but not defined in the class. This will cause a runtime error unless the method is defined elsewhere or inherited from a parent class.

Copilot uses AI. Check for mistakes.
Comment on lines +515 to +519
let descendants = getDescendantIds(potentialAncestor);
// Remove self id to avoid that an element is considered a descendant of itself
descendants = descendants.filter(id => !id.startsWith(potentialAncestor.id));

return descendants.includes(element.id);
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The variable 'descendants' is misleading because it includes the potential ancestor itself until filtered. Consider renaming to 'descendantIds' for consistency with the function name.

Suggested change
let descendants = getDescendantIds(potentialAncestor);
// Remove self id to avoid that an element is considered a descendant of itself
descendants = descendants.filter(id => !id.startsWith(potentialAncestor.id));
return descendants.includes(element.id);
let descendantIds = getDescendantIds(potentialAncestor);
// Remove self id to avoid that an element is considered a descendant of itself
descendantIds = descendantIds.filter(id => !id.startsWith(potentialAncestor.id));
return descendantIds.includes(element.id);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants