diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index bf854174..d7b7c211 100644 --- a/app/client/components/BaseView.js +++ b/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. - // We flip their sign to make them as belonging to the remote table only. - // TODO: if we wanted to show rows removed in the local (left) table, we'd need to - // 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); + // Assign extra row ids for any rows added in the remote (right) table or removed in the + // local (left) table. + const extraRowIds = this.extraRows.getExtraRows(); this._mainRowSource = rowset.ExtendedRowSource.create(this, this._mainRowSource, extraRowIds); } diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index 827500d7..2da241fb 100644 --- a/app/client/components/GridView.js +++ b/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(); - if (rightAddRows.has(rowId)) { return 'diff-remote'; } - else if (rightRemoveRows.has(rowId)) { return 'diff-parent'; } - else if (leftAddRows.has(rowId)) { return 'diff-local'; } - return ''; + const rowType = self.extraRows.getRowType(row.id()); + return rowType && `diff-${rowType}` || ''; }) : null, kd.foreach(v.viewFields(), function(field) { diff --git a/app/client/components/viewCommon.css b/app/client/components/viewCommon.css index 44c48c9a..1a7fc73e 100644 --- a/app/client/components/viewCommon.css +++ b/app/client/components/viewCommon.css @@ -152,26 +152,21 @@ white-space: pre-wrap; } -.diff-change .diff-parent, .diff-change .diff-remote { - 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 { +.diff-local, .diff-local-add { 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; } diff --git a/app/client/models/DataTableModelWithDiff.ts b/app/client/models/DataTableModelWithDiff.ts index 52040a1f..79883190 100644 --- a/app/client/models/DataTableModelWithDiff.ts +++ b/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; + readonly rightRemoveRows: Set; + readonly leftAddRows: Set; + readonly leftRemoveRows: Set; + + /** + * 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 { + 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; } - } - if (rowId < 0) { - const value = this.rightTableDelta.finalRowContent?.[-rowId]?.[colId]; + } else { // keep row.id consistent with rowId for convenience. - if (colId === 'id') { return - (value as number); } - return value; + if (colId === 'id') { return rowId; } + 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); } diff --git a/app/client/ui/ShareMenu.ts b/app/client/ui/ShareMenu.ts index d61f2995..fbcfcc3a 100644 --- a/app/client/ui/ShareMenu.ts +++ b/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'), + ), ]; } diff --git a/app/client/widgets/DiffBox.ts b/app/client/widgets/DiffBox.ts index 22292a15..91d979f6 100644 --- a/app/client/widgets/DiffBox.ts +++ b/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), - formatter.format(versions.remote)); + return this._prepareTextDiff( + 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), - formatter.format(versions.local)) + return this._prepareTextDiff( + 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)], - [DIFF_LOCAL, formatter.format(versions.local)], - [DIFF_INSERT, formatter.format(versions.remote)]]; + return [[DIFF_DELETE, formatter.formatAny(versions.parent)], + [DIFF_LOCAL, formatter.formatAny(versions.local)], + [DIFF_INSERT, formatter.formatAny(versions.remote)]]; } // Run diff-match-patch on the text, do its cleanup, and then some extra diff --git a/app/common/ActionSummary.ts b/app/common/ActionSummary.ts index 831ee596..8f4f36d4 100644 --- a/app/common/ActionSummary.ts +++ b/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}; } /** diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index fbaab34e..c0eff7ad 100644 --- a/app/server/lib/DocApi.ts +++ b/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);