From f1842cd89e844d9f860a2d160f5df713e4fc31ba Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Thu, 12 Nov 2020 10:35:15 -0500 Subject: [PATCH] (core) tolerate table renames when displaying differences Summary: This makes data diff rendering robust to changes in the names of tables. It does not yet show information about those changes, but at least it won't fail to show table content changes. Added a missing case to ActionSummary concatenation that came up in testing. Test Plan: added test, updated test Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2661 --- app/client/components/BaseView.js | 2 +- app/client/models/DataTableModelWithDiff.ts | 33 +++++++++++++++------ app/common/ActionSummary.ts | 22 ++++++++++++++ app/server/lib/ActionSummary.ts | 6 ++-- 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index d7b7c211..65cb6619 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -7,7 +7,7 @@ var {getSelectionDesc} = require('app/common/DocActions'); var {nativeCompare, roundDownToMultiple, waitObs} = require('app/common/gutil'); var gristTypes = require('app/common/gristTypes'); var koUtil = require('../lib/koUtil'); -var tableUtil = require('../lib/tableUtil'); +var tableUtil = require('../lib/tableUtil'); var {DataRowModel} = require('../models/DataRowModel'); var {DynamicQuerySet} = require('../models/QuerySet'); var {SortFunc} = require('app/common/SortFunc'); diff --git a/app/client/models/DataTableModelWithDiff.ts b/app/client/models/DataTableModelWithDiff.ts index 325590c1..20cb66ce 100644 --- a/app/client/models/DataTableModelWithDiff.ts +++ b/app/client/models/DataTableModelWithDiff.ts @@ -5,7 +5,7 @@ import { TableRec } from 'app/client/models/entities/TableRec'; import { TableQuerySets } from 'app/client/models/QuerySet'; import { RowGrouping, SortedRowSet } from 'app/client/models/rowset'; import { TableData } from 'app/client/models/TableData'; -import { createEmptyTableDelta, TableDelta } from 'app/common/ActionSummary'; +import { createEmptyTableDelta, getTableIdAfter, getTableIdBefore, TableDelta } from 'app/common/ActionSummary'; import { DisposableWithEvents } from 'app/common/DisposableWithEvents'; import { CellVersions, UserAction } from 'app/common/DocActions'; import { GristObjCode } from "app/common/gristTypes"; @@ -40,12 +40,15 @@ export class ExtraRows { } 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)); + const remoteTableId = getRemoteTableId(tableId, comparison); + this.leftTableDelta = this.comparison?.leftChanges?.tableDeltas[tableId]; + if (remoteTableId) { + this.rightTableDelta = this.comparison?.rightChanges?.tableDeltas[remoteTableId]; + } + this.rightAddRows = new Set(this.rightTableDelta?.addRows.map(id => -id*2)); + 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)); } /** @@ -98,10 +101,12 @@ export class DataTableModelWithDiff extends DisposableWithEvents implements Data this.tableMetaRow = core.tableMetaRow; this.tableQuerySets = core.tableQuerySets; this.docModel = core.docModel; + const tableId = core.tableData.tableId; + const remoteTableId = getRemoteTableId(tableId, comparison) || ''; this.tableData = new TableDataWithDiff( core.tableData, - comparison.leftChanges.tableDeltas[core.tableData.tableId] || createEmptyTableDelta(), - comparison.rightChanges.tableDeltas[core.tableData.tableId] || createEmptyTableDelta()) as any; + comparison.leftChanges.tableDeltas[tableId] || createEmptyTableDelta(), + comparison.rightChanges.tableDeltas[remoteTableId] || createEmptyTableDelta()) as any; this.isLoaded = core.isLoaded; this._wrappedModel = new DataTableModel(this.docModel, this.tableData, this.tableMetaRow); } @@ -244,3 +249,13 @@ function newValue(delta: CellDelta) { if (delta[1] === '?') { return null; } return delta[1]?.[0]; } + +/** + * Figure out the id of the specified table in the remote document. + * Returns null if table is deleted or unknown in the remote document. + */ +function getRemoteTableId(tableId: string, comparison?: DocStateComparisonDetails) { + if (!comparison) { return tableId; } + const parentTableId = getTableIdBefore(comparison.leftChanges.tableRenames, tableId); + return getTableIdAfter(comparison.rightChanges.tableRenames, parentTableId); +} diff --git a/app/common/ActionSummary.ts b/app/common/ActionSummary.ts index 8f4f36d4..8e6bbea6 100644 --- a/app/common/ActionSummary.ts +++ b/app/common/ActionSummary.ts @@ -233,3 +233,25 @@ export function getAffectedTables(summary: ActionSummary): string[] { ...Object.keys(summary.tableDeltas) ]; } + +/** + * Given a tableId from after the specified renames, figure out what the tableId was before + * the renames. Returns null if table didn't exist. + */ +export function getTableIdBefore(renames: LabelDelta[], tableIdAfter: string|null): string|null { + if (tableIdAfter === null) { return tableIdAfter; } + const rename = renames.find(rename => rename[1] === tableIdAfter); + return rename ? rename[0] : tableIdAfter; +} + +/** + * Given a tableId from before the specified renames, figure out what the tableId is after + * the renames. Returns null if there is no valid tableId to return. + */ +export function getTableIdAfter(renames: LabelDelta[], tableIdBefore: string|null): string|null { + if (tableIdBefore === null) { return tableIdBefore; } + const rename = renames.find(rename => rename[0] === tableIdBefore); + const tableIdAfter = rename ? rename[1] : tableIdBefore; + if (tableIdAfter?.startsWith('-')) { return null; } + return tableIdAfter; +} diff --git a/app/server/lib/ActionSummary.ts b/app/server/lib/ActionSummary.ts index 0fde4603..ef9609c1 100644 --- a/app/server/lib/ActionSummary.ts +++ b/app/server/lib/ActionSummary.ts @@ -246,12 +246,14 @@ function planNameMerge(names1: LabelDelta[], names2: LabelDelta[]): NameMerge { result.rename1.set(after1, after2); result.merge.push([before1, after2]); } - // Look through part 2 for any changes not already covered. We won't need to do any - // renaming since table/column names at end of part 2 are just what we want. + // Look through part 2 for any changes not already covered. for (const [before2, after2] of names2) { if (!before2 && !after2) { throw new Error("invalid name change found"); } if (before2 && names1ByFinalName[before2]) { continue; } // Already handled result.merge.push([before2, after2]); + // If table/column is renamed in part 2, and name was stable in part 1, + // rekey any information about it in part 1. + if (before2 && after2) { result.rename1.set(before2, after2); } } // For neatness, sort the merge order. Not essential. result.merge = sortBy(result.merge, ([a, b]) => [a || "", b || ""]);