From c387fc4bceec8f667821835c9bb90f3b2903dcec Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 18 Nov 2020 10:54:23 -0500 Subject: [PATCH] (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 | 4 +- app/client/components/viewCommon.css | 5 ++ app/client/lib/sortUtil.ts | 4 +- app/client/models/ClientColumnGetters.ts | 23 +++++- app/client/models/DataTableModelWithDiff.ts | 40 +++++++--- app/client/models/rowset.ts | 82 +++++++++++++++++++-- app/common/DocActions.ts | 10 ++- app/common/TableData.ts | 29 +++++++- app/common/gristTypes.ts | 7 +- app/plugin/objtypes.ts | 10 +++ 10 files changed, 185 insertions(+), 29 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 65cb6619..6d1e295a 100644 --- a/app/client/components/BaseView.js +++ b/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) => { diff --git a/app/client/components/viewCommon.css b/app/client/components/viewCommon.css index 1a7fc73e..1821af32 100644 --- a/app/client/components/viewCommon.css +++ b/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'; } diff --git a/app/client/lib/sortUtil.ts b/app/client/lib/sortUtil.ts index 71db94f6..e458de13 100644 --- a/app/client/lib/sortUtil.ts +++ b/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(); diff --git a/app/client/models/ClientColumnGetters.ts b/app/client/models/ClientColumnGetters.ts index f34decc1..c0734173 100644 --- a/app/client/models/ClientColumnGetters.ts +++ b/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 { diff --git a/app/client/models/DataTableModelWithDiff.ts b/app/client/models/DataTableModelWithDiff.ts index 20cb66ce..5d238cdd 100644 --- a/app/client/models/DataTableModelWithDiff.ts +++ b/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]; diff --git a/app/client/models/rowset.ts b/app/client/models/rowset.ts index fe3feca1..8f4b93bc 100644 --- a/app/client/models/rowset.ts +++ b/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; export type RowsChanged = RowList | typeof ALL; @@ -514,10 +515,13 @@ export class SortedRowSet extends RowListener { private _allRows: Set = new Set(); private _isPaused: boolean = false; private _koArray: KoArray; + private _keepFunc?: (rowId: number|'new') => boolean; - constructor(private _compareFunc: CompareFunc) { + constructor(private _compareFunc: CompareFunc, + private _skippableRows?: SkippableRows) { super(); this._koArray = this.autoDispose(koArray()); + 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) { diff --git a/app/common/DocActions.ts b/app/common/DocActions.ts index 6800324f..879f274a 100644 --- a/app/common/DocActions.ts +++ b/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; import map = require('lodash/map'); diff --git a/app/common/TableData.ts b/app/common/TableData.ts index 55b39d7f..ce9e0a7d 100644 --- a/app/common/TableData.ts +++ b/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; @@ -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'. */ diff --git a/app/common/gristTypes.ts b/app/common/gristTypes.ts index 503cb4a6..bdb037e4 100644 --- a/app/common/gristTypes.ts +++ b/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) { diff --git a/app/plugin/objtypes.ts b/app/plugin/objtypes.ts index 7baf8ea4..b2a5a0f0 100644 --- a/app/plugin/objtypes.ts +++ b/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) {