(core) simplify document comparison code, and flesh out diff with local changes

Summary:
With recent changes to action history, we can now remove the temporary
`finalRowContent` field from change details, since all the information
we need is now in the ActionSummary.

We also now have more information about the state of the common ancestor,
which previously we could not get either from ActionSummary or from
`finalRowContent`. We take advantage of that to flesh out rendering
differences where there are some changes locally and some changes
remotely.

There's still a lot more to do, this is just one step.

I have added a link to the UI for viewing the comparison. I wouldn't
want to advertise that link until diffs are robust to name changes.

Test Plan: added test, updated tests

Reviewers: dsagal

Reviewed By: dsagal

Differential Revision: https://phab.getgrist.com/D2658

app/client/components/BaseView.js

@@ -24,6 +24,7 @@ const {urlState} = require('app/client/models/gristUrlState');
 const {SectionFilter} = require('app/client/models/SectionFilter');
 const {copyToClipboard} = require('app/client/lib/copyToClipboard');
 const {setTestState} = require('app/client/lib/testState');
+const {ExtraRows} = require('app/client/models/DataTableModelWithDiff');
 const {createFilterMenu} = require('app/client/ui/ColumnFilterMenu');
 
 /**
@@ -47,18 +48,10 @@ function BaseView(gristDoc, viewSectionModel, options) {
 
   // Check if we are making a comparison with another document.
   this.comparison = this.gristDoc.comparison;
-  if (this.comparison) {
-    const tableId = this.schemaModel.tableId();
-    // TODO: make robust to name changes.
-    this.leftTableDelta = this.comparison.details.leftChanges.tableDeltas[tableId];
-    this.rightTableDelta = this.comparison.details.rightChanges.tableDeltas[tableId];
-  } else {
-    this.rightTableDelta = null;
-    this.leftTableDelta = null;
-  }
 
   // TODO: but accessing by tableId identifier may be problematic when the table is renamed.
   this.tableModel = this.gristDoc.getTableModelMaybeWithDiff(this.schemaModel.tableId());
+  this.extraRows = new ExtraRows(this.schemaModel.tableId(), this.comparison && this.comparison.details);
 
   // We use a DynamicQuerySet as the underlying RowSource, with ColumnFilters applies on top of
   // it. It filters based on section linking, re-querying as needed in case of onDemand tables.
@@ -76,13 +69,9 @@ function BaseView(gristDoc, viewSectionModel, options) {
   }
 
   if (this.comparison) {
-    // Assign extra row ids for any rows added in the remote (right) table.
+    // Assign extra row ids for any rows added in the remote (right) table or removed in the
-    // We flip their sign to make them as belonging to the remote table only.
+    // local (left) table.
-    // TODO: if we wanted to show rows removed in the local (left) table, we'd need to
+    const extraRowIds = this.extraRows.getExtraRows();
-    // add those too, and come up with ids to give them.  Without this, there's no
-    // way to render an update that was made remotely to a row that was removed locally.
-    const extraRowIds = (this.rightTableDelta && this.rightTableDelta.addRows || [])
-          .map(rowId => -rowId);
     this._mainRowSource = rowset.ExtendedRowSource.create(this, this._mainRowSource, extraRowIds);
   }
 

app/client/components/GridView.js

@@ -704,10 +704,6 @@ GridView.prototype.buildDom = function() {
   let vVerticalGridlines   = v.optionsObj.prop('verticalGridlines');
   let vZebraStripes        = v.optionsObj.prop('zebraStripes');
 
-  const rightAddRows = new Set(this.rightTableDelta && this.rightTableDelta.addRows.map(id => -id));
-  const rightRemoveRows = new Set(this.rightTableDelta && this.rightTableDelta.removeRows);
-  const leftAddRows = new Set(this.leftTableDelta && this.leftTableDelta.addRows);
-
   var renameCommands = {
     nextField: function() {
       editIndex(editIndex() + 1);
@@ -908,11 +904,8 @@ GridView.prototype.buildDom = function() {
         kd.toggleClass('record-even', () => (row._index()+1) % 2 === 0 ),
 
         self.comparison ? kd.cssClass(() => {
-          var rowId = row.id();
+          const rowType = self.extraRows.getRowType(row.id());
-          if (rightAddRows.has(rowId))         { return 'diff-remote'; }
+          return rowType && `diff-${rowType}` || '';
-          else if (rightRemoveRows.has(rowId)) { return 'diff-parent'; }
-          else if (leftAddRows.has(rowId))     { return 'diff-local';  }
-          return '';
         }) : null,
 
         kd.foreach(v.viewFields(), function(field) {

app/client/components/viewCommon.css

@@ -152,26 +152,21 @@
   white-space: pre-wrap;
 }
 
-.diff-change .diff-parent, .diff-change .diff-remote {
+.diff-local, .diff-local-add {
-  display: inline-block;
-  width: 50%;
-}
-
-.diff-conflict .diff-parent, .diff-conflict .diff-local, .diff-conflict .diff-remote {
-  display: inline-block;
-  width: 33.33%;
-}
-
-.diff-local {
   background-color: #dfdfff;
 }
 
-.diff-parent {
+.diff-parent, .diff-remote-remove {
   background-color: #ffdfdf;
   text-decoration: line-through;
 }
 
-.diff-remote {
+.diff-local-remove {
+  background-color: #dfdfdf;
+  text-decoration: line-through;
+}
+
+.diff-remote, .diff-remote-add {
   background-color: #afffaf;
 }
 

app/client/models/DataTableModelWithDiff.ts

@@ -13,6 +13,61 @@ import { CellDelta } from 'app/common/TabularDiff';
 import { DocStateComparisonDetails } from 'app/common/UserAPI';
 import { CellValue } from 'app/plugin/GristData';
 
+/**
+ * Represent extra rows in a table that correspond to rows added in a remote (right) document,
+ * or removed in the local (left) document relative to a common ancestor.
+ *
+ * We assign synthetic row ids for these rows somewhat arbitrarily as follows:
+ *  - For rows added remotely, we double their id and flip sign
+ *  - For rows removed locally, we double their id, add one, and flip sign
+ * This should be the only part of the code that knows that.
+ */
+export class ExtraRows {
+  readonly leftTableDelta?: TableDelta;
+  readonly rightTableDelta?: TableDelta;
+  readonly rightAddRows: Set<number>;
+  readonly rightRemoveRows: Set<number>;
+  readonly leftAddRows: Set<number>;
+  readonly leftRemoveRows: Set<number>;
+
+  /**
+   * Map back from a possibly synthetic row id to an original strictly-positive row id.
+   */
+  public static interpretRowId(rowId: number): { type: 'remote-add'|'local-remove'|'shared', id: number } {
+    if (rowId >= 0) { return { type: 'shared', id: rowId }; }
+    if (rowId % 2 === 0) { return { type: 'remote-add', id: -rowId / 2 }; }
+    return { type: 'local-remove', id: (-rowId - 1) / 2 };
+  }
+
+  public constructor(readonly tableId: string, readonly comparison?: DocStateComparisonDetails) {
+    this.leftTableDelta = this.comparison?.leftChanges?.tableDeltas[this.tableId];
+    this.rightTableDelta = this.comparison?.rightChanges?.tableDeltas[this.tableId];
+    this.rightAddRows = new Set(this.rightTableDelta && this.rightTableDelta.addRows.map(id => -id*2));
+    this.rightRemoveRows = new Set(this.rightTableDelta && this.rightTableDelta.removeRows);
+    this.leftAddRows = new Set(this.leftTableDelta && this.leftTableDelta.addRows);
+    this.leftRemoveRows = new Set(this.leftTableDelta && this.leftTableDelta.removeRows.map(id => -id*2 -1));
+  }
+
+  /**
+   * Get a list of extra synthetic row ids to add.
+   */
+  public getExtraRows(): ReadonlyArray<number> {
+    return [...this.rightAddRows].concat([...this.leftRemoveRows]);
+  }
+
+  /**
+   * Classify the row as either remote-add, remote-remove, local-add, or local-remove.
+   */
+  public getRowType(rowId: number) {
+    if (this.rightAddRows.has(rowId))         { return 'remote-add'; }
+    else if (this.leftAddRows.has(rowId))     { return 'local-add';  }
+    else if (this.rightRemoveRows.has(rowId)) { return 'remote-remove'; }
+    else if (this.leftRemoveRows.has(rowId))  { return 'local-remove'; }
+    // TODO: consider what should happen when a row is removed both locally and remotely.
+    return '';
+  }
+}
+
 /**
  *
  * A variant of DataTableModel that is aware of a comparison with another version of the table.
@@ -154,25 +209,20 @@ export class TableDataWithDiff {
           parent: oldValue(left),
           local: newValue(left)
         } as CellVersions];
-      } else {
-        // No change in ActionSummary for this cell, but it could be a formula
-        // column.  So we do a crude comparison between the values available.
-        // We won't be able to do anything useful for conflicts (e.g. to know
-        // the display text in a reference columnn for the common parent).
-        // We also won't be able to detect local changes at all.
-        const parent = this.core.getValue(rowId, colId);
-        const remote = this.rightTableDelta.finalRowContent?.[rowId]?.[colId];
-        if (remote !== undefined && JSON.stringify(remote) !== JSON.stringify(parent)) {
-          return [GristObjCode.Versions, {parent, remote} as CellVersions];
-        }
-        return parent;
       }
-    }
+    } else {
-    if (rowId < 0) {
-      const value = this.rightTableDelta.finalRowContent?.[-rowId]?.[colId];
       // keep row.id consistent with rowId for convenience.
-      if (colId === 'id') { return - (value as number); }
+      if (colId === 'id') { return rowId; }
-      return value;
+      const {type, id} = ExtraRows.interpretRowId(rowId);
+      if (type === 'remote-add') {
+        const cell = this.rightTableDelta.columnDeltas[colId]?.[id];
+        const value = (cell !== undefined) ? newValue(cell) : undefined;
+        return value;
+      } else if (type === 'local-remove') {
+        const cell = this.leftTableDelta.columnDeltas[colId]?.[id];
+        const value = (cell !== undefined) ? oldValue(cell) : undefined;
+        return value;
+      }
     }
     return this.core.getValue(rowId, colId);
   }

app/client/ui/ShareMenu.ts

@@ -136,6 +136,9 @@ function menuOriginal(doc: Document, appModel: AppModel, isSnapshot: boolean) {
   const termToUse = isSnapshot ? "Current Version" : "Original";
   const origUrlId = buildOriginalUrlId(doc.id, isSnapshot);
   const originalUrl = urlState().makeUrl({doc: origUrlId});
+  const originalUrlComparison = urlState().makeUrl({
+    doc: origUrlId, params: { compare: doc.id }
+  });
   function replaceOriginal() {
     const user = appModel.currentValidUser;
     replaceTrunkWithFork(user, doc, appModel, origUrlId).catch(reportError);
@@ -151,6 +154,9 @@ function menuOriginal(doc: Document, appModel: AppModel, isSnapshot: boolean) {
       dom.cls('disabled', !roles.canEdit(doc.trunkAccess || null)),
       testId('replace-original'),
     ),
+    menuItemLink({href: originalUrlComparison, target: '_blank'}, `Compare to ${termToUse}`,
+      testId('compare-original'),
+    ),
   ];
 }
 

app/client/widgets/DiffBox.ts

@@ -74,23 +74,25 @@ export class DiffBox extends NewAbstractWidget {
       // selected on the basis of one column, but we are displaying the
       // content of another.  We have more version information for the
       // reference column than for its display column.
-      return [[DIFF_EQUAL, formatter.format(value)]];
+      return [[DIFF_EQUAL, formatter.formatAny(value)]];
     }
     const versions = value[1];
     if (!('local' in versions)) {
       // Change was made remotely only.
-      return this._prepareTextDiff(formatter.format(versions.parent),
+      return this._prepareTextDiff(
-                                   formatter.format(versions.remote));
+        formatter.formatAny(versions.parent),
+        formatter.formatAny(versions.remote));
     } else if (!('remote' in versions)) {
       // Change was made locally only.
-      return this._prepareTextDiff(formatter.format(versions.parent),
+      return this._prepareTextDiff(
-                                   formatter.format(versions.local))
+        formatter.formatAny(versions.parent),
+        formatter.formatAny(versions.local))
         .map(([code, txt]) => [code === DIFF_INSERT ? DIFF_LOCAL : code, txt]);
     }
     // Change was made both locally and remotely.
-    return [[DIFF_DELETE, formatter.format(versions.parent)],
+    return [[DIFF_DELETE, formatter.formatAny(versions.parent)],
-            [DIFF_LOCAL, formatter.format(versions.local)],
+            [DIFF_LOCAL, formatter.formatAny(versions.local)],
-            [DIFF_INSERT, formatter.format(versions.remote)]];
+            [DIFF_INSERT, formatter.formatAny(versions.remote)]];
   }
 
   // Run diff-match-patch on the text, do its cleanup, and then some extra

app/common/ActionSummary.ts

@@ -1,6 +1,5 @@
 import {CellDelta, TabularDiff, TabularDiffs} from 'app/common/TabularDiff';
 import toPairs = require('lodash/toPairs');
-import { RowRecord } from 'app/plugin/GristData';
 
 /**
  * An ActionSummary represents the overall effect of changes that took place
@@ -91,13 +90,6 @@ export interface TableDelta {
   /** Partial record of cell-level changes - large bulk changes not included. */
   columnDeltas: {[colId: string]: ColumnDelta};
   columnRenames: LabelDelta[];  /** a list of column renames/additions/removals */
-
-  /*
-   * A snapshot of row content for added and updated rows, including formula columns.
-   * Not included in regular summaries, but used in comparisons.  Hopefully this
-   * can evaporate in future.
-   */
-  finalRowContent?: {[rowId: number]: RowRecord};
 }
 
 /**

app/server/lib/DocApi.ts

@@ -1,7 +1,7 @@
-import { ActionSummary, createEmptyActionSummary } from "app/common/ActionSummary";
+import { createEmptyActionSummary } from "app/common/ActionSummary";
 import { ApiError } from 'app/common/ApiError';
 import { BrowserSettings } from "app/common/BrowserSettings";
-import { fromTableDataAction, RowRecord, TableColValues } from 'app/common/DocActions';
+import { fromTableDataAction, TableColValues } from 'app/common/DocActions';
 import { arrayRepeat, isAffirmative } from "app/common/gutil";
 import { SortFunc } from 'app/common/SortFunc';
 import { DocReplacementOptions, DocState, DocStateComparison, DocStates, NEW_DOCUMENT_CODE} from 'app/common/UserAPI';
@@ -18,7 +18,6 @@ import { expressWrap } from 'app/server/lib/expressWrap';
 import { GristServer } from 'app/server/lib/GristServer';
 import { HashUtil } from 'app/server/lib/HashUtil';
 import { makeForkIds } from "app/server/lib/idUtils";
-import * as log from 'app/server/lib/log';
 import { getDocId, getDocScope, integerParam, isParameterOn, optStringParam,
   sendOkReply, sendReply, stringParam } from 'app/server/lib/requestUtils';
 import { SandboxError } from "app/server/lib/sandboxUtil";
@@ -532,58 +531,9 @@ export class DocWorkerApi {
         rightChanges: totalAction
       }
     };
-    // Currently, as a bit of a hack, the full final state of updated/added rows
-    // is included, including formula columns, by looking at the current state
-    // of the document.
-    if (rightOffset === 0) {
-      await this._addRowsToActionSummary(docSession, activeDoc, totalAction);
-    } else {
-      // In the future final row content may not be needed, if formula cells end
-      // up included in ActionSummary.
-      log.debug('cannot add rows when not comparing to current state of doc');
-    }
     return result;
   }
 
-  /**
-   * Adds the content of updated and added rows to an ActionSummary.
-   * For visualizing differences, currently there's no other way to get formula
-   * information.  This only makes sense for an ActionSummary between a previous
-   * version of the document and the current version, since it accesses the row
-   * content from the current version of the document.
-   */
-  private async _addRowsToActionSummary(docSession: OptDocSession, activeDoc: ActiveDoc,
-                                        summary: ActionSummary) {
-    for (const tableId of Object.keys(summary.tableDeltas)) {
-      const tableDelta = summary.tableDeltas[tableId];
-      const rowIds = new Set([...tableDelta.addRows, ...tableDelta.updateRows]);
-      try {
-        // Inefficient code that reads the entire table in order to pull out the few
-        // rows we need.
-        const [, , ids, columns] = await handleSandboxError(tableId, [], activeDoc.fetchQuery(
-          docSession, {tableId, filters: {}}, true));
-        const rows: {[key: number]: RowRecord} = {};
-        for (const rowId of rowIds) {
-          const rec: RowRecord = {id: rowId};
-          const idx = ids.indexOf(rowId);
-          if (idx >= 0) {
-            for (const colId of Object.keys(columns)) {
-              rec[colId] = columns[colId][idx];
-            }
-            rows[rowId] = rec;
-          }
-        }
-        tableDelta.finalRowContent = rows;
-      } catch (e) {
-        // ActionSummary has some rough spots - if there's some junk in it we just ignore
-        // that for now.
-        // TODO: add ids to doc actions and their undos so they can be aligned, so ActionSummary
-        // doesn't need to use heuristics.
-        log.error('_addRowsToChanges skipped a table');
-      }
-    }
-  }
-
   private async _removeDoc(req: Request, res: Response, permanent: boolean) {
     const scope = getDocScope(req);
     const docId = getDocId(req);