[fix] Ungrouping (#144)

* Adds auto ungroup if grouping only a grouped shape

* Adds test for multiple shape ungrouping

packages/tldraw/src/state/command/group/group.command.spec.ts

@@ -318,6 +318,36 @@ describe('Group command', () => {
       ])
     })
 
+    it('Ungroups if the only shape selected is a group', () => {
+      tlstate.resetDocument().createShapes(
+        {
+          id: 'rect1',
+          type: TLDrawShapeType.Rectangle,
+          childIndex: 1,
+        },
+        {
+          id: 'rect2',
+          type: TLDrawShapeType.Rectangle,
+          childIndex: 2,
+        },
+        {
+          id: 'rect3',
+          type: TLDrawShapeType.Rectangle,
+          childIndex: 3,
+        }
+      )
+
+      expect(tlstate.shapes.length).toBe(3)
+
+      tlstate.selectAll().group()
+
+      expect(tlstate.shapes.length).toBe(4)
+
+      tlstate.selectAll().group()
+
+      expect(tlstate.shapes.length).toBe(3)
+    })
+
     /*
       The layers should be in the same order as the original layers as
       they would have appeared on a layers tree (lowest child index

packages/tldraw/src/state/command/ungroup/ungroup.command.spec.ts

@@ -37,13 +37,29 @@ describe('Ungroup command', () => {
         .loadDocument(mockDocument)
         .group(['rect1', 'rect2'], 'groupA')
         .createPage('page2')
-        .ungroup('groupA', 'page1')
+        .ungroup(['groupA'], 'page1')
 
       expect(tlstate.getShape('groupA', 'page1')).toBeUndefined()
       tlstate.undo()
       expect(tlstate.getShape('groupA', 'page1')).toBeDefined()
     })
 
+    it('Ungroups multiple selected groups', () => {
+      tlstate
+        .loadDocument(mockDocument)
+        .createShapes({
+          id: 'rect4',
+          type: TLDrawShapeType.Rectangle,
+        })
+        .group(['rect1', 'rect2'], 'groupA')
+        .group(['rect3', 'rect4'], 'groupB')
+        .selectAll()
+        .ungroup()
+
+      expect(tlstate.getShape('groupA', 'page1')).toBeUndefined()
+      expect(tlstate.getShape('groupB', 'page1')).toBeUndefined()
+    })
+
     it('Does not ungroup if a group shape is not selected', () => {
       tlstate.loadDocument(mockDocument).select('rect1')
       const before = tlstate.state
@@ -52,6 +68,33 @@ describe('Ungroup command', () => {
       expect(tlstate.state).toStrictEqual(before)
     })
 
+    it('Correctly selects children after ungrouping', () => {
+      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,
+          }
+        )
+        .group(['rect1', 'rect2'], 'groupA')
+        .selectAll()
+        .ungroup()
+
+      // State should not have changed
+      expect(tlstate.selectedIds).toStrictEqual(['rect3', 'rect1', 'rect2'])
+    })
+
     it('Reparents shapes to the page at the correct childIndex', () => {
       const tlstate = new TLDrawState()
         .createShapes(
@@ -80,7 +123,7 @@ describe('Ungroup command', () => {
       expect(tlstate.getShape('rect2').childIndex).toBe(2)
       expect(tlstate.getShape('rect3').childIndex).toBe(3)
 
-      tlstate.ungroup('groupA')
+      tlstate.ungroup()
 
       expect(tlstate.getShape('rect1').childIndex).toBe(1)
       expect(tlstate.getShape('rect2').childIndex).toBe(2)

packages/tldraw/src/state/command/ungroup/ungroup.command.ts

@@ -3,100 +3,108 @@ import type { Data, TLDrawCommand } from '~types'
 import { TLDR } from '~state/tldr'
 import type { Patch } from 'rko'
 
-export function ungroup(data: Data, groupId: string, pageId: string): TLDrawCommand | undefined {
+export function ungroup(
+  data: Data,
+  selectedIds: string[],
+  groupShapes: GroupShape[],
+  pageId: string
+): TLDrawCommand | undefined {
   const beforeShapes: Record<string, Patch<TLDrawShape | undefined>> = {}
   const afterShapes: Record<string, Patch<TLDrawShape | undefined>> = {}
 
   const beforeBindings: Record<string, Patch<TLDrawBinding | undefined>> = {}
   const afterBindings: Record<string, Patch<TLDrawBinding | undefined>> = {}
 
-  // The group shape
-  const groupShape = TLDR.getShape<GroupShape>(data, groupId, pageId)
-
-  const idsToUngroup = groupShape.children
-  const shapesToUngroup: TLDrawShape[] = []
-  const deletedGroupIds: string[] = []
-
-  // Collect all of the shapes to group (and their ids)
-  for (const id of idsToUngroup) {
-    const shape = TLDR.getShape(data, id, pageId)
-    shapesToUngroup.push(shape)
-  }
+  const beforeSelectedIds = selectedIds
+  const afterSelectedIds = selectedIds.filter((id) => !groupShapes.find((shape) => shape.id === id))
 
-  // We'll start placing the shapes at this childIndex
-  const startingChildIndex = groupShape.childIndex
+  // The group shape
+  groupShapes.forEach((groupShape) => {
+    const shapesToReparent: TLDrawShape[] = []
+    const deletedGroupIds: string[] = []
+
+    // Remove the group shape in the next state
+    beforeShapes[groupShape.id] = groupShape
+    afterShapes[groupShape.id] = undefined
+
+    // Select its children in the next state
+    groupShape.children.forEach((id) => {
+      afterSelectedIds.push(id)
+      const shape = TLDR.getShape(data, id, pageId)
+      shapesToReparent.push(shape)
+    })
 
-  // And we'll need to fit them under this child index
-  const endingChildIndex = TLDR.getChildIndexAbove(data, groupShape.id, pageId)
+    // We'll start placing the shapes at this childIndex
+    const startingChildIndex = groupShape.childIndex
 
-  const step = (endingChildIndex - startingChildIndex) / shapesToUngroup.length
+    // And we'll need to fit them under this child index
+    const endingChildIndex = TLDR.getChildIndexAbove(data, groupShape.id, pageId)
 
-  // An array of shapes in order by their child index
-  const sortedShapes = shapesToUngroup.sort((a, b) => a.childIndex - b.childIndex)
+    const step = (endingChildIndex - startingChildIndex) / shapesToReparent.length
 
-  // Remove the group shape
-  beforeShapes[groupId] = groupShape
-  afterShapes[groupId] = undefined
+    // An array of shapes in order by their child index
+    const sortedShapes = shapesToReparent.sort((a, b) => a.childIndex - b.childIndex)
 
-  // Reparent shapes to the page
-  sortedShapes.forEach((shape, index) => {
-    beforeShapes[shape.id] = {
-      parentId: shape.parentId,
-      childIndex: shape.childIndex,
-    }
+    // Reparent shapes to the page
+    sortedShapes.forEach((shape, index) => {
+      beforeShapes[shape.id] = {
+        parentId: shape.parentId,
+        childIndex: shape.childIndex,
+      }
 
-    afterShapes[shape.id] = {
-      parentId: pageId,
-      childIndex: startingChildIndex + step * index,
-    }
-  })
+      afterShapes[shape.id] = {
+        parentId: pageId,
+        childIndex: startingChildIndex + step * index,
+      }
+    })
 
-  const page = TLDR.getPage(data, pageId)
-
-  // We also need to delete bindings that reference the deleted shapes
-  Object.values(page.bindings)
-    .filter((binding) => binding.toId === groupId || binding.fromId === groupId)
-    .forEach((binding) => {
-      for (const id of [binding.toId, binding.fromId]) {
-        // If the binding references the deleted group...
-        if (afterShapes[id] === undefined) {
-          // Delete the binding
-          beforeBindings[binding.id] = binding
-          afterBindings[binding.id] = undefined
-
-          // Let's also look each the bound shape...
-          const shape = TLDR.getShape(data, id, pageId)
-
-          // If the bound shape has a handle that references the deleted binding...
-          if (shape.handles) {
-            Object.values(shape.handles)
-              .filter((handle) => handle.bindingId === binding.id)
-              .forEach((handle) => {
-                // Save the binding reference in the before patch
-                beforeShapes[id] = {
-                  ...beforeShapes[id],
-                  handles: {
-                    ...beforeShapes[id]?.handles,
-                    [handle.id]: { bindingId: binding.id },
-                  },
-                }
-
-                // Unless we're currently deleting the shape, remove the
-                // binding reference from the after patch
-                if (!deletedGroupIds.includes(id)) {
-                  afterShapes[id] = {
-                    ...afterShapes[id],
+    const page = TLDR.getPage(data, pageId)
+
+    // We also need to delete bindings that reference the deleted shapes
+    Object.values(page.bindings)
+      .filter((binding) => binding.toId === groupShape.id || binding.fromId === groupShape.id)
+      .forEach((binding) => {
+        for (const id of [binding.toId, binding.fromId]) {
+          // If the binding references the deleted group...
+          if (afterShapes[id] === undefined) {
+            // Delete the binding
+            beforeBindings[binding.id] = binding
+            afterBindings[binding.id] = undefined
+
+            // Let's also look each the bound shape...
+            const shape = TLDR.getShape(data, id, pageId)
+
+            // If the bound shape has a handle that references the deleted binding...
+            if (shape.handles) {
+              Object.values(shape.handles)
+                .filter((handle) => handle.bindingId === binding.id)
+                .forEach((handle) => {
+                  // Save the binding reference in the before patch
+                  beforeShapes[id] = {
+                    ...beforeShapes[id],
                     handles: {
-                      ...afterShapes[id]?.handles,
-                      [handle.id]: { bindingId: undefined },
+                      ...beforeShapes[id]?.handles,
+                      [handle.id]: { bindingId: binding.id },
                     },
                   }
-                }
-              })
+
+                  // Unless we're currently deleting the shape, remove the
+                  // binding reference from the after patch
+                  if (!deletedGroupIds.includes(id)) {
+                    afterShapes[id] = {
+                      ...afterShapes[id],
+                      handles: {
+                        ...afterShapes[id]?.handles,
+                        [handle.id]: { bindingId: undefined },
+                      },
+                    }
+                  }
+                })
+            }
           }
         }
-      }
-    })
+      })
+  })
 
   return {
     id: 'ungroup',
@@ -110,7 +118,7 @@ export function ungroup(data: Data, groupId: string, pageId: string): TLDrawComm
         },
         pageStates: {
           [pageId]: {
-            selectedIds: [groupId],
+            selectedIds: beforeSelectedIds,
           },
         },
       },
@@ -125,7 +133,7 @@ export function ungroup(data: Data, groupId: string, pageId: string): TLDrawComm
         },
         pageStates: {
           [pageId]: {
-            selectedIds: idsToUngroup,
+            selectedIds: afterSelectedIds,
           },
         },
       },

packages/tldraw/src/state/tlstate.ts

@@ -2225,7 +2225,12 @@ export class TLDrawState extends StateManager<Data> {
     groupId = Utils.uniqueId(),
     pageId = this.currentPageId
   ): this => {
+    if (ids.length === 1 && this.getShape(ids[0], pageId).type === TLDrawShapeType.Group) {
+      return this.ungroup(ids, pageId)
+    }
+
     if (ids.length < 2) return this
+
     const command = Commands.group(this.state, ids, groupId, pageId)
     if (!command) return this
     return this.setState(command)
@@ -2235,11 +2240,14 @@ export class TLDrawState extends StateManager<Data> {
    * Ungroup the selected groups.
    * @todo
    */
-  ungroup = (groupId = this.selectedIds[0], pageId = this.currentPageId): this => {
-    const shape = this.getShape(groupId, pageId)
-    if (shape.type !== TLDrawShapeType.Group) return this
+  ungroup = (ids = this.selectedIds, pageId = this.currentPageId): this => {
+    const groups = ids
+      .map((id) => this.getShape(id, pageId))
+      .filter((shape) => shape.type === TLDrawShapeType.Group)
+
+    if (groups.length === 0) return this
 
-    const command = Commands.ungroup(this.state, groupId, pageId)
+    const command = Commands.ungroup(this.state, ids, groups as GroupShape[], pageId)
     if (!command) return this
     return this.setState(command)
   }