diff --git a/packages/core/src/components/shape/shape.tsx b/packages/core/src/components/shape/shape.tsx index 593bf90ed..dfbb463ff 100644 --- a/packages/core/src/components/shape/shape.tsx +++ b/packages/core/src/components/shape/shape.tsx @@ -29,6 +29,7 @@ export const Shape = React.memo( transform={transform} {...events} > + {shape.childIndex} {isEditing && utils.isEditableText ? ( { if (shape.parentId !== page.id) return false diff --git a/packages/tldraw/src/state/command/group/group.command.spec.ts b/packages/tldraw/src/state/command/group/group.command.spec.ts index 470d8952e..36be3f23a 100644 --- a/packages/tldraw/src/state/command/group/group.command.spec.ts +++ b/packages/tldraw/src/state/command/group/group.command.spec.ts @@ -122,6 +122,12 @@ describe('Group command', () => { */ it('creates a new group on the page', () => { + /* + When the selected shapes are the children of another group, and so + long as the children do not represent ALL of the group's children, + then a new group should be created that is a child of the parent group. + */ + tlstate.resetDocument().createShapes( { id: 'rect1', @@ -200,49 +206,144 @@ describe('Group command', () => { expect(tlstate.getShape('newGroupB').children).toStrictEqual(['rect1', 'rect3']) }) - /* - When the selected shapes are the children of another group, and so - long as the children do not represent ALL of the group's children, - then a new group should be created that is a child of the parent group. - */ - - it.todo('does not group shapes if shapes are all the groups children') - /* + it('does not group shapes if shapes are all the groups children', () => { + /* If the selected shapes represent ALL of the children of the a group, then no effect should occur. */ + tlstate.resetDocument().createShapes( + { + id: 'rect1', + type: TLDrawShapeType.Rectangle, + childIndex: 1, + }, + { + id: 'rect2', + type: TLDrawShapeType.Rectangle, + childIndex: 2, + }, + { + id: 'rect3', + type: TLDrawShapeType.Rectangle, + childIndex: 3, + } + ) - it.todo('creates the new group as a child of the parent group') - /* - The new group should be a child of the parent group. - */ - - it('moves the selected layers to the new group', () => { - /* - The new group should have the selected children. The old parents - should no longer have the selected shapes among their children. - All of the selected shapes should be assigned the new parent. - */ + tlstate.group(['rect1', 'rect2', 'rect3'], 'newGroupA') + tlstate.group(['rect1', 'rect2', 'rect3'], 'newGroupB') + expect(tlstate.getShape('newGroupB')).toBeUndefined() }) - it.todo('deletes any groups that no longer have children') - /* - If the selected groups included the children of another group, then - that group should be destroyed. Other rules around deleted - shapes should here apply: bindings connected to the group - should be deleted, etc. + it('deletes any groups that no longer have children', () => { + /* + If the selected groups included the children of another group + in addition to other shapes then that group should be destroyed. + Other rules around deleted shapes should here apply: bindings + connected to the group should be deleted, etc. */ + tlstate.resetDocument().createShapes( + { + id: 'rect1', + type: TLDrawShapeType.Rectangle, + childIndex: 1, + }, + { + id: 'rect2', + type: TLDrawShapeType.Rectangle, + childIndex: 2, + }, + { + id: 'rect3', + type: TLDrawShapeType.Rectangle, + childIndex: 3, + } + ) + + tlstate.group(['rect1', 'rect2'], 'newGroupA') + tlstate.group(['rect1', 'rect2', 'rect3'], 'newGroupB') + expect(tlstate.getShape('newGroupA')).toBeUndefined() + expect(tlstate.getShape('newGroupB').children).toStrictEqual([ + 'rect1', + 'rect2', + 'rect3', + ]) + }) + + it('marges selected groups that no longer have children', () => { + /* + If the user is creating a group while having selected other + groups, then the selected groups should be destroyed and a new + group created with the selected shapes and the group(s)' children. + */ + tlstate.resetDocument().createShapes( + { + id: 'rect1', + type: TLDrawShapeType.Rectangle, + childIndex: 1, + }, + { + id: 'rect2', + type: TLDrawShapeType.Rectangle, + childIndex: 2, + }, + { + id: 'rect3', + type: TLDrawShapeType.Rectangle, + childIndex: 3, + } + ) + + tlstate.group(['rect1', 'rect2'], 'newGroupA') + tlstate.group(['newGroupA', 'rect3'], 'newGroupB') + expect(tlstate.getShape('newGroupA')).toBeUndefined() + expect(tlstate.getShape('newGroupB').children).toStrictEqual([ + 'rect1', + 'rect2', + 'rect3', + ]) + + tlstate.undo() + + expect(tlstate.getShape('newGroupB')).toBeUndefined() + expect(tlstate.getShape('newGroupA')).toBeDefined() + expect(tlstate.getShape('newGroupA').children).toStrictEqual(['rect1', 'rect2']) + + tlstate.redo() + + expect(tlstate.getShape('newGroupA')).toBeUndefined() + expect(tlstate.getShape('newGroupB')).toBeDefined() + expect(tlstate.getShape('newGroupB').children).toStrictEqual([ + 'rect1', + 'rect2', + 'rect3', + ]) + }) - it.todo('preserves the child index order') /* The layers should be in the same order as the original layers as they would have appeared on a layers tree (lowest child index first, parent inclusive). */ + + it.todo('preserves the child index order') + + /* --------------------- Nesting -------------------- */ + + // it.todo('creates the new group as a child of the parent group') + /* + The new group should be a child of the parent group. + */ + + // it.todo('moves the selected layers to the new group') + /* + The new group should have the selected children. The old parents + should no longer have the selected shapes among their children. + All of the selected shapes should be assigned the new parent. + */ }) - describe('when grouping shapes with different parents', () => { - /* + // describe('when grouping shapes with different parents', () => { + /* When two shapes with different parents are grouped, the new parent group should have the same parent as the shape nearest to the top of the layer tree. The new group's child index should be that @@ -291,10 +392,20 @@ describe('Group command', () => { the top. */ - it.todo('creates a group in the correct place') - /* + // it.todo('creates a group in the correct place') + /* The new group should be a child of the nearest shape to the top of the tree. */ - }) + + /* + If the selected groups included the children of another group, then + that group should be destroyed. Other rules around deleted + shapes should here apply: bindings connected to the group + should be deleted, etc. + */ + + // it.todo('deletes any groups that no longer have children') + + // }) }) diff --git a/packages/tldraw/src/state/command/group/group.command.ts b/packages/tldraw/src/state/command/group/group.command.ts index 4b7bedd64..733629c13 100644 --- a/packages/tldraw/src/state/command/group/group.command.ts +++ b/packages/tldraw/src/state/command/group/group.command.ts @@ -17,17 +17,32 @@ export function group( const { currentPageId } = data.appState - const initialShapes = ids.map((id) => TLDR.getShape(data, id, currentPageId)) + const idsToGroup = [...ids] + const shapesToGroup: TLDrawShape[] = [] + const deletedGroupIds: string[] = [] + const otherEffectedGroups: TLDrawShape[] = [] + + // Collect all of the shapes to group (and their ids) + for (const id of ids) { + const shape = TLDR.getShape(data, id, currentPageId) + if (shape.children === undefined) { + shapesToGroup.push(shape) + } else { + otherEffectedGroups.push(shape) + idsToGroup.push(...shape.children) + shapesToGroup.push(...shape.children.map((id) => TLDR.getShape(data, id, currentPageId))) + } + } // 1. Can we create this group? // Do the shapes have the same parent? - if (initialShapes.every((shape) => shape.parentId === initialShapes[0].parentId)) { + if (shapesToGroup.every((shape) => shape.parentId === shapesToGroup[0].parentId)) { // Is the common parent a shape (not the page)? - if (initialShapes[0].parentId !== currentPageId) { - const commonParent = TLDR.getShape(data, initialShapes[0].parentId, currentPageId) + if (shapesToGroup[0].parentId !== currentPageId) { + const commonParent = TLDR.getShape(data, shapesToGroup[0].parentId, currentPageId) // Are all of the common parent's shapes selected? - if (commonParent.children?.length === ids.length) { + if (commonParent.children?.length === idsToGroup.length) { // Don't create a group if that group would be the same as the // existing group. return @@ -40,11 +55,11 @@ export function group( // A map of shapes to their index in flattendShapes const shapeIndexMap = Object.fromEntries( - initialShapes.map((shape) => [shape.id, flattenedShapes.indexOf(shape)]) + shapesToGroup.map((shape) => [shape.id, flattenedShapes.indexOf(shape)]) ) // An array of shapes in order by their index in flattendShapes - const sortedShapes = initialShapes.sort((a, b) => shapeIndexMap[a.id] - shapeIndexMap[b.id]) + const sortedShapes = shapesToGroup.sort((a, b) => shapeIndexMap[a.id] - shapeIndexMap[b.id]) // The parentId is always the current page const groupParentId = currentPageId // sortedShapes[0].parentId @@ -57,7 +72,7 @@ export function group( ).childIndex // The shape's point is the min point of its childrens' common bounds - const groupBounds = Utils.getCommonBounds(initialShapes.map((shape) => TLDR.getBounds(shape))) + const groupBounds = Utils.getCommonBounds(shapesToGroup.map((shape) => TLDR.getBounds(shape))) // Create the group beforeShapes[groupId] = undefined @@ -71,9 +86,6 @@ export function group( children: sortedShapes.map((shape) => shape.id), }) - // Collect parents (other groups) that will have lost children - const otherEffectedGroups: TLDrawShape[] = [] - // Reparent shapes to the new group sortedShapes.forEach((shape, index) => { // If the shape is part of a different group, mark the parent shape for cleanup @@ -95,9 +107,6 @@ export function group( } }) - // These are the ids of deleted groups - const deletedShapeIds: string[] = [] - // Clean up effected parents while (otherEffectedGroups.length > 0) { const shape = otherEffectedGroups.pop() @@ -105,7 +114,7 @@ export function group( // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const nextChildren = (beforeShapes[shape.id]?.children || shape.children)!.filter( - (childId) => childId && !(ids.includes(childId) || deletedShapeIds.includes(childId)) + (childId) => childId && !(idsToGroup.includes(childId) || deletedGroupIds.includes(childId)) ) // If the parent has no children, remove it @@ -114,8 +123,9 @@ export function group( afterShapes[shape.id] = undefined // And if that parent is part of a different group, mark it for cleanup + // (This is necessary only when we implement nested groups.) if (shape.parentId !== currentPageId) { - deletedShapeIds.push(shape.id) + deletedGroupIds.push(shape.id) otherEffectedGroups.push(TLDR.getShape(data, shape.parentId, currentPageId)) } } else { @@ -131,7 +141,7 @@ export function group( } } - // TODO: This code is copied from delete.command, create a shared helper + // TODO: This code is copied from delete.command. Create a shared helper! const page = TLDR.getPage(data, currentPageId) @@ -163,7 +173,7 @@ export function group( // Unless we're currently deleting the shape, remove the // binding reference from the after patch - if (!deletedShapeIds.includes(id)) { + if (!deletedGroupIds.includes(id)) { afterShapes[id] = { ...afterShapes[id], handles: { diff --git a/packages/tldraw/src/state/tlstate.spec.ts b/packages/tldraw/src/state/tlstate.spec.ts index c3d1ecfcc..2d153d97f 100644 --- a/packages/tldraw/src/state/tlstate.spec.ts +++ b/packages/tldraw/src/state/tlstate.spec.ts @@ -75,7 +75,6 @@ describe('TLDrawState', () => { tlstate.loadDocument(mockDocument).deselectAll() tlu.clickShape('rect1') tlu.clickShape('rect2', { shiftKey: true }) - expect(tlstate.selectedIds).toStrictEqual(['rect1', 'rect2']) tlu.clickShape('rect2', { shiftKey: true }) expect(tlstate.selectedIds).toStrictEqual(['rect1']) expect(tlstate.appState.status.current).toBe('idle') @@ -108,13 +107,6 @@ describe('TLDrawState', () => { expect(tlstate.appState.status.current).toBe('idle') }) - it('does not select on meta-shift-click', () => { - tlstate.loadDocument(mockDocument).deselectAll() - tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true }) - expect(tlstate.selectedIds).toStrictEqual([]) - expect(tlstate.appState.status.current).toBe('idle') - }) - it.todo('deletes shapes if cancelled during creating') it.todo('deletes shapes on undo after creating') @@ -232,8 +224,6 @@ describe('TLDrawState', () => { describe('Mutates bound shapes', () => { const tlstate = new TLDrawState() - - tlstate .createShapes( { id: 'rect', @@ -269,4 +259,105 @@ describe('TLDrawState', () => { expect(tlstate.getShape('arrow').style.color).toBe(ColorStyle.Red) expect(tlstate.getShape('rect').style.color).toBe(ColorStyle.Red) }) + + describe('when selecting shapes in a group', () => { + it('selects the group when a grouped shape is clicked', () => { + const tlstate = new TLDrawState() + .loadDocument(mockDocument) + .group(['rect1', 'rect2'], 'groupA') + + const tlu = new TLStateUtils(tlstate) + tlu.clickShape('rect1') + expect(tlstate.selectedGroupId).toBeUndefined() + expect(tlstate.selectedIds).toStrictEqual(['groupA']) + }) + + it('selects the grouped shape when double clicked', () => { + const tlstate = new TLDrawState() + .loadDocument(mockDocument) + .group(['rect1', 'rect2'], 'groupA') + + const tlu = new TLStateUtils(tlstate) + tlu.doubleClickShape('rect1') + expect(tlstate.selectedGroupId).toStrictEqual('groupA') + expect(tlstate.selectedIds).toStrictEqual(['rect1']) + }) + + it('clears the selectedGroupId when selecting a different shape', () => { + const tlstate = new TLDrawState() + .loadDocument(mockDocument) + .group(['rect1', 'rect2'], 'groupA') + + const tlu = new TLStateUtils(tlstate) + tlu.doubleClickShape('rect1') + tlu.clickShape('rect3') + expect(tlstate.selectedGroupId).toBeUndefined() + expect(tlstate.selectedIds).toStrictEqual(['rect3']) + }) + + it('selects a grouped shape when meta-shift-clicked', () => { + const tlstate = new TLDrawState() + .loadDocument(mockDocument) + .group(['rect1', 'rect2'], 'groupA') + .deselectAll() + + const tlu = new TLStateUtils(tlstate) + + tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true }) + expect(tlstate.selectedIds).toStrictEqual(['rect1']) + + tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true }) + expect(tlstate.selectedIds).toStrictEqual([]) + }) + + it('selects a hovered shape from the selected group when meta-shift-clicked', () => { + const tlstate = new TLDrawState() + .loadDocument(mockDocument) + .group(['rect1', 'rect2'], 'groupA') + + const tlu = new TLStateUtils(tlstate) + tlu.hoverShape('rect1') + + tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true }) + expect(tlstate.selectedIds).toStrictEqual(['rect1']) + + tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true }) + expect(tlstate.selectedIds).toStrictEqual([]) + }) + }) + + describe('when creating shapes', () => { + it('Creates shapes with the correct child index', () => { + const tlstate = new TLDrawState() + .createShapes( + { + id: 'rect1', + type: TLDrawShapeType.Rectangle, + childIndex: 1, + }, + { + id: 'rect2', + type: TLDrawShapeType.Rectangle, + childIndex: 2, + }, + { + id: 'rect3', + type: TLDrawShapeType.Rectangle, + childIndex: 3, + } + ) + .selectTool(TLDrawShapeType.Rectangle) + .createActiveToolShape([0, 0], 'rect4') + + expect(tlstate.getShape('rect4').childIndex).toBe(4) + + tlstate + .group(['rect2', 'rect3', 'rect4'], 'groupA') + .selectTool(TLDrawShapeType.Rectangle) + .createActiveToolShape([0, 0], 'rect5') + + expect(tlstate.getShape('groupA').childIndex).toBe(2) + expect(tlstate.getShape('rect5').childIndex).toBe(3) + }) + }) }) diff --git a/packages/tldraw/src/state/tlstate.ts b/packages/tldraw/src/state/tlstate.ts index 1a01dc943..0e7eeb27d 100644 --- a/packages/tldraw/src/state/tlstate.ts +++ b/packages/tldraw/src/state/tlstate.ts @@ -113,6 +113,8 @@ export class TLDrawState extends StateManager { isCreating = false + selectedGroupId?: string + constructor(id = Utils.uniqueId()) { super(initialData, id, 2, (prev, next, prevVersion) => { const state = { ...prev } @@ -1765,18 +1767,9 @@ export class TLDrawState extends StateManager { brushUpdater.clear() break } - case TLDrawStatus.Translating: { - this.cancelSession() - break - } - case TLDrawStatus.Transforming: { - this.cancelSession() - break - } - case TLDrawStatus.Rotating: { - this.cancelSession() - break - } + case TLDrawStatus.Translating: + case TLDrawStatus.Transforming: + case TLDrawStatus.Rotating: case TLDrawStatus.Creating: { this.cancelSession() break @@ -1870,8 +1863,7 @@ export class TLDrawState extends StateManager { return this } - createActiveToolShape = (point: number[]): this => { - const id = Utils.uniqueId() + createActiveToolShape = (point: number[], id = Utils.uniqueId()): this => { const pagePoint = Vec.round(this.getPagePoint(point)) if (this.appState.activeTool === 'select') return this @@ -1883,7 +1875,11 @@ export class TLDrawState extends StateManager { const shapes = this.getShapes() const childIndex = - shapes.length === 0 ? 1 : shapes.sort((a, b) => b.childIndex - a.childIndex)[0].childIndex + 1 + shapes.length === 0 + ? 1 + : shapes + .filter((shape) => shape.parentId === this.currentPageId) + .sort((a, b) => b.childIndex - a.childIndex)[0].childIndex + 1 this.patchState( { @@ -2141,7 +2137,7 @@ export class TLDrawState extends StateManager { } else if (this.selectedIds.includes(info.target)) { // If we're holding shift... if (info.shiftKey) { - // Unless we just shift-selected the shape, remove it from the selected shapes + // unless we just shift-selected the shape, remove it from the selected shapes if (this.pointedId !== info.target) { this.select(...this.selectedIds.filter((id) => id !== info.target)) } @@ -2259,19 +2255,59 @@ export class TLDrawState extends StateManager { switch (this.appState.activeTool) { case 'select': { if (info.metaKey) { - // While holding command key, start a brush session - this.startBrushSession(this.getPagePoint(info.point)) + if (info.shiftKey) { + // While holding command and shift, select or deselect + // the shape, ignoring any group that may contain it + this.pointedId = info.target + + if (this.selectedIds.includes(info.target)) { + // Deselect if selected + this.select(...this.selectedIds.filter((id) => id !== info.target)) + } else { + // Otherwise, push select the shape + this.select(...this.selectedIds, info.target) + } + } else { + // While holding just command key, start a brush session + this.startBrushSession(this.getPagePoint(info.point)) + } return } - if (!this.selectedIds.includes(info.target)) { - this.pointedId = info.target - // Set the pointed ID to the shape that was clicked. + // If we've clicked on a shape that is inside of a group, + // then select the group rather than the shape. + let shapeIdToSelect: string + const { parentId } = this.getShape(info.target) - // If the shape is not selected; then if the user is pressing shift, + // If the pointed shape is a child of the page, select the + // target shape and clear the selected group id. + if (parentId === this.currentPageId) { + shapeIdToSelect = info.target + this.selectedGroupId = undefined + } else { + // If the parent is some other group... + if (parentId === this.selectedGroupId) { + // If that group is the selected group, then select + // the target shape. + shapeIdToSelect = info.target + } else { + // Otherwise, select the group and clear the selected + // group id. + shapeIdToSelect = parentId + this.selectedGroupId = undefined + } + } + + if (!this.selectedIds.includes(shapeIdToSelect)) { + // Set the pointed ID to the shape that was clicked. + this.pointedId = shapeIdToSelect + + // If the shape is not selected: then if the user is pressing shift, // add the shape to the current selection; otherwise, set the shape as // the only selected shape. - this.select(...(info.shiftKey ? [...this.selectedIds, info.target] : [info.target])) + this.select( + ...(info.shiftKey ? [...this.selectedIds, shapeIdToSelect] : [shapeIdToSelect]) + ) } this.setStatus(TLDrawStatus.PointingBounds) @@ -2281,7 +2317,30 @@ export class TLDrawState extends StateManager { break } case TLDrawStatus.PointingBounds: { - this.pointedId = info.target + // If we've clicked on a shape that is inside of a group, + // then select the group rather than the shape. + + if (info.metaKey && info.shiftKey) { + const targetId = this.pageState.hoveredId + if (targetId) { + this.pointedId = targetId + const shape = this.getShape(targetId) + if (this.selectedIds.includes(targetId)) { + this.select(...this.selectedIds.filter((id) => id !== targetId)) + } else { + if (this.selectedIds.includes(shape.parentId)) { + this.select(targetId) + } else { + this.select(...this.selectedIds, targetId) + } + } + return + } + } + + const { parentId } = this.getShape(info.target) + this.pointedId = parentId === this.currentPageId ? info.target : parentId + break } } @@ -2293,10 +2352,15 @@ export class TLDrawState extends StateManager { onDoubleClickShape: TLPointerEventHandler = (info) => { switch (this.appState.status.current) { + case TLDrawStatus.Idle: case TLDrawStatus.PointingBounds: { switch (this.appState.activeTool) { case 'select': { - this.startTextSession(info.target) + const shape = this.getShape(info.target) + if (shape.parentId !== this.currentPageId) { + this.selectedGroupId = shape.parentId + } + this.select(info.target) break } } diff --git a/packages/tldraw/src/test/state-utils.tsx b/packages/tldraw/src/test/state-utils.tsx index 5be2a02d0..68d01bbe0 100644 --- a/packages/tldraw/src/test/state-utils.tsx +++ b/packages/tldraw/src/test/state-utils.tsx @@ -17,6 +17,11 @@ export class TLStateUtils { this.tlstate = tlstate } + hoverShape = (id: string, options: PointerOptions = {}) => { + const { tlstate } = this + tlstate.onHoverShape(inputs.pointerDown(this.getPoint(options), id), {} as React.PointerEvent) + } + pointCanvas = (options: PointerOptions = {}) => { this.tlstate.onPointCanvas( inputs.pointerDown(this.getPoint(options), 'canvas'), @@ -62,7 +67,7 @@ export class TLStateUtils { stopPointing = (target = 'canvas', options: PointerOptions = {}) => { this.tlstate.onPointerUp( - inputs.pointerDown(this.getPoint(options), target), + inputs.pointerUp(this.getPoint(options), target), {} as React.PointerEvent ) return this @@ -75,6 +80,9 @@ export class TLStateUtils { } clickShape = (id: string, options: PointerOptions = {}) => { + if (this.tlstate.selectedIds.includes(id)) { + this.pointBounds(options) + } this.pointShape(id, options) this.stopPointing(id, options) return this @@ -82,7 +90,7 @@ export class TLStateUtils { clickBounds = (options: PointerOptions = {}) => { this.pointBounds(options) - this.stopPointing() + this.stopPointing('bounds', options) return this }