(core) hide long sequences of unchanged rows in diffs

Summary:
It can be hard to find changes, even when highlighted, in a table with many rows.  This diff replaces long sequences of unchanged rows with a row containing "..."s.

With daff, I found that it is important to do this for sequences of unchanged columns also, but not tackling that yet.

Test Plan: added test

Reviewers: dsagal

Reviewed By: dsagal

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

app/client/components/BaseView.js

@@ -86,10 +86,10 @@ function BaseView(gristDoc, viewSectionModel, options) {
   }));
 
   // Sorted collection of all rows to show in this view.
-  this.sortedRows = rowset.SortedRowSet.create(this, null);
+  this.sortedRows = rowset.SortedRowSet.create(this, null, this.tableModel.tableData);
 
   // Re-sort when sortSpec changes.
-  this.sortFunc = new SortFunc(new ClientColumnGetters(this.tableModel));
+  this.sortFunc = new SortFunc(new ClientColumnGetters(this.tableModel, {unversioned: true}));
   this.autoDispose(this.viewSection.activeDisplaySortSpec.subscribeInit(function(spec) {
     this.sortFunc.updateSpec(spec);
     this.sortedRows.updateSort((rowId1, rowId2) => {

app/client/components/viewCommon.css

@@ -75,6 +75,11 @@
   background-color: unset;
 }
 
+.field_clip.field-error-S {
+  color: #aaa;
+  background-color: unset;
+}
+
 /* Insert a zero-width space into each cell, to size cells to at least one line of text. */
 .field_clip:empty::before { content: '\200B'; }
 

app/client/lib/sortUtil.ts

@@ -72,10 +72,10 @@ export async function updatePositions(gristDoc: GristDoc, section: ViewSectionRe
 
   // Build a sorted array of rowIds the way a view would, using the active sort spec. We just need
   // the sorted list, and can dispose the observable array immediately.
-  const sortFunc = new SortFunc(new ClientColumnGetters(tableModel));
+  const sortFunc = new SortFunc(new ClientColumnGetters(tableModel, {unversioned: true}));
   sortFunc.updateSpec(section.activeDisplaySortSpec.peek());
   const sortedRows = rowset.SortedRowSet.create(null, (a: rowset.RowId, b: rowset.RowId) =>
-    sortFunc.compare(a as number, b as number));
+    sortFunc.compare(a as number, b as number), tableModel.tableData);
   sortedRows.subscribeTo(tableModel);
   const sortedRowIds = sortedRows.getKoArray().peek().slice(0);
   sortedRows.dispose();

app/client/models/ClientColumnGetters.ts

@@ -1,3 +1,4 @@
+import * as DataTableModel from 'app/client/models/DataTableModel';
 import {ColumnGetters} from 'app/common/ColumnGetters';
 import * as gristTypes from 'app/common/gristTypes';
 
@@ -9,12 +10,30 @@ import * as gristTypes from 'app/common/gristTypes';
  */
 export class ClientColumnGetters implements ColumnGetters {
 
-  constructor(private _tableModel: any) {
+  // If the "unversioned" option is set, then cells with multiple
+  // versions will be read as a single version - the first version
+  // available of parent, local, or remote.  This can make sense for
+  // sorting, so cells appear in a reasonably sensible place.
+  constructor(private _tableModel: DataTableModel, private _options: {
+    unversioned?: boolean} = {}) {
   }
 
   public getColGetter(colRef: number): ((rowId: number) => any) | null {
     const colId = this._tableModel.docModel.columns.getRowModel(Math.abs(colRef)).colId();
-    return this._tableModel.tableData.getRowPropFunc(colId);
+    const getter = this._tableModel.tableData.getRowPropFunc(colId);
+    if (!getter) { return getter || null; }
+    if (this._options.unversioned && this._tableModel.tableData.mayHaveVersions()) {
+      return (rowId) => {
+        const value = getter(rowId);
+        if (value && gristTypes.isVersions(value)) {
+          const versions = value[1];
+          return ('parent' in versions) ? versions.parent :
+            ('local' in versions) ? versions.local : versions.remote;
+        }
+        return value;
+      };
+    }
+    return getter;
   }
 
   public getManualSortGetter(): ((rowId: number) => any) | null {

app/client/models/DataTableModelWithDiff.ts

@@ -13,13 +13,17 @@ import { CellDelta } from 'app/common/TabularDiff';
 import { DocStateComparisonDetails } from 'app/common/UserAPI';
 import { CellValue } from 'app/plugin/GristData';
 
+// A special row id, representing omitted rows.
+const ROW_ID_SKIP = -1;
+
 /**
  * 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
+ *  - For rows added remotely, we map their id to - id * 2 - 1
+ *  - For rows removed locally, we map their id to - id * 2 - 2
+ *  - (id of -1 is left free for use in skipped rows)
  * This should be the only part of the code that knows that.
  */
 export class ExtraRows {
@@ -33,10 +37,11 @@ export class ExtraRows {
   /**
    * 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 } {
+  public static interpretRowId(rowId: number): { type: 'remote-add'|'local-remove'|'shared'|'skipped', 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 };
+    else if (rowId === ROW_ID_SKIP) { return { type: 'skipped', id: rowId }; }
+    else if (rowId % 2 !== 0) { return { type: 'remote-add', id: -(rowId + 1) / 2 }; }
+    return { type: 'local-remove', id: -(rowId + 2) / 2 };
   }
 
   public constructor(readonly tableId: string, readonly comparison?: DocStateComparisonDetails) {
@@ -45,10 +50,10 @@ export class ExtraRows {
     if (remoteTableId) {
       this.rightTableDelta = this.comparison?.rightChanges?.tableDeltas[remoteTableId];
     }
-    this.rightAddRows = new Set(this.rightTableDelta?.addRows.map(id => -id*2));
+    this.rightAddRows = new Set(this.rightTableDelta?.addRows.map(id => -id * 2 - 1));
     this.rightRemoveRows = new Set(this.rightTableDelta?.removeRows);
     this.leftAddRows = new Set(this.leftTableDelta?.addRows);
-    this.leftRemoveRows = new Set(this.leftTableDelta?.removeRows.map(id => -id*2 -1));
+    this.leftRemoveRows = new Set(this.leftTableDelta?.removeRows.map(id => -id * 2 - 2));
   }
 
   /**
@@ -184,17 +189,34 @@ export class TableDataWithDiff {
     const fn = this.core.getRowPropFunc(colId);
     if (!fn) { return fn; }
     return (rowId: number|"new") => {
-      if (rowId !== 'new' && this._updates.has(rowId)) {
+      if (rowId !== 'new' && (rowId < 0 || this._updates.has(rowId))) {
         return this.getValue(rowId, colId);
       }
-      return (rowId !== 'new' && rowId < 0) ? this.getValue(rowId, colId) : fn(rowId);
+      return fn(rowId);
     };
   }
 
+  public getKeepFunc(): undefined | ((rowId: number|"new") => boolean) {
+    return (rowId: number|'new') => {
+      return rowId === 'new' || this._updates.has(rowId) || rowId < 0;
+    };
+  }
+
+  public getSkipRowId(): number {
+    return ROW_ID_SKIP;
+  }
+
+  public mayHaveVersions() {
+    return true;
+  }
+
   /**
    * Intercept requests for updated cells or cells from remote rows.
    */
   public getValue(rowId: number, colId: string): CellValue|undefined {
+    if (rowId === ROW_ID_SKIP && colId !== 'id') {
+      return [GristObjCode.Skip];
+    }
     if (this._updates.has(rowId)) {
       const left = this.leftTableDelta.columnDeltas[colId]?.[rowId];
       const right = this.rightTableDelta.columnDeltas[colId]?.[rowId];

app/client/models/rowset.ts

@@ -24,6 +24,7 @@
 import koArray, {KoArray} from 'app/client/lib/koArray';
 import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
 import {CompareFunc, sortedIndex} from 'app/common/gutil';
+import {SkippableRows} from 'app/common/TableData';
 
 /**
  * Special constant value that can be used for the `rows` array for the 'rowNotify'
@@ -33,7 +34,7 @@ export const ALL: unique symbol = Symbol("ALL");
 
 export type ChangeType = 'add' | 'remove' | 'update';
 export type ChangeMethod = 'onAddRows' | 'onRemoveRows' | 'onUpdateRows';
-export type RowId = number | string;
+export type RowId = number | 'new';
 export type RowList = Iterable<RowId>;
 export type RowsChanged = RowList | typeof ALL;
 
@@ -514,10 +515,13 @@ export class SortedRowSet extends RowListener {
   private _allRows: Set<RowId> = new Set();
   private _isPaused: boolean = false;
   private _koArray: KoArray<RowId>;
+  private _keepFunc?: (rowId: number|'new') => boolean;
 
-  constructor(private _compareFunc: CompareFunc<RowId>) {
+  constructor(private _compareFunc: CompareFunc<RowId>,
+              private _skippableRows?: SkippableRows) {
     super();
     this._koArray = this.autoDispose(koArray<RowId>());
+    this._keepFunc = _skippableRows?.getKeepFunc();
   }
 
   /**
@@ -557,13 +561,13 @@ export class SortedRowSet extends RowListener {
     if (this._isPaused) {
       return;
     }
-    if (isSmallChange(rows)) {
+    if (this._canChangeIncrementally(rows)) {
       for (const r of rows) {
         const insertIndex = sortedIndex(this._koArray.peek(), r, this._compareFunc);
         this._koArray.splice(insertIndex, 0, r);
       }
     } else {
-      this._koArray.assign(Array.from(this._allRows).sort(this._compareFunc));
+      this._koArray.assign(this._keep(Array.from(this._allRows).sort(this._compareFunc)));
     }
   }
 
@@ -574,7 +578,7 @@ export class SortedRowSet extends RowListener {
     if (this._isPaused) {
       return;
     }
-    if (isSmallChange(rows)) {
+    if (this._canChangeIncrementally(rows)) {
       for (const r of rows) {
         const index = this._koArray.peek().indexOf(r);
         if (index !== -1) {
@@ -582,7 +586,7 @@ export class SortedRowSet extends RowListener {
         }
       }
     } else {
-      this._koArray.assign(Array.from(this._allRows).sort(this._compareFunc));
+      this._koArray.assign(this._keep(Array.from(this._allRows).sort(this._compareFunc)));
     }
   }
 
@@ -603,15 +607,77 @@ export class SortedRowSet extends RowListener {
       return;
     }
 
-    if (isSmallChange(rows)) {
+    if (this._canChangeIncrementally(rows)) {
       // Note that we can't add any rows before we remove all affected rows, because affected rows
       // may no longer be in the correct sort order, so binary search is broken until they are gone.
       this.onRemoveRows(rows);
       this.onAddRows(rows);
     } else {
-      this._koArray.assign(Array.from(this._koArray.peek()).sort(this._compareFunc));
+      this._koArray.assign(this._keep(Array.from(this._koArray.peek()).sort(this._compareFunc)));
     }
   }
+
+  // Check whether a change in the specified rows can be applied incrementally.
+  private _canChangeIncrementally(rows: RowList) {
+    return !this._keepFunc && isSmallChange(rows);
+  }
+
+  // Filter out any rows that should be skipped. This is a no-op if no _keepFunc was found.
+  // All rows that sort within nContext rows of something meant to be kept are also kept.
+  private _keep(rows: RowId[], nContext: number = 2) {
+    // Nothing to be done if there's no _keepFunc.
+    if (!this._keepFunc) { return rows; }
+
+    // Seed a list of rows to be kept (we'll expand it as we go).
+    const keeping = rows.map(this._keepFunc);
+
+    // Within a range of skipped rows, we'll keep one as an interstitial, with its
+    // rowId replaced with a special "skip" id that makes it get rendered a special
+    // way (with "..." in every cell).
+    // Start with a blank list (we'll fill it out as we go).
+    const edge = rows.map(() => false);
+
+    // Keep the first and last (typically 'new') row.
+    const n = rows.length;
+    if (n >= 1) { keeping[0] = true; }
+    if (n >= 2) { keeping[n - 1] = true; }
+
+    // Sweep forwards through the list of kept rows, keeping an extra nContext rows
+    // after each.
+    let last = - nContext - 1;
+    for (let i = 0; i < n; i++) {
+      if (keeping[i]) { last = i; }
+      else if (i - last <= nContext) { keeping[i] = true; }
+    }
+
+    // Sweep backwards through the list of kept rows, keeping an extra nContext rows
+    // before each.
+    last = n + nContext + 1;
+    for (let i = n - 1; i >= 0; i--) {
+      if (keeping[i]) { last = i; }
+      else if (last - i <= nContext) { keeping[i] = true; }
+    }
+
+    // Keep one extra "edge" row from each sequence of rows that are to be skipped.
+    let skipping: boolean = false;
+    for (let i = 0; i < n; i++) {
+      if (keeping[i]) {
+        skipping = false;
+      } else {
+        if (!skipping) {
+          edge[i] = true;
+          skipping = true;
+        }
+      }
+    }
+
+    // Go ahead and filter out the rows to keep, tweaking the row id of the
+    // "edge" rows.
+    const skipRowId = this._skippableRows?.getSkipRowId() || 0;
+    return rows
+      .map((v, i) => edge[i] ? skipRowId : v)
+      .filter((v, i) => keeping[i] || edge[i] || v === 'new');
+  }
 }
 
 function isSmallChange(rows: RowList) {

app/common/DocActions.ts

@@ -7,10 +7,12 @@ import { CellValue } from 'app/plugin/GristData';
 export { CellValue, RowRecord } from 'app/plugin/GristData';
 
 // Part of a special CellValue used for comparisons, embedding several versions of a CellValue.
-export type CellVersions =
-  { parent: CellValue, remote: CellValue } |
-  { parent: CellValue, local: CellValue }  |
-  { parent: CellValue, local: CellValue, remote: CellValue };
+export interface AllCellVersions {
+  parent: CellValue;
+  remote: CellValue;
+  local: CellValue;
+}
+export type CellVersions = Partial<AllCellVersions>;
 
 import map = require('lodash/map');
 

app/common/TableData.ts

@@ -17,6 +17,16 @@ interface ColData {
   values: CellValue[];
 }
 
+/**
+ * An interface for a table with rows that may be skipped.
+ */
+export interface SkippableRows {
+  // If there may be skippable rows, return a function to test rowIds for keeping.
+  getKeepFunc(): undefined | ((rowId: number|"new") => boolean);
+  // Get a special row id which represents a skipped sequence of rows.
+  getSkipRowId(): number;
+}
+
 /**
  * TableData class to maintain a single table's data.
  *
@@ -24,7 +34,7 @@ interface ColData {
  * represent it as column-wise arrays. (An early hope was to allow use of TypedArrays, but since
  * types can be mixed, those are not used.)
  */
-export class TableData extends ActionDispatcher {
+export class TableData extends ActionDispatcher implements SkippableRows {
   private _tableId: string;
   private _isLoaded: boolean = false;
   private _fetchPromise?: Promise<void>;
@@ -156,6 +166,16 @@ export class TableData extends ActionDispatcher {
     return function(rowId: number|"new") { return rowId === "new" ? "new" : values[rowMap.get(rowId)!]; };
   }
 
+  // By default, no rows are skippable, all are kept.
+  public getKeepFunc(): undefined | ((rowId: number|"new") => boolean) {
+    return undefined;
+  }
+
+  // By default, no special row id for skip rows is needed.
+  public getSkipRowId(): number {
+    throw new Error('no skip row id defined');
+  }
+
   /**
    * Returns the list of all rowIds in this table, in unspecified and unstable order. Equivalent
    * to getColValues('id').
@@ -171,6 +191,13 @@ export class TableData extends ActionDispatcher {
     return this._rowIdCol.slice(0).sort((a, b) => a - b);
   }
 
+  /**
+   * Returns true if cells may contain multiple versions (e.g. in diffs).
+   */
+  public mayHaveVersions() {
+    return false;
+  }
+
   /**
    * Returns the list of colIds in this table, including 'id'.
    */

app/common/gristTypes.ts

@@ -18,6 +18,7 @@ export const enum GristObjCode {
   Dict            = 'O',
   DateTime        = 'D',
   Date            = 'd',
+  Skip            = 'S',
   Reference       = 'R',
   Exception       = 'E',
   Pending         = 'P',
@@ -119,6 +120,10 @@ export function isVersions(value: CellValue): value is [GristObjCode.Versions, C
   return getObjCode(value) === GristObjCode.Versions;
 }
 
+export function isSkip(value: CellValue): value is [GristObjCode.Skip] {
+  return getObjCode(value) === GristObjCode.Skip;
+}
+
 /**
  * Returns whether a value (as received in a DocAction) represents a list or is null,
  * which is a valid value for list types in grist.
@@ -139,7 +144,7 @@ function isNumberOrNull(v: CellValue) { return isNumber(v) || v === null; }
 function isBoolean(v: CellValue) { return typeof v === 'boolean' || v === 1 || v === 0; }
 
 // These values are not regular cell values, even in a column of type Any.
-const abnormalValueTypes: string[] = [GristObjCode.Exception, GristObjCode.Pending,
+const abnormalValueTypes: string[] = [GristObjCode.Exception, GristObjCode.Pending, GristObjCode.Skip,
                                       GristObjCode.Unmarshallable, GristObjCode.Versions];
 
 function isNormalValue(value: CellValue) {

app/plugin/objtypes.ts

@@ -100,6 +100,15 @@ export class PendingValue {
   }
 }
 
+/**
+ * A trivial placeholder for a value that won't be shown.
+ */
+export class SkipValue {
+  public toString() {
+    return '...';
+  }
+}
+
 /**
  * Produces a Grist-encoded version of the value, e.g. turning a Date into ['d', timestamp].
  * Returns ['U', repr(value)] if it fails to encode otherwise.
@@ -162,6 +171,7 @@ export function decodeObject(value: CellValue): unknown {
       case 'O': return mapValues(args[0] as {[key: string]: CellValue}, decodeObject, {sort: true});
       case 'P': return new PendingValue();
       case 'R': return new Reference(String(args[0]), args[1]);
+      case 'S': return new SkipValue();
       case 'U': return new UnknownValue(args[0]);
     }
   } catch (e) {