Merge pull request #2410 from concord-consortium/188129121-history-bu…

…g-fixes

Make history playback work for "X It!"

src/models/history/tree-manager.test.ts

@@ -226,6 +226,54 @@ const action4 = {
   undoable: true
 };
 
+const sharedModelChange = {
+    "action": "/content/sharedModelMap/sm1/sharedModel/setValue",
+    "created": expect.any(Number),
+    "id": expect.any(String),
+    "records": [
+      {
+        "action": "/content/sharedModelMap/sm1/sharedModel/setValue",
+        "inversePatches": [
+          {
+            "op": "replace",
+            "path": "/content/sharedModelMap/sm1/sharedModel/value",
+            "value": undefined,
+          },
+        ],
+        "patches": [
+          {
+            "op": "replace",
+            "path": "/content/sharedModelMap/sm1/sharedModel/value",
+            "value": "shared value",
+          },
+        ],
+        "tree": "test",
+      },
+      {
+        "action": "/handleSharedModelChanges",
+        "inversePatches": [
+          {
+            "op": "replace",
+            "path": "/content/tileMap/t1/content/text",
+          },
+        ],
+        "patches": [
+           {
+            "op": "replace",
+            "path": "/content/tileMap/t1/content/text",
+            "value": "shared value-tile",
+          },
+        ],
+        "tree": "test",
+      }
+    ],
+    "state": "complete",
+    "tree": "test",
+    "undoable": true,
+  };
+
+
+
 /**
  * Remove the Jest `expect.any(Number)` on created, and provide a real id.
  * @param entry
@@ -295,6 +343,17 @@ it("can replay the history entries", async () => {
   expect(getSnapshot(manager.document.history)).toEqual(history);
 });
 
+it("records tile model changes in response to shared model changes", async () => {
+  const {tileContent, manager, sharedModel} = setupDocument();
+  sharedModel.setValue("shared value");
+  await expectEntryToBeComplete(manager, 1);
+  expect(tileContent.text).toBe("shared value-tile");
+  expect(tileContent.updateCount).toBe(1);
+
+  const changeDocument = manager.document as Instance<typeof CDocument>;
+  expect(getSnapshot(changeDocument.history)).toEqual([sharedModelChange]);
+});
+
 // TODO: it would nicer to use a custom Jest matcher here so we can
 // provide a better error message when it fails
 async function expectEntryToBeComplete(manager: Instance<typeof TreeManager>, length: number) {

src/models/history/tree-manager.ts

@@ -543,13 +543,17 @@ export const TreeManager = types
     const startingIndex = direction === 1 ? self.numHistoryEventsApplied : self.numHistoryEventsApplied - 1;
     const endingIndex = direction === 1 ? newHistoryPosition : newHistoryPosition - 1;
     for (let i=startingIndex; i !== endingIndex; i=i+direction) {
-      const entry = self.document.history.at(i);
-      for (const treeEntry of (entry?.records || [])) {
-        const patches = treePatches[treeEntry.tree];
+      const historyEntry = self.document.history.at(i);
+      let records = historyEntry ? [ ...historyEntry.records] : [];
+      if (direction === -1) {
+        records = records.reverse();
+      }
+      for (const entryRecord of records) {
+        const patches = treePatches[entryRecord.tree];
         if (newHistoryPosition > self.numHistoryEventsApplied) {
-          patches?.push(...treeEntry.getPatches(HistoryOperation.Redo));
+          patches?.push(...entryRecord.getPatches(HistoryOperation.Redo));
         } else {
-          patches?.push(...treeEntry.getPatches(HistoryOperation.Undo));
+          patches?.push(...entryRecord.getPatches(HistoryOperation.Undo));
         }
       }
     }

src/models/history/tree-monitor.ts

@@ -220,6 +220,7 @@ export class TreeMonitor {
     // TODO: If there are multiple shared model changes, we might want
     // to send them all to the tree at the same time, that way it can
     // inform the tiles of all changes at the same time.
+    const exchangesToProcess: { id: string, path: string }[] = [];
     for (const [sharedModelPath, numModifications] of Object.entries(sharedModelModifications)) {
       if (numModifications > 0) {
         // If a shared model has been deleted, we can't run these callbacks without errors,
@@ -251,32 +252,7 @@ export class TreeMonitor {
         await this.manager.startExchange(historyEntryId, sharedModelChangesExchangeId,
           "recordAction.sharedModelChanges");
 
-        // Recursion: handleSharedModelChanges is an action on the
-        // tree, and we are currently in a middleware that is
-        // monitoring actions on that tree. At this point in the
-        // middleware we are finishing a different action. Calling
-        // handleSharedModelChanges starts a new top level action:
-        // an action with no parent actions. This is what we want so
-        // we can record any changes made to the tree as part of the
-        // undo entry. I don't know if calling an action from a
-        // middleware is an officially supported or tested approach.
-        // It is working now. If it stops working we could delay the
-        // call to handleSharedModelChanges with a setTimeout.
-        //
-        // It is recursive because we will end up back in this
-        // recordAction function. Because we are awaiting
-        // handleSharedModelChanges that second recursive
-        // recordAction will get kicked off before this call to
-        // handleSharedModelChanges returns. The tree's
-        // implementation of handleSharedModelChanges should not
-        // modify the shared model itself or we could get into an
-        // infinite loop.
-        //
-        // Within this recursive call to recordAction,
-        // addTreePatchRecord will be called. This is how the
-        // startExchange above is closed out.
-        await this.tree.handleSharedModelChanges(historyEntryId, sharedModelChangesExchangeId,
-          call, sharedModelPath, initialSharedModelMap);
+        exchangesToProcess.push({ id: sharedModelChangesExchangeId, path: sharedModelPath });
       }
     }
 
@@ -301,5 +277,36 @@ export class TreeMonitor {
       inversePatches,
     };
     this.manager.addTreePatchRecord(historyEntryId, exchangeId, record);
+
+    for (const exchange of exchangesToProcess) {
+      // Now that the patches have been recorded, we can process the shared model changes
+      // Recursion: handleSharedModelChanges is an action on the
+      // tree, and we are currently in a middleware that is
+      // monitoring actions on that tree. At this point in the
+      // middleware we are finishing a different action. Calling
+      // handleSharedModelChanges starts a new top level action:
+      // an action with no parent actions. This is what we want so
+      // we can record any changes made to the tree as part of the
+      // undo entry. I don't know if calling an action from a
+      // middleware is an officially supported or tested approach.
+      // It is working now. If it stops working we could delay the
+      // call to handleSharedModelChanges with a setTimeout.
+      //
+      // It is recursive because we will end up back in this
+      // recordAction function. Because we are awaiting
+      // handleSharedModelChanges that second recursive
+      // recordAction will get kicked off before this call to
+      // handleSharedModelChanges returns. The tree's
+      // implementation of handleSharedModelChanges should not
+      // modify the shared model itself or we could get into an
+      // infinite loop.
+      //
+      // Within this recursive call to recordAction,
+      // addTreePatchRecord will be called. This is how the
+      // startExchange above is closed out.
+      await this.tree.handleSharedModelChanges(historyEntryId, exchange.id,
+        call, exchange.path, initialSharedModelMap);
+    }
+
   }
 }

src/models/history/undo-store.test.ts

@@ -610,21 +610,21 @@ const initialSharedModelUpdateEntry = {
   created: expect.any(Number),
   id: expect.any(String),
   records: [
-    { action: "/handleSharedModelChanges",
+    { action: "/content/sharedModelMap/sm1/sharedModel/setValue",
       inversePatches: [
-        { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined}
+        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined}
       ],
       patches: [
-        { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"}
+        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"}
       ],
       tree: "test"
     },
-    { action: "/content/sharedModelMap/sm1/sharedModel/setValue",
+    { action: "/handleSharedModelChanges",
       inversePatches: [
-        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined}
+        { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined}
       ],
       patches: [
-        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"}
+        { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"}
       ],
       tree: "test"
     }
@@ -710,19 +710,19 @@ const redoSharedModelEntry = {
   records: [
     { action: "/applyPatchesFromManager",
       inversePatches: [
-        { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined}
+        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined}
       ],
       patches: [
-        { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"}
+        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"}
       ],
       tree: "test"
     },
     { action: "/applyPatchesFromManager",
       inversePatches: [
-        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined}
+        { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined}
       ],
       patches: [
-        { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"}
+        { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"}
       ],
       tree: "test"
     }
@@ -821,24 +821,6 @@ it("can track the addition of a new shared model", async () => {
       created: expect.any(Number),
       id: expect.any(String),
       records: [
-        {
-          tree: "test",
-          action: "/handleSharedModelChanges",
-          patches: [
-            {
-              op: "replace",
-              path: "/content/tileMap/t1/content/text",
-              value: "new model-tile"
-            }
-          ],
-          inversePatches: [
-            {
-              op: "replace",
-              path: "/content/tileMap/t1/content/text",
-              value: undefined
-            }
-          ]
-        },
         {
           action: "/content/addSharedModel",
           inversePatches: [
@@ -859,6 +841,24 @@ it("can track the addition of a new shared model", async () => {
             }
           ],
           tree: "test"
+        },
+        {
+          tree: "test",
+          action: "/handleSharedModelChanges",
+          patches: [
+            {
+              op: "replace",
+              path: "/content/tileMap/t1/content/text",
+              value: "new model-tile"
+            }
+          ],
+          inversePatches: [
+            {
+              op: "replace",
+              path: "/content/tileMap/t1/content/text",
+              value: undefined
+            }
+          ]
         }
       ],
       state: "complete",

src/models/history/undo-store.ts

@@ -65,16 +65,20 @@ export const UndoStore = types
     const uniqueTreeIds = [...new Set(treeIds)];
 
     // first disable shared model syncing in each tree
+    // Order of calls does not matter for this operation.
     const startPromises = uniqueTreeIds.map(treeId => {
       const startExchangeId = nanoid();
       manager.startExchange(historyEntryId, startExchangeId, "UndoStore.applyPatchesToTrees.start");
-
       return manager.trees[treeId].startApplyingPatchesFromManager(historyEntryId, startExchangeId);
     });
     yield Promise.all(startPromises);
 
-    // apply the patches to all trees
-    const applyPromises = treePatchRecords.map(treePatchRecord => {
+    // apply the patches to all trees, in reverse order if we are undoing changes.
+    const undoRecords = [ ...treePatchRecords ];
+    if (opType === HistoryOperation.Undo) {
+      undoRecords.reverse();
+    }
+    for (const treePatchRecord of undoRecords) {
       // console.log(`send tile entry to ${opType} to the tree`, getSnapshot(treeEntry));
 
       // If there are multiple trees, and a patch is applied to shared model
@@ -91,10 +95,9 @@ export const UndoStore = types
       manager.startExchange(historyEntryId, applyExchangeId, "UndoStore.applyPatchesToTrees.apply");
 
       const tree = manager.trees[treePatchRecord.tree];
-      return tree.applyPatchesFromManager(historyEntryId,  applyExchangeId,
+      yield tree.applyPatchesFromManager(historyEntryId,  applyExchangeId,
           treePatchRecord.getPatches(opType));
-    });
-    yield Promise.all(applyPromises);
+    }
 
     // finish the patch application
     //

src/plugins/data-card/components/case-attribute.tsx

@@ -1,9 +1,11 @@
 import React, { useCallback, useEffect, useState } from "react";
 import { observer } from "mobx-react";
+import { isAlive } from "mobx-state-tree";
 import escapeStringRegexp from "escape-string-regexp";
 import { useCombobox } from "downshift";
 import { uniq } from "lodash";
 import { VisuallyHidden } from "@chakra-ui/react";
+import classNames from "classnames";
 import { gImageMap } from "../../../models/image-map";
 import { ITileModel } from "../../../models/tiles/tile-model";
 import { DataCardContentModelType } from "../data-card-content";
@@ -25,7 +27,6 @@ import NumberTypeIcon from "../assets/id-type-number.svg";
 import ExpandDownIcon from "../assets/expand-more-icon.svg";
 
 import './single-card-data-area.scss';
-import classNames from "classnames";
 
 const typeIcons = {
   "date": <DateTypeIcon />,
@@ -57,13 +58,16 @@ export const CaseAttribute: React.FC<IProps> = observer(props => {
     model, caseId, attrKey, currEditAttrId, currEditFacet,
     setCurrEditFacet, setCurrEditAttrId, readOnly
   } = props;
+  if (!isAlive(model)) {
+    console.log("rendering unalive model", model);
+  }
   const content = model.content as DataCardContentModelType;
   const dataSet = content.dataSet;
   const cell = { attributeId: attrKey, caseId: caseId ?? "" };
   const isLinked = useIsLinked();
 
   const getName = useCallback(() => {
-    return content.dataSet.attrFromID(attrKey).name;
+    return content.dataSet.attrFromID(attrKey)?.name;
   }, [attrKey, content.dataSet]);
 
   const getValue = useCallback(() => {
@@ -92,7 +96,9 @@ export const CaseAttribute: React.FC<IProps> = observer(props => {
   }, [editingValue, valueCandidate.length]);
 
   useEffect(() => {
-    const nameLines = measureTextLines(getName(), kFieldWidthFactor);
+    const name = getName();
+    if (!name) return;
+    const nameLines = measureTextLines(name, kFieldWidthFactor);
     const nameCandidateLines = measureTextLines(nameCandidate, kFieldWidthFactor);
     const nameLinesNeeded = Math.max(nameLines, nameCandidateLines);
     const valueLines = measureTextLines(valueStr, kFieldWidthFactor);

src/plugins/data-card/components/data-card-toolbar-buttons.tsx

@@ -53,7 +53,7 @@ export const DeleteAttrButton = () => {
   const isEditingValue = !!context?.currEditAttrId && context?.currEditFacet === "value";
 
   const handleClick = () => {
-    const thisCaseId = content?.dataSet.caseIDFromIndex(content.caseIndex);
+    const thisCaseId = content?.dataSet.caseIDFromIndex(content.caseIndexNumber);
     if (thisCaseId && context){
       content?.setAttValue(thisCaseId, context.currEditAttrId, "");
     }