diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index 98479502..2f9231ac 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -44,7 +44,7 @@ import {startWelcomeTour} from 'app/client/ui/welcomeTour'; import {isNarrowScreen, mediaSmall, testId} from 'app/client/ui2018/cssVars'; import {IconName} from 'app/client/ui2018/IconList'; import {FieldEditor} from "app/client/widgets/FieldEditor"; -import {ActionGroup} from 'app/common/ActionGroup'; +import {MinimalActionGroup} from 'app/common/ActionGroup'; import {ClientQuery} from "app/common/ActiveDocAPI"; import {delay} from 'app/common/delay'; import {DisposableWithEvents} from 'app/common/DisposableWithEvents'; @@ -376,7 +376,7 @@ export class GristDoc extends DisposableWithEvents { * Switch to the view/section and scroll to the record indicated by cursorPos. If cursorPos is * null, then moves to a position best suited for optActionGroup (not yet implemented). */ - public async moveToCursorPos(cursorPos?: CursorPos, optActionGroup?: ActionGroup): Promise { + public async moveToCursorPos(cursorPos?: CursorPos, optActionGroup?: MinimalActionGroup): Promise { if (!cursorPos || cursorPos.sectionId == null) { // TODO We could come up with a suitable cursorPos here based on the action itself. // This should only come up if trying to undo/redo after reloading a page (since the cursorPos diff --git a/app/client/components/UndoStack.ts b/app/client/components/UndoStack.ts index 0d652710..09edcdd5 100644 --- a/app/client/components/UndoStack.ts +++ b/app/client/components/UndoStack.ts @@ -1,12 +1,12 @@ import {CursorPos} from 'app/client/components/Cursor'; import {GristDoc} from 'app/client/components/GristDoc'; import * as dispose from 'app/client/lib/dispose'; -import {ActionGroup} from 'app/common/ActionGroup'; +import {MinimalActionGroup} from 'app/common/ActionGroup'; import {PromiseChain} from 'app/common/gutil'; import {fromKo, Observable} from 'grainjs'; import * as ko from 'knockout'; -export interface ActionGroupWithCursorPos extends ActionGroup { +export interface ActionGroupWithCursorPos extends MinimalActionGroup { cursorPos?: CursorPos; } @@ -27,13 +27,13 @@ export class UndoStack extends dispose.Disposable { private _gristDoc: GristDoc; private _stack: ActionGroupWithCursorPos[]; private _pointer: number; - private _linkMap: {[actionNum: number]: ActionGroup}; + private _linkMap: {[actionNum: number]: MinimalActionGroup}; // Chain of promises which send undo actions to the server. This delays the execution of the // next action until the current one has been received and moved the pointer index. private _undoChain = new PromiseChain(); - public create(log: ActionGroup[], options: {gristDoc: GristDoc}) { + public create(log: MinimalActionGroup[], options: {gristDoc: GristDoc}) { this._gristDoc = options.gristDoc; // TODO: _stack and _linkMap grow without bound within a single session. @@ -65,7 +65,7 @@ export class UndoStack extends dispose.Disposable { * Should only be given own actions. Pays attention to actionNum, otherId, linkId, and * uses those to adjust undo index. */ - public pushAction(ag: ActionGroup): void { + public pushAction(ag: MinimalActionGroup): void { if (!ag.fromSelf) { return; } @@ -133,7 +133,7 @@ export class UndoStack extends dispose.Disposable { /** * Find all actionGroups in the bundle that starts with the given action group. */ - private _findActionBundle(ag: ActionGroup) { + private _findActionBundle(ag: MinimalActionGroup) { const prevNums = new Set(); const actionGroups = []; // Follow references through the linkMap adding items to the array bundle. diff --git a/app/common/ActionGroup.ts b/app/common/ActionGroup.ts index f8977e24..4b9d6f15 100644 --- a/app/common/ActionGroup.ts +++ b/app/common/ActionGroup.ts @@ -1,19 +1,28 @@ import {ActionSummary} from 'app/common/ActionSummary'; -/** This is the action representation the client works with. */ -export interface ActionGroup { +/** + * This is the action representation the client works with, for the purposes of undos/redos. + */ +export interface MinimalActionGroup { actionNum: number; actionHash: string; - desc?: string; - actionSummary: ActionSummary; fromSelf: boolean; linkId: number; otherId: number; - time: number; - user: string; rowIdHint: number; // If non-zero, this is a rowId that would be a good place to put // the cursor after an undo. - primaryAction: string; // The name of the first user action in the ActionGroup. isUndo: boolean; // True if the first user action is ApplyUndoActions. +} + +/** + * This is the action representation the client works with, for the purposes of document + * history and undos/redos. + */ +export interface ActionGroup extends MinimalActionGroup { + desc?: string; + actionSummary: ActionSummary; + time: number; + user: string; + primaryAction: string; // The name of the first user action in the ActionGroup. internal: boolean; // True if it is inappropriate to log/undo the action. } diff --git a/app/common/DocListAPI.ts b/app/common/DocListAPI.ts index c78ae3e1..4534fd0a 100644 --- a/app/common/DocListAPI.ts +++ b/app/common/DocListAPI.ts @@ -1,4 +1,4 @@ -import {ActionGroup} from 'app/common/ActionGroup'; +import {MinimalActionGroup} from 'app/common/ActionGroup'; import {TableDataAction} from 'app/common/DocActions'; import {Role} from 'app/common/roles'; import {StringUnion} from 'app/common/StringUnion'; @@ -41,7 +41,7 @@ export interface OpenLocalDocResult { docFD: number; clientId: string; // the docFD is meaningful only in the context of this session doc: {[tableId: string]: TableDataAction}; - log: ActionGroup[]; + log: MinimalActionGroup[]; recoveryMode?: boolean; userOverride?: UserOverride; } diff --git a/app/server/lib/ActionHistory.ts b/app/server/lib/ActionHistory.ts index 8b162d18..0c2c8235 100644 --- a/app/server/lib/ActionHistory.ts +++ b/app/server/lib/ActionHistory.ts @@ -14,7 +14,7 @@ */ import {LocalActionBundle} from 'app/common/ActionBundle'; -import {ActionGroup} from 'app/common/ActionGroup'; +import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup'; import {createEmptyActionSummary} from 'app/common/ActionSummary'; import {getSelectionDesc, UserAction} from 'app/common/DocActions'; import {DocState} from 'app/common/UserAPI'; @@ -27,7 +27,7 @@ export interface ActionGroupOptions { summarize?: boolean; // The client for which the action group is being prepared, if known. - client?: {clientId: string}|null; + clientId?: string; // Values returned by the action, if known. retValues?: any[]; @@ -36,6 +36,20 @@ export interface ActionGroupOptions { internal?: boolean; } +/** + * Metadata about an action that is needed for undo/redo stack. + */ +export interface ActionHistoryUndoInfoWithoutClient { + otherId: number; + linkId: number; + rowIdHint: number; + isUndo: boolean; +} + +export interface ActionHistoryUndoInfo extends ActionHistoryUndoInfoWithoutClient { + clientId: string; +} + export abstract class ActionHistory { /** * Initialize the ActionLog by reading the database. No other methods may be used until the @@ -112,6 +126,8 @@ export abstract class ActionHistory { */ public abstract getRecentActionGroups(maxActions: number, options: ActionGroupOptions): Promise; + public abstract getRecentMinimalActionGroups(maxActions: number, clientId?: string): Promise; + /** * Get the most recent states from the history. States are just * actions without any content. Results are ordered by most recent @@ -131,10 +147,10 @@ export abstract class ActionHistory { * Associates an action with a client. This association is expected to be transient, rather * than persistent. It should survive a client-side reload but not a server-side restart. */ - public abstract setActionClientId(actionHash: string, clientId: string): void; + public abstract setActionUndoInfo(actionHash: string, undoInfo: ActionHistoryUndoInfo): void; /** Check for any client associated with an action, identified by checksum */ - public abstract getActionClientId(actionHash: string): string | undefined; + public abstract getActionUndoInfo(actionHash: string): ActionHistoryUndoInfo | undefined; /** * Remove all stored actions except the last keepN and run the VACUUM command @@ -232,11 +248,62 @@ export function humanDescription(actions: UserAction[]): string { export function asActionGroup(history: ActionHistory, act: LocalActionBundle, options: ActionGroupOptions): ActionGroup { - const {summarize, client, retValues} = options; + const {summarize, clientId} = options; const info = act.info[1]; - const fromSelf = (client && client.clientId && act.actionHash) ? - (history.getActionClientId(act.actionHash) === client.clientId) : false; + const fromSelf = (act.actionHash && clientId) ? + (history.getActionUndoInfo(act.actionHash)?.clientId === clientId) : false; + + const {extra: {primaryAction}, minimal: {rowIdHint, isUndo}} = + getActionUndoInfoWithoutClient(act, options.retValues); + + return { + actionNum: act.actionNum, + actionHash: act.actionHash || "", + desc: info.desc || humanDescription(act.userActions), + actionSummary: summarize ? summarizeAction(act) : createEmptyActionSummary(), + fromSelf, + linkId: info.linkId, + otherId: info.otherId, + time: info.time, + user: info.user, + rowIdHint, + primaryAction, + isUndo, + internal: options.internal || false, + }; +} + +export function asMinimalActionGroup(history: ActionHistory, + act: {actionHash: string, actionNum: number}, + clientId?: string): MinimalActionGroup { + const undoInfo = act.actionHash ? history.getActionUndoInfo(act.actionHash) : undefined; + const fromSelf = clientId ? (undoInfo?.clientId === clientId) : false; + return { + actionNum: act.actionNum, + actionHash: act.actionHash || "", + fromSelf, + linkId: undoInfo?.linkId || 0, + otherId: undoInfo?.otherId || 0, + rowIdHint: undoInfo?.rowIdHint || 0, + isUndo: undoInfo?.isUndo || false, + }; +} + +export function getActionUndoInfo(act: LocalActionBundle, clientId: string, + retValues: any[]): ActionHistoryUndoInfo { + return { + ...getActionUndoInfoWithoutClient(act, retValues).minimal, + clientId, + }; +} + +/** + * Compute undo information from an action bundle and return values if available. + * Results are returned as {minimal, extra} where core has information needed for minimal + * action groups, and extra has information only needed for full action groups. + */ +function getActionUndoInfoWithoutClient(act: LocalActionBundle, retValues?: any[]) { let rowIdHint = 0; if (retValues) { // A hint for cursor position. This logic used to live on the client, but now trying to @@ -255,22 +322,18 @@ export function asActionGroup(history: ActionHistory, } } + const info = act.info[1]; const primaryAction: string = String((act.userActions[0] || [""])[0]); const isUndo = primaryAction === 'ApplyUndoActions'; - return { - actionNum: act.actionNum, - actionHash: act.actionHash || "", - desc: info.desc || humanDescription(act.userActions), - actionSummary: summarize ? summarizeAction(act) : createEmptyActionSummary(), - fromSelf, - linkId: info.linkId, - otherId: info.otherId, - time: info.time, - user: info.user, - rowIdHint, - primaryAction, - isUndo, - internal: options.internal || false, + minimal: { + rowIdHint, + otherId: info.otherId, + linkId: info.linkId, + isUndo, + }, + extra: { + primaryAction, + }, }; } diff --git a/app/server/lib/ActionHistoryImpl.ts b/app/server/lib/ActionHistoryImpl.ts index 6f13a17c..5048ca6e 100644 --- a/app/server/lib/ActionHistoryImpl.ts +++ b/app/server/lib/ActionHistoryImpl.ts @@ -2,14 +2,15 @@ * Minimal ActionHistory implementation */ import {LocalActionBundle} from 'app/common/ActionBundle'; -import {ActionGroup} from 'app/common/ActionGroup'; +import {ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup'; import * as marshaller from 'app/common/marshal'; import {DocState} from 'app/common/UserAPI'; import {reportTimeTaken} from 'app/server/lib/reportTimeTaken'; import * as crypto from 'crypto'; import keyBy = require('lodash/keyBy'); import mapValues = require('lodash/mapValues'); -import {ActionGroupOptions, ActionHistory, asActionGroup} from './ActionHistory'; +import {ActionGroupOptions, ActionHistory, ActionHistoryUndoInfo, asActionGroup, + asMinimalActionGroup} from './ActionHistory'; import {ISQLiteDB, ResultRow} from './SQLiteDB'; // History will from time to time be pruned back to within these limits @@ -165,7 +166,7 @@ export class ActionHistoryImpl implements ActionHistory { private _haveLocalSent: boolean = false; // cache for this.haveLocalSent() private _haveLocalUnsent: boolean = false; // cache for this.haveLocalUnsent() private _initialized: boolean = false; // true when initialize() has completed - private _actionClient = new Map(); // transient cache of who created actions + private _actionUndoInfo = new Map(); // transient cache of undo info constructor(private _db: ISQLiteDB, private _options: ActionHistoryOptions = defaultOptions) { } @@ -174,7 +175,7 @@ export class ActionHistoryImpl implements ActionHistory { public async wipe() { await this._db.run("UPDATE _gristsys_ActionHistoryBranch SET actionRef = NULL"); await this._db.run("DELETE FROM _gristsys_ActionHistory"); - this._actionClient.clear(); + this._actionUndoInfo.clear(); } public async initialize(): Promise { @@ -382,6 +383,17 @@ export class ActionHistoryImpl implements ActionHistory { () => actions.map(row => asActionGroup(this, decodeActionFromRow(row), options))); } + public async getRecentMinimalActionGroups(maxActions: number, clientId?: string): Promise { + // Don't look at content of actions. + const actions = await this._getRecentActionRows(maxActions, false); + return reportTimeTaken( + "getRecentMinimalActionGroups", + () => actions.map(row => asMinimalActionGroup( + this, + {actionHash: row.actionHash, actionNum: row.actionNum}, + clientId))); + } + public async getRecentStates(maxStates?: number): Promise { const branches = await this._getBranches(); const states = await this._fetchParts(null, @@ -432,25 +444,27 @@ export class ActionHistoryImpl implements ActionHistory { }); } - public setActionClientId(actionHash: string, clientId: string): void { - this._actionClient.set(actionHash, clientId); + public setActionUndoInfo(actionHash: string, undoInfo: ActionHistoryUndoInfo): void { + this._actionUndoInfo.set(actionHash, undoInfo); } - public getActionClientId(actionHash: string): string | undefined { - return this._actionClient.get(actionHash); + public getActionUndoInfo(actionHash: string): ActionHistoryUndoInfo | undefined { + return this._actionUndoInfo.get(actionHash); } /** * Fetches the most recent action row from the history, orderd with earlier actions first. * If `maxActions` is supplied, at most that number of actions are returned. */ - private async _getRecentActionRows(maxActions?: number): Promise { + private async _getRecentActionRows(maxActions: number|undefined, + withBody: boolean = true): Promise { const branches = await this._getBranches(); + const columns = '_gristsys_ActionHistory.id, actionNum, actionHash' + (withBody ? ', body' : ''); const result = await this._fetchParts(null, - branches.local_unsent, - "_gristsys_ActionHistory.id, actionNum, actionHash, body", - maxActions, - true); + branches.local_unsent, + columns, + maxActions, + true); result.reverse(); // Implementation note: this could be optimized away when `maxActions` // is not specified, by simply asking _fetchParts for ascending order. return result; @@ -618,7 +632,7 @@ export class ActionHistoryImpl implements ActionHistory { await this._db.run(`DELETE FROM _gristsys_ActionHistory WHERE id ${invert ? 'NOT' : ''} IN (${idList})`); for (const row of rows) { - this._actionClient.delete(row.actionHash); + this._actionUndoInfo.delete(row.actionHash); } return ids; } diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 5c41622e..01fc6ec7 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -15,7 +15,7 @@ import fetch from 'node-fetch'; import * as tmp from 'tmp'; import {getEnvContent, LocalActionBundle, SandboxActionBundle, UserActionBundle} from 'app/common/ActionBundle'; -import {ActionGroup} from 'app/common/ActionGroup'; +import { ActionGroup, MinimalActionGroup} from 'app/common/ActionGroup'; import { ApplyUAOptions, ApplyUAResult, @@ -251,7 +251,7 @@ export class ActiveDoc extends EventEmitter { */ public async getRecentActions(docSession: OptDocSession, summarize: boolean): Promise { const groups = await this._actionHistory.getRecentActionGroups(MAX_RECENT_ACTIONS, - {client: docSession.client, summarize}); + {clientId: docSession.client?.clientId, summarize}); const permittedGroups: ActionGroup[] = []; // Process groups serially since the work is synchronous except for some // possible db accesses that will be serialized in any case. @@ -263,6 +263,11 @@ export class ActiveDoc extends EventEmitter { return permittedGroups; } + public async getRecentMinimalActions(docSession: OptDocSession): Promise { + return this._actionHistory.getRecentMinimalActionGroups( + MAX_RECENT_ACTIONS, docSession.client?.clientId); + } + /** expose action history for tests */ public getActionHistory(): ActionHistory { return this._actionHistory; diff --git a/app/server/lib/DocManager.ts b/app/server/lib/DocManager.ts index 008c80f5..f0944c34 100644 --- a/app/server/lib/DocManager.ts +++ b/app/server/lib/DocManager.ts @@ -317,7 +317,7 @@ export class DocManager extends EventEmitter { const [metaTables, recentActions] = await Promise.all([ activeDoc.fetchMetaTables(docSession), - activeDoc.getRecentActions(docSession, false) + activeDoc.getRecentMinimalActions(docSession) ]); this.emit('open-doc', this.storageManager.getPath(activeDoc.docName)); diff --git a/app/server/lib/Sharing.ts b/app/server/lib/Sharing.ts index c716adea..f9274654 100644 --- a/app/server/lib/Sharing.ts +++ b/app/server/lib/Sharing.ts @@ -14,7 +14,7 @@ import {shortDesc} from 'app/server/lib/shortDesc'; import * as assert from 'assert'; import {Mutex} from 'async-mutex'; import * as Deque from 'double-ended-queue'; -import {ActionHistory, asActionGroup} from './ActionHistory'; +import { ActionHistory, asActionGroup, getActionUndoInfo} from './ActionHistory'; import {summarizeAction} from "./ActionSummary"; import {ActiveDoc} from './ActiveDoc'; import {makeExceptionalDocSession, OptDocSession} from './DocSession'; @@ -275,7 +275,9 @@ export class Sharing { // Check isCalculate because that's not an action we should allow undo/redo for (it's not // considered as performed by a particular client). if (client && client.clientId && !isCalculate) { - this._actionHistory.setActionClientId(localActionBundle.actionHash!, client.clientId); + this._actionHistory.setActionUndoInfo( + localActionBundle.actionHash!, + getActionUndoInfo(localActionBundle, client.clientId, sandboxActionBundle.retValues)); } }); } @@ -286,7 +288,7 @@ export class Sharing { // Broadcast the action to connected browsers. const actionGroup = asActionGroup(this._actionHistory, localActionBundle, { - client, + clientId: client?.clientId, retValues: sandboxActionBundle.retValues, // Mark the on-open Calculate action as internal. In future, synchronizing fields to today's // date and other changes from external values may count as internal.