Merge pull request #913 from concord-consortium/186144182-display-for…

…mula

Update formula's display form when attribute name is changed

v3/src/components/case-table/attribute-menu/edit-formula-modal.tsx

@@ -2,7 +2,7 @@ import {
   Button, FormControl, FormLabel, Input, ModalBody, ModalCloseButton, ModalFooter, ModalHeader,
   Textarea, Tooltip
 } from "@chakra-ui/react"
-import React, { useState } from "react"
+import React, { useEffect, useState } from "react"
 import { observer } from "mobx-react-lite"
 import { CodapModal } from "../../codap-modal"
 import t from "../../../utilities/translation/translate"
@@ -17,7 +17,11 @@ interface IProps {
 export const EditFormulaModal = observer(function EditFormulaModal({ attributeId, isOpen, onClose }: IProps) {
   const dataSet = useDataSetContext()
   const attribute = dataSet?.attrFromID(attributeId)
-  const [formula, setFormula] = useState(attribute?.formula.display || "")
+  const [formula, setFormula] = useState("")
+
+  useEffect(() => {
+    setFormula(attribute?.formula.display || "")
+  }, [attribute?.formula.display])
 
   const applyFormula = () => {
     attribute?.setDisplayFormula(formula)

v3/src/models/data/formula-manager.ts

@@ -5,7 +5,7 @@ import { CaseGroup, ICase, IGroupedCase, symParent } from "./data-set-types"
 import { onAnyAction } from "../../utilities/mst-utils"
 import {
   getFormulaDependencies, formulaError, getFormulaChildMostAggregateCollectionIndex, getIncorrectChildAttrReference,
-  getIncorrectParentAttrReference, safeSymbolName
+  getIncorrectParentAttrReference, safeSymbolName, reverseDisplayNameMap
 } from "./formula-utils"
 import {
   DisplayNameMap, IFormulaDependency, GLOBAL_VALUE, LOCAL_ATTR, ILocalAttributeDependency, IGlobalValueDependency,
@@ -278,6 +278,12 @@ export class FormulaManager {
     })
   }
 
+  updateDisplayFormulas() {
+    this.formulaMetadata.forEach(({ formula }) => {
+      formula.updateDisplayFormula()
+    })
+  }
+
   unregisterFormula(formulaId: string) {
     const formulaMetadata = this.formulaMetadata.get(formulaId)
     if (formulaMetadata) {
@@ -332,8 +338,9 @@ export class FormulaManager {
     })
   }
 
-  getDisplayNameMapForFormula(formulaId: string, makeSymbolNamesSafe = true) {
+  getDisplayNameMapForFormula(formulaId: string, options?: { useSafeSymbolNames: boolean }) {
     const { dataSet: localDataSet } = this.getFormulaContext(formulaId)
+    const { useSafeSymbolNames } = options || { useSafeSymbolNames: true }
 
     const displayNameMap: DisplayNameMap = {
       localNames: {},
@@ -343,7 +350,7 @@ export class FormulaManager {
     const mapAttributeNames = (dataSet: IDataSet, prefix: string) => {
       const result: Record<string, string> = {}
       dataSet.attributes.forEach(attr => {
-        result[makeSymbolNamesSafe ? safeSymbolName(attr.name) : attr.name] = `${prefix}${attr.id}`
+        result[useSafeSymbolNames ? safeSymbolName(attr.name) : attr.name] = `${prefix}${attr.id}`
       })
       return result
     }
@@ -373,8 +380,13 @@ export class FormulaManager {
     return displayNameMap
   }
 
+  getCanonicalNameMap(formulaId: string) {
+    const displayNameMap = this.getDisplayNameMapForFormula(formulaId, { useSafeSymbolNames: false })
+    return reverseDisplayNameMap(displayNameMap)
+  }
+
   observeDatasetChanges(dataSet: IDataSet) {
-    const dispose = onAnyAction(dataSet, mstAction => {
+    const disposeSetCollectionForAttributeObserver = onAnyAction(dataSet, mstAction => {
       switch (mstAction.name) {
         // When attribute is moved to a new collection, it usually affects grouping that is respected by formulas.
         // It'd be possible to optimize this by checking formula dependencies and limit number of updates, but
@@ -384,6 +396,15 @@ export class FormulaManager {
           break
       }
     })
+    // When any attribute name is updated, we need to update display formulas.
+    const disposeAttrNameReaction = reaction(
+      () => dataSet.attrNameMap,
+      () => this.updateDisplayFormulas()
+    )
+    const dispose = () => {
+      disposeSetCollectionForAttributeObserver()
+      disposeAttrNameReaction()
+    }
     this.dataSetMetadata.set(dataSet.id, { dispose })
   }
 

v3/src/models/data/formula-types.ts

@@ -23,6 +23,9 @@ export type DisplayNameMap = {
   }>
 }
 
+// A map of canonical names to display names. Canonical names are unique, so simple map is enough.
+export type CanonicalNameMap = Record<string, string>
+
 export interface ILocalAttributeDependency {
   type: "localAttribute"
   attrId: string

v3/src/models/data/formula-utils.test.ts

@@ -120,11 +120,11 @@ describe("canonicalToDisplay", () => {
   it("converts canonical formula to display formula maintaining whitespace characters", () => {
     expect(canonicalToDisplay(
       "mean(LOCAL_ATTR_ATTR_LIFE_SPAN) + GLOBAL_VALUE_GLOB_V1",
-      "mean (\nLifeSpan\n) + v1 ", displayNameMapExample
+      "mean (\nLifeSpan\n) + v1 ", reverseDisplayNameMap(displayNameMapExample)
     )).toEqual("mean (\nLifeSpan\n) + v1 ")
     expect(canonicalToDisplay(
       "mean(LOCAL_ATTR_ATTR_LIFE_SPAN) + LOCAL_ATTR_ATTR_ORDER * GLOBAL_VALUE_GLOB_V1",
-      "mean (\nOldLifeSpan\n) + OldOrder * OldV1", displayNameMapExample
+      "mean (\nOldLifeSpan\n) + OldOrder * OldV1", reverseDisplayNameMap(displayNameMapExample)
     )).toEqual("mean (\nLifeSpan\n) + Order * v1")
   })
   describe("when function name or constant is equal to attribute name", () => {
@@ -137,7 +137,7 @@ describe("canonicalToDisplay", () => {
     it("still converts canonical formula to display formula correctly", () => {
       expect(canonicalToDisplay(
         "mean(LOCAL_ATTR_ATTR_MEAN) + 'mean'",
-        "mean ( mean ) + 'mean'", displayMap
+        "mean ( mean ) + 'mean'", reverseDisplayNameMap(displayMap)
       )).toEqual("mean ( NewMeanAttr ) + 'mean'")
     })
   })
@@ -151,7 +151,7 @@ describe("canonicalToDisplay", () => {
     it("is enclosed in backticks", () => {
       expect(canonicalToDisplay(
         "mean(LOCAL_ATTR_ATTR_MEAN) + 'mean'",
-        "mean ( mean ) + 'mean'", testDisplayMap
+        "mean ( mean ) + 'mean'", reverseDisplayNameMap(testDisplayMap)
       )).toEqual("mean ( `new mean attribute 🙃` ) + 'mean'")
     })
   })
@@ -160,7 +160,7 @@ describe("canonicalToDisplay", () => {
       expect(canonicalToDisplay(
         "lookupByKey('DATA_ROLLER_COASTER', 'ATTR_PARK', 'ATTR_TOP_SPEED', LOCAL_ATTR_ATTR_ORDER) * 2",
         "lookupByKey('Old Roller Coaster', 'Old Park', 'Old Top Speed', OldOrder) * 2",
-        displayNameMapExample
+        reverseDisplayNameMap(displayNameMapExample)
       )).toEqual("lookupByKey('Roller Coaster', 'Park', 'Top Speed', Order) * 2")
     })
   })

v3/src/models/data/formula-utils.ts

@@ -1,6 +1,7 @@
-import { parse, MathNode, isFunctionNode, isSymbolNode } from "mathjs"
+import { parse, MathNode, isFunctionNode } from "mathjs"
 import {
-  LOCAL_ATTR, GLOBAL_VALUE, DisplayNameMap, IFormulaDependency, isConstantStringNode, isNonFunctionSymbolNode
+  LOCAL_ATTR, GLOBAL_VALUE, DisplayNameMap, CanonicalNameMap, IFormulaDependency, isConstantStringNode,
+  isNonFunctionSymbolNode,
 } from "./formula-types"
 import { typedFnRegistry } from "./formula-fn-registry"
 import type { IDataSet } from "./data-set"
@@ -45,7 +46,9 @@ export const customizeFormula = (formula: string) => {
   return formula.replace(/(?<!!)=(?!=)/g, "==")
 }
 
-export const reverseDisplayNameMap = (displayNameMap: DisplayNameMap) => {
+export const preprocessFormula = (formula: string) => customizeFormula(makeNamesSafe(formula))
+
+export const reverseDisplayNameMap = (displayNameMap: DisplayNameMap): CanonicalNameMap => {
   return Object.fromEntries([
     ...Object.entries(displayNameMap.localNames).map(([attrName, attrId]) => [attrId, attrName]),
     ...Object.entries(displayNameMap.dataSet).map(([dataSetName, dataSet]) => [dataSet.id, dataSetName]),
@@ -55,7 +58,7 @@ export const reverseDisplayNameMap = (displayNameMap: DisplayNameMap) => {
   ])
 }
 
-export const canonicalToDisplay = (canonical: string, originalDisplay: string, displayNameMap: DisplayNameMap) => {
+export const canonicalToDisplay = (canonical: string, originalDisplay: string, canonicalNameMap: CanonicalNameMap) => {
   // Algorithm is as follows:
   // 1. Parse original display formula and get all the names that need to be replaced.
   // 2. Parse canonical formula and get all the names that will replace the original names.
@@ -65,9 +68,8 @@ export const canonicalToDisplay = (canonical: string, originalDisplay: string, d
   // function names and constants might be identical to the symbol name. E.g. 'mean(mean) + "mean"' is a valid formula
   // if there's attribute called "mean". If we process function names and constants, it'll be handled correctly.
   originalDisplay = makeNamesSafe(originalDisplay) // so it can be parsed by MathJS
-  const idToName = reverseDisplayNameMap(displayNameMap)
   const getNameFromId = (id: string, wrapInBackTicks: boolean) => {
-    let name = idToName[id]
+    let name = canonicalNameMap[id]
     if (wrapInBackTicks && name && name !== safeSymbolName(name)) {
       name = `\`${name}\`` // wrap in backticks if it's not a valid symbol name
     }
@@ -110,7 +112,7 @@ export const ifSelfReference = (dependency?: IFormulaDependency, formulaAttribut
 // Function replaces all the symbol names typed by user (display names) with the symbol canonical names that
 // can be resolved by formula context and do not rely on user-based display names.
 export const displayToCanonical = (displayExpression: string, displayNameMap: DisplayNameMap) => {
-  const formulaTree = parse(customizeFormula(makeNamesSafe(displayExpression)))
+  const formulaTree = parse(preprocessFormula(displayExpression))
   const visitNode = (node: MathNode, path: string, parent: MathNode) => {
     if (isNonFunctionSymbolNode(node, parent)) {
       const canonicalName = generateCanonicalSymbolName(node.name, displayNameMap)

v3/src/models/data/formula.ts

@@ -1,33 +1,24 @@
 import { Instance, types } from "mobx-state-tree"
 import { parse } from "mathjs"
 import { typedId } from "../../utilities/js-utils"
-import { displayToCanonical, customizeFormula, isRandomFunctionPresent } from "./formula-utils"
+import { canonicalToDisplay, displayToCanonical, isRandomFunctionPresent, preprocessFormula } from "./formula-utils"
 import { getFormulaManager } from "../tiles/tile-environment"
 
 export const Formula = types.model("Formula", {
   id: types.optional(types.identifier, () => typedId("FORM")),
-  display: ""
+  display: "",
+  canonical: ""
 })
 .views(self => ({
   get formulaManager() {
     return getFormulaManager(self)
   },
-  get canonical() {
-    if (!this.valid) {
-      return ""
-    }
-    if (!this.formulaManager || !self.display) {
-      return ""
-    }
-    const displayNameMap = this.formulaManager.getDisplayNameMapForFormula(self.id)
-    return displayToCanonical(self.display, displayNameMap)
-  },
   get empty() {
     return self.display.length === 0
   },
   get syntaxError() {
     try {
-      parse(customizeFormula(self.display))
+      parse(preprocessFormula(self.display))
     } catch (error: any) {
       return error.message
     }
@@ -37,12 +28,27 @@ export const Formula = types.model("Formula", {
     return !this.empty && !this.syntaxError
   },
   get isRandomFunctionPresent() {
-    return isRandomFunctionPresent(this.canonical)
+    return isRandomFunctionPresent(self.canonical)
   },
 }))
 .actions(self => ({
   setDisplayFormula(displayFormula: string) {
     self.display = displayFormula
+    self.canonical = "" // reset canonical formula immediately, in case of errors that are handled below
+    if (self.empty || !self.valid || !self.formulaManager) {
+      return
+    }
+    const displayNameMap = self.formulaManager.getDisplayNameMapForFormula(self.id)
+    self.canonical = displayToCanonical(self.display, displayNameMap)
+  },
+  updateDisplayFormula() {
+    // This action should be called when one of the attributes is renamed. The canonical form is still valid, while
+    // display form needs to be updated. The old display form is used to preserve the user's whitespace / formatting.
+    if (self.empty || !self.valid || !self.formulaManager) {
+      return
+    }
+    const canonicalNameMap = self.formulaManager.getCanonicalNameMap(self.id)
+    self.display = canonicalToDisplay(self.canonical, self.display, canonicalNameMap)
   },
   rerandomize() {
     self.formulaManager?.recalculateFormula(self.id)