Merge pull request #2220 from concord-consortium/185581730-add-points

Graph manual points editing

cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js

@@ -40,6 +40,23 @@ function buildTable(data) {
   });
 }
 
+// Parse `transform` attributes (used for point positioning)
+function xAttributeOfTransform(matcher) {
+  return attributeOfTransform(matcher, 1);
+}
+function yAttributeOfTransform(matcher) {
+  return attributeOfTransform(matcher, 2);
+}
+function attributeOfTransform(matcher, n) {
+  return matcher
+    .invoke('attr', 'transform')
+    .then(transform => {
+      return transform.match(/translate\((-?[0-9.]+), *(-?[0-9.]+)\)/)[n];
+    })
+    .then(parseFloat);
+}
+
+
 function beforeTest(params) {
   cy.clearQAData('all');
   cy.visit(params);
@@ -80,6 +97,8 @@ context('XYPlot Tool Tile', function () {
       cy.log("Link Table");
       clueCanvas.clickToolbarButton('graph', 'link-tile-multiple');
       xyTile.linkTable("Table Data 1");
+      cy.wait(1000); // Needs a little extra time, probably due to legend resizing.
+      // Otherwise the upcoming typeInTableCell fails.
 
       cy.log("shows edit boxes on axes");
       xyTile.getEditableAxisBox("bottom", "min").should("exist");
@@ -469,27 +488,65 @@ context('XYPlot Tool Tile', function () {
       xyTile.getRemoveVariablesButtons().should("have.length", 1);
     });
 
-    it("Test points by hand", () => {
+    it('Test points by hand', () => {
       beforeTest(queryParamsMultiDataset);
-      cy.log("Add XY Plot Tile");
+      cy.log('Add XY Plot Tile');
       cy.collapseResourceTabs();
-      clueCanvas.addTile("graph");
-      xyTile.getTile().should('be.visible');
-
-      clueCanvas.clickToolbarButton("graph", "add-points-by-hand");
-      xyTile.getXAttributesLabel().should('have.length', 1).should("contain.text", "X Variable");
-      xyTile.getYAttributesLabel().should('have.length', 1).should("contain.text", "Y Variable 1");
-      xyTile.getLayerName().should('have.length', 1).should("contain.text", "Added by hand");
+      clueCanvas.addTile('graph');
+      xyTile.getTile('.primary-workspace').should('be.visible');
+      clueCanvas.toolbarButtonIsDisabled('graph', 'move-points');
+      clueCanvas.toolbarButtonIsNotSelected('graph', 'move-points');
+      clueCanvas.toolbarButtonIsDisabled('graph', 'add-points');
+      clueCanvas.toolbarButtonIsNotSelected('graph', 'add-points');
+
+      // Create manual layer
+      clueCanvas.clickToolbarButton('graph', 'add-points-by-hand');
+      clueCanvas.toolbarButtonIsDisabled('graph', 'add-points-by-hand'); // only one manual set allowed
+      clueCanvas.toolbarButtonIsEnabled('graph', 'move-points');
+      clueCanvas.toolbarButtonIsEnabled('graph', 'add-points');
+      clueCanvas.toolbarButtonIsSelected('graph', 'add-points'); // automatically turns on "add" mode
+      xyTile.getXAttributesLabel().should('have.length', 1).should('contain.text', 'X Variable');
+      xyTile.getYAttributesLabel().should('have.length', 1).should('contain.text', 'Y Variable 1');
+      xyTile.getLayerName().should('have.length', 1).should('contain.text', 'Added by hand');
       xyTile.getLayerNameInput().should('not.be.visible');
 
+      // Rename manual layer
       xyTile.getLayerNameEditButton().click();
       xyTile.getLayerNameEditButton().should('have.length', 0);
       xyTile.getLayerNameInput().should('be.visible').type('Renamed{enter}');
       xyTile.getLayerNameInput().should('not.be.visible');
-      xyTile.getLayerName().should('have.length', 1).should("contain.text", "Renamed");
-    });
+      xyTile.getLayerName().should('have.length', 1).should('contain.text', 'Renamed');
 
-  });
+      // Add points
+      xyTile.getGraphDot().should('have.length', 0);
+      xyTile.getTile('.primary-workspace').should('have.length', 1);
+      xyTile.getGraphBackground().should('have.length', 1).click(150, 50);
+      xyTile.getGraphBackground().click(200, 100);
+      xyTile.getGraphDot().should('have.length', 2);
 
+      // Switch to 'select/move' mode
+      clueCanvas.clickToolbarButton('graph', 'move-points');
+      clueCanvas.toolbarButtonIsSelected('graph', 'move-points');
+      clueCanvas.toolbarButtonIsNotSelected('graph', 'add-points');
+      xyTile.getGraphBackground().click(250, 100); // should not add a point
+      xyTile.getGraphDot().should('have.length', 2);
 
+      // Drag a point to reposition.  Should start out where we initially clicked
+      xAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 150, 10);
+      yAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 50, 10);
+      // {force: true} seems to be necessary, not sure why
+      xyTile.getGraphDot().eq(0).children('circle').eq(1)
+        .trigger("mousedown", 150, 50, { force: true })
+        .trigger("drag", 175, 75, { force: true })
+        .trigger("mouseup", 175, 75, { force: true });
+      cy.wait(1000);
+      xAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 175, 10);
+      yAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 75, 10);
+
+      // Click toolbar button again to leave edit mode
+      clueCanvas.clickToolbarButton('graph', 'move-points');
+      clueCanvas.toolbarButtonIsNotSelected('graph', 'move-points');
+      clueCanvas.toolbarButtonIsNotSelected('graph', 'add-points');
+    });
+  });
 });

cypress/support/elements/tile/TableToolTile.js

@@ -64,14 +64,12 @@ class TableToolTile{
       return cy.get('.rdg-row .rdg-cell .rdg-text-editor');
     }
     typeInTableCellXY(row, col, text) {
-      this.getTableCellXY(row, col).dblclick().then(() => {
-        this.getTableCellEdit().type(`${text}{enter}`);
-      });
+      this.getTableCellXY(row, col).dblclick();
+      this.getTableCellEdit().type(`${text}{enter}`);
     }
     typeInTableCell(i, text) {
-      this.getTableCell().eq(i).dblclick().then(() => {
-        this.getTableCellEdit().type(`${text}{enter}`);
-      });
+      this.getTableCell().eq(i).dblclick();
+      this.getTableCellEdit().type(`${text}{enter}`);
     }
     getTableCellWithColIndex(colIndex, colValue){
         return cy.get('.rdg-row').contains('.rdg-cell[aria-colindex="' + colIndex + '"]', colValue);

cypress/support/elements/tile/XYPlotToolTile.js

@@ -6,6 +6,9 @@ class XYPlotToolTile {
   getTile(workspaceClass) {
     return cy.get(`${wsClass(workspaceClass)} .canvas-area .graph-tool-tile`);
   }
+  getGraphBackground(workspaceClass) {
+    return cy.get(`${wsClass(workspaceClass)} svg [data-testid=graph-background]`);
+  }
   getXYPlotTitle(workspaceClass) {
     return cy.get(`${wsClass(workspaceClass)} .canvas-area .editable-tile-title`);
   }

docs/mst-detached-error.md

@@ -12,7 +12,7 @@ Below is a strategy for tracking down the cause and a solution for one particula
 
 Hopefully from the message you can figure out the rough area of the code where the problem is happening. If you can pin point the UI action that causes the warning to be printed then you can use the following approach.
 
-In your local code find `kEnableLivelinessChecking` located in `index.tsx` and set to be `true`. This will cause these console warnings to actually be thrown errors instead. In many cases these thrown errors will be caught and ignored so after you set this to true you probably won't see any messages in the console if you try to duplicate the problem again.
+In your local code find `kEnableLivelinessChecking` located in `initialize-app.tsx` and set to be `true`. This will cause these console warnings to actually be thrown errors instead. In many cases these thrown errors will be caught and ignored so after you set this to true you probably won't see any messages in the console if you try to duplicate the problem again.
 
 Instead of looking in the console, what you should do is use the dev tools debugger to track it down. Setup the app to just before the error would normally be printed to the console. Then in the debug panel turn on "pause on exceptions" and check the box "pause on caught exceptions". Now trigger the error. The app should pause and you should be in the debugger.
 

docs/unit-configuration.md

@@ -165,13 +165,16 @@ Not updated to common toolbar framework and does not support toolbar configurati
 - `emptyPlotIsNumeric`: boolean, default true
 - `scalePlotOnValueChange`: boolean, default true
 
-Common toolbar framework; default toolbar buttons:
+Uses the common toolbar framework. Default toolbar buttons:
 
 - `link-tile` (opens dialog to replace dataset with a new one)
+- `add-points-by-hand` (creates a dataset owned by the graph)
 - `fit-all` (rescale axes to fit all points in view)
 - `toggle-lock` (lock axes so they won't automatically rescale)
+- `move-points` (mode where points can be moved)
+- `add-points` (mode where points can be added)
 
-Also available:
+Additional buttons available not in default set:
 
 - `link-tile-multiple` (opens dialog to add an additional dataset or link variables)
 

src/clue/app-config.json

@@ -272,7 +272,10 @@
           "add-points-by-hand",
           "|",
           "fit-all",
-          "toggle-lock"
+          "toggle-lock",
+          "|",
+          "move-points",
+          "add-points"
         ]
       },
       "simulator": {

src/components/utilities/editable-label-with-button.tsx

@@ -11,21 +11,14 @@ interface IProps {
   onSubmit: (value:string) => void;
 }
 
+/**
+ * Generic component for an in-place editable label.
+ *
+ * Differs from stock <Editable> in that there's an explicit edit button rather
+ * than invoking edit mode by clicking on the label text itself.
+ */
 export const EditableLabelWithButton = observer(function EditableDataSetName({defaultValue, onSubmit}: IProps) {
 
-  function EditButton() {
-    const { isEditing, getEditButtonProps } = useEditableControls();
-    if (!isEditing) {
-      return (
-        <button aria-label="Edit name" {...getEditButtonProps()}>
-          <EditIcon/>
-        </button>
-      );
-    } else {
-      return null;
-    }
-  }
-
   return (
     <Editable
       defaultValue={defaultValue}
@@ -38,3 +31,16 @@ export const EditableLabelWithButton = observer(function EditableDataSetName({de
     </Editable>
   );
 });
+
+function EditButton() {
+  const { isEditing, getEditButtonProps } = useEditableControls();
+  if (!isEditing) {
+    return (
+      <button aria-label="Edit name" {...getEditButtonProps()}>
+        <EditIcon/>
+      </button>
+    );
+  } else {
+    return null;
+  }
+}

src/models/data/data-set.ts

@@ -221,6 +221,7 @@ export const DataSet = types.model("DataSet", {
       self.cases.push(newCase);
       beforeIndex = self.cases.length - 1;
     }
+    return newCase;
   }
 
   // `affectedAttributes` are not used in the function, but are present as a potential
@@ -689,14 +690,16 @@ export const DataSet = types.model("DataSet", {
       },
 
       addCanonicalCasesWithIDs(cases: ICase[], beforeID?: string | string[]) {
+        const newCases: ICase[] = [];
         cases.forEach((aCase, index) => {
           const beforeIndex = beforeIndexForInsert(index, beforeID);
           self.attributes.forEach((attr: IAttribute) => {
             const value = aCase[attr.id];
             attr.addValue(value != null ? value : undefined, beforeIndex);
           });
-          insertCaseIDAtIndex(aCase.__id__, beforeIndex);
+          newCases.push(insertCaseIDAtIndex(aCase.__id__, beforeIndex));
         });
+        return newCases;
       },
 
       setCaseValues(cases: ICase[], affectedAttributes?: string[]) {
@@ -904,7 +907,7 @@ export function addCanonicalCasesToDataSet(dataset: IDataSet, cases: ICaseCreati
       aCase.__id__ = newCaseId();
     }
   });
-  dataset.addCanonicalCasesWithIDs(newCases, beforeID);
+  return dataset.addCanonicalCasesWithIDs(newCases, beforeID);
 }
 
 export function getDataSetBounds(dataSet: IDataSet) {

src/models/document/document-content-types.ts

@@ -1,5 +1,4 @@
 import { IArrowAnnotation } from "../annotations/arrow-annotation";
-import { SharedModelSnapshotType } from "../shared/shared-model";
 import { IDragSharedModelItem } from "../shared/shared-model-manager";
 import { IDragTileItem } from "../tiles/tile-model";
 import { IDropRowInfo } from "./tile-row";
@@ -46,10 +45,3 @@ export interface IDragTilesData {
   annotations: IArrowAnnotation[];
 }
 
-export interface PartialTile {
-  id: string;
-}
-export interface PartialSharedModelEntry {
-  sharedModel: SharedModelSnapshotType;
-  tiles: PartialTile[];
-}

src/models/document/document-content.ts

@@ -1,7 +1,7 @@
 import stringify from "json-stringify-pretty-compact";
 import { applySnapshot, getSnapshot, Instance, SnapshotIn } from "mobx-state-tree";
 import { cloneDeep, each } from "lodash";
-import { IDragTilesData, PartialSharedModelEntry, PartialTile,
+import { IDragTilesData,
          IDocumentContentAddTileOptions } from "./document-content-types";
 import { DocumentContentModelWithTileDragging } from "./drag-tiles";
 import { IDropRowInfo, TileRowModelType, TileRowSnapshotOutType } from "./tile-row";
@@ -14,6 +14,7 @@ import { getTileContentInfo, IDocumentExportOptions } from "../tiles/tile-conten
 import { IDragTileItem, IDropTileItem, ITileModel, ITileModelSnapshotOut } from "../tiles/tile-model";
 import { uniqueId } from "../../utilities/js-utils";
 import { comma, StringBuilder } from "../../utilities/string-builder";
+import { SharedModelEntrySnapshotType } from "./shared-model-entry";
 
 // Imports related to hard coding shared model duplication
 import {
@@ -231,13 +232,13 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
   // Copies tiles and shared models into the specified row, giving them all new ids
   copyTiles(
     tiles: IDragTileItem[],
-    sharedModelEntries: PartialSharedModelEntry[],
+    sharedModelEntries: SharedModelEntrySnapshotType[],
     annotations: IArrowAnnotationSnapshot[],
     rowInfo: IDropRowInfo
   ) {
     // Update shared models with new names and ids
     const updatedSharedModelMap: Record<string, UpdatedSharedDataSetIds> = {};
-    const newSharedModelEntries: PartialSharedModelEntry[] = [];
+    const newSharedModelEntries: SharedModelEntrySnapshotType[] = [];
     sharedModelEntries.forEach(sharedModelEntry => {
       // For now, only duplicate shared data sets
       if (isSharedDataSetSnapshot(sharedModelEntry.sharedModel)) {
@@ -266,7 +267,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
     });
 
     const findTileSharedModelEntries = (tileId: string) => {
-      return sharedModelEntries.filter(entry => entry.tiles?.map(t => t.id).includes(tileId));
+      return sharedModelEntries.filter(entry => entry.tiles?.includes(tileId));
     };
 
     // Update tile content with new shared model ids
@@ -302,7 +303,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
     // Update tile ids for shared models and add those references to document.
     // The shared datasets have already been added above.
     newSharedModelEntries.forEach(sharedModelEntry => {
-      const updatedTileIds: string[] = sharedModelEntry.tiles.map((oldTile: PartialTile) => tileIdMap[oldTile.id])
+      const updatedTileIds: string[] = (sharedModelEntry.tiles||[]).map((oldTile) => tileIdMap[oldTile])
         .filter((tileId: string | undefined) => tileId !== undefined);
       if (isSharedDataSetSnapshot(sharedModelEntry.sharedModel)) {
         const oldProvider = sharedModelEntry.sharedModel.providerId;
@@ -332,7 +333,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
     });
 
     // Update tile and object ids for annotations and add copies to document
-    const updateObject = (object?: IClueObjectSnapshot) => {
+    const updateAnnotationObject = (object?: IClueObjectSnapshot) => {
       if (object) {
         const tile = tiles.find(t => t.tileId === object?.tileId);
         if (tile) {
@@ -344,11 +345,12 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
         }
       }
     };
+
     annotations.forEach(annotation => {
       if (isArrowAnnotationSnapshot(annotation)) {
         const newAnnotationSnapshot = cloneDeep(annotation);
-        updateObject(newAnnotationSnapshot.sourceObject);
-        updateObject(newAnnotationSnapshot.targetObject);
+        updateAnnotationObject(newAnnotationSnapshot.sourceObject);
+        updateAnnotationObject(newAnnotationSnapshot.targetObject);
         updateArrowAnnotationTileIds(newAnnotationSnapshot, tileIdMap);
         newAnnotationSnapshot.id = uniqueId();
         self.addArrow(ArrowAnnotation.create(newAnnotationSnapshot));
@@ -361,7 +363,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
     const { tiles, sharedModels, annotations } = dragTiles;
 
     // Convert IDragSharedModelItems to partial SharedModelEnries
-    const sharedModelEntries: PartialSharedModelEntry[] = [];
+    const sharedModelEntries: SharedModelEntrySnapshotType[] = [];
     sharedModels.forEach(dragSharedModel => {
       try {
         const content = JSON.parse(dragSharedModel.content);
@@ -370,7 +372,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
         if (sharedModel) {
           sharedModelEntries.push({
             sharedModel,
-            tiles: dragSharedModel.tileIds.map(tileId => ({ id: tileId }))
+            tiles: dragSharedModel.tileIds
           });
         }
       } catch (e) {
@@ -389,9 +391,14 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named("
     const sharedModelEntries = Object.values(self.getSharedModelsUsedByTiles(tileIds));
     const annotations = Object.values(self.getAnnotationsUsedByTiles(tileIds, true));
 
+    // Make sure we are only passing snapshots to copyTiles, since they need to be cloned & modified.
+    const snapshots = sharedModelEntries.map(sme => {
+      return getSnapshot(sme);
+    });
+
     self.copyTiles(
       tiles,
-      sharedModelEntries,
+      snapshots,
       annotations,
       { rowInsertIndex: rowIndex }
     );