Fix group element removing (#1676)

src/actions/actionDeleteSelected.tsx

@@ -1,4 +1,4 @@
-import { deleteSelectedElements, isSomeElementSelected } from "../scene";
+import { isSomeElementSelected } from "../scene";
 import { KEYS } from "../keys";
 import { ToolButton } from "../components/ToolButton";
 import React from "react";
@@ -6,14 +6,49 @@ import { trash } from "../components/icons";
 import { t } from "../i18n";
 import { register } from "./register";
 import { getNonDeletedElements } from "../element";
+import { ExcalidrawElement } from "../element/types";
+import { AppState } from "../types";
+import { newElementWith } from "../element/mutateElement";
+import { getElementsInGroup } from "../groups";
+
+const deleteSelectedElements = (
+  elements: readonly ExcalidrawElement[],
+  appState: AppState,
+) => {
+  return {
+    elements: elements.map((el) => {
+      if (appState.selectedElementIds[el.id]) {
+        return newElementWith(el, { isDeleted: true });
+      }
+      return el;
+    }),
+    appState: {
+      ...appState,
+      selectedElementIds: {},
+    },
+  };
+};
 
 export const actionDeleteSelected = register({
   name: "deleteSelectedElements",
   perform: (elements, appState) => {
-    const {
+    let {
       elements: nextElements,
       appState: nextAppState,
     } = deleteSelectedElements(elements, appState);
+
+    if (appState.editingGroupId) {
+      const siblingElements = getElementsInGroup(
+        getNonDeletedElements(nextElements),
+        appState.editingGroupId!,
+      );
+      if (siblingElements.length) {
+        nextAppState = {
+          ...nextAppState,
+          selectedElementIds: { [siblingElements[0].id]: true },
+        };
+      }
+    }
     return {
       elements: nextElements,
       appState: {

src/components/App.tsx

@@ -30,7 +30,6 @@ import {
   isNonDeletedElement,
 } from "../element";
 import {
-  deleteSelectedElements,
   getElementsWithinSelection,
   isOverScrollBars,
   getElementAtPosition,
@@ -126,7 +125,7 @@ import { invalidateShapeForElement } from "../renderer/renderElement";
 import { unstable_batchedUpdates } from "react-dom";
 import { SceneStateCallbackRemover } from "../scene/globalScene";
 import { isLinearElement } from "../element/typeChecks";
-import { actionFinalize } from "../actions";
+import { actionFinalize, actionDeleteSelected } from "../actions";
 import {
   restoreUsernameFromLocalStorage,
   saveUsernameToLocalStorage,
@@ -593,13 +592,7 @@ class App extends React.Component<any, AppState> {
       return;
     }
     this.copyAll();
-    const { elements: nextElements, appState } = deleteSelectedElements(
+    this.actionManager.executeAction(actionDeleteSelected);
-      globalSceneState.getElementsIncludingDeleted(),
-      this.state,
-    );
-    globalSceneState.replaceAllElements(nextElements);
-    history.resumeRecording();
-    this.setState({ ...appState });
     event.preventDefault();
   });
 

src/element/mutateElement.ts

@@ -34,7 +34,7 @@ export const mutateElement = <TElement extends Mutable<ExcalidrawElement>>(
       if (
         (element as any)[key] === value &&
         // if object, always update in case its deep prop was mutated
-        (typeof value !== "object" || value === null)
+        (typeof value !== "object" || value === null || key === "groupIds")
       ) {
         continue;
       }

src/element/newElement.ts

@@ -12,6 +12,7 @@ import { measureText, getFontString } from "../utils";
 import { randomInteger, randomId } from "../random";
 import { newElementWith } from "./mutateElement";
 import { getNewGroupIdsForDuplication } from "../groups";
+import { AppState } from "../types";
 
 type ElementConstructorOpts = {
   x: ExcalidrawGenericElement["x"];
@@ -169,7 +170,7 @@ export const deepCopyElement = (val: any, depth: number = 0) => {
  * @param overrides Any element properties to override
  */
 export const duplicateElement = <TElement extends Mutable<ExcalidrawElement>>(
-  editingGroupId: GroupId | null,
+  editingGroupId: AppState["editingGroupId"],
   groupIdMapForOperation: Map<GroupId, GroupId>,
   element: TElement,
   overrides?: Partial<TElement>,

src/element/types.ts

@@ -21,7 +21,7 @@ type _ExcalidrawElementBase = Readonly<{
   version: number;
   versionNonce: number;
   isDeleted: boolean;
-  groupIds: GroupId[];
+  groupIds: readonly GroupId[];
 }>;
 
 export type ExcalidrawSelectionElement = _ExcalidrawElementBase & {

src/groups.ts

@@ -7,15 +7,31 @@ export function selectGroup(
   appState: AppState,
   elements: readonly NonDeleted<ExcalidrawElement>[],
 ): AppState {
+  const elementsInGroup = elements.filter((element) =>
+    element.groupIds.includes(groupId),
+  );
+
+  if (elementsInGroup.length < 2) {
+    if (
+      appState.selectedGroupIds[groupId] ||
+      appState.editingGroupId === groupId
+    ) {
+      return {
+        ...appState,
+        selectedGroupIds: { ...appState.selectedGroupIds, [groupId]: false },
+        editingGroupId: null,
+      };
+    }
+    return appState;
+  }
+
   return {
     ...appState,
     selectedGroupIds: { ...appState.selectedGroupIds, [groupId]: true },
     selectedElementIds: {
       ...appState.selectedElementIds,
       ...Object.fromEntries(
-        elements
+        elementsInGroup.map((element) => [element.id, true]),
-          .filter((element) => element.groupIds.includes(groupId))
-          .map((element) => [element.id, true]),
       ),
     },
   };
@@ -89,8 +105,8 @@ export function getSelectedGroupIdForElement(
 }
 
 export function getNewGroupIdsForDuplication(
-  groupIds: GroupId[],
+  groupIds: ExcalidrawElement["groupIds"],
-  editingGroupId: GroupId | null,
+  editingGroupId: AppState["editingGroupId"],
   mapper: (groupId: GroupId) => GroupId,
 ) {
   const copy = [...groupIds];
@@ -107,9 +123,9 @@ export function getNewGroupIdsForDuplication(
 }
 
 export function addToGroup(
-  prevGroupIds: GroupId[],
+  prevGroupIds: ExcalidrawElement["groupIds"],
   newGroupId: GroupId,
-  editingGroupId: GroupId | null,
+  editingGroupId: AppState["editingGroupId"],
 ) {
   // insert before the editingGroupId, or push to the end.
   const groupIds = [...prevGroupIds];
@@ -123,7 +139,7 @@ export function addToGroup(
 }
 
 export function removeFromSelectedGroups(
-  groupIds: GroupId[],
+  groupIds: ExcalidrawElement["groupIds"],
   selectedGroupIds: { [groupId: string]: boolean },
 ) {
   return groupIds.filter((groupId) => !selectedGroupIds[groupId]);

src/history.ts

@@ -22,6 +22,7 @@ const clearAppStatePropertiesForHistory = (appState: AppState) => {
   return {
     selectedElementIds: appState.selectedElementIds,
     viewBackgroundColor: appState.viewBackgroundColor,
+    editingGroupId: appState.editingGroupId,
     name: appState.name,
   };
 };

src/scene/index.ts

@@ -1,6 +1,5 @@
 export { isOverScrollBars } from "./scrollbars";
 export {
-  deleteSelectedElements,
   isSomeElementSelected,
   getElementsWithinSelection,
   getCommonAttributeOfSelectedElements,

src/scene/selection.ts

@@ -4,7 +4,6 @@ import {
 } from "../element/types";
 import { getElementAbsoluteCoords, getElementBounds } from "../element";
 import { AppState } from "../types";
-import { newElementWith } from "../element/mutateElement";
 
 export const getElementsWithinSelection = (
   elements: readonly NonDeletedExcalidrawElement[],
@@ -31,24 +30,6 @@ export const getElementsWithinSelection = (
   });
 };
 
-export const deleteSelectedElements = (
-  elements: readonly ExcalidrawElement[],
-  appState: AppState,
-) => {
-  return {
-    elements: elements.map((el) => {
-      if (appState.selectedElementIds[el.id]) {
-        return newElementWith(el, { isDeleted: true });
-      }
-      return el;
-    }),
-    appState: {
-      ...appState,
-      selectedElementIds: {},
-    },
-  };
-};
-
 export const isSomeElementSelected = (
   elements: readonly NonDeletedExcalidrawElement[],
   appState: AppState,

src/types.ts

@@ -71,8 +71,10 @@ export type AppState = {
   showShortcutsDialog: boolean;
   zenModeEnabled: boolean;
 
-  // groups
+  /** top-most selected groups (i.e. does not include nested groups) */
   selectedGroupIds: { [groupId: string]: boolean };
+  /** group being edited when you drill down to its constituent element
+    (e.g. when you double-click on a group's element) */
   editingGroupId: GroupId | null;
 };