Archives/gristlabs_grist-core
1
0
Fork 0
mirror of https://github.com/gristlabs/grist-core.git synced 2024-10-27 20:44:07 +00:00
Code Issues Projects Releases Wiki Activity

(core) Change UndoStack._linkMap to store an array of action groups for each linkId

Browse Source 
Summary:
While working on type conversion, I ran into a bug when multiple action groups belonging to a bundle are quickly sent. `DocData._sendActionsImpl` can set the same `linkId: this._lastActionNum` on multiple action groups before `_lastActionNum` is updated with a result from the server. Only one of these groups gets saved in `UndoStack._linkMap` so undoing a bundle misses out on some action groups.

This diff associates each `linkId` key with an array of action groups instead of just one, then combines them all together when undoing.

Test Plan: I've confirmed that this fixes my problem within my type conversion diff, but I haven't found a way to reproduce the general problem in master. I think the existing tests are probably fine since undo is tested extensively everywhere, but I'd also like to see if there are existing bugs which this fixes.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3223
This commit is contained in:
Alex Hall 2022-01-19 14:41:04 +02:00
parent db7d1802ce
commit 0c80547e0d
1 changed files with 13 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
app/client/components/UndoStack.ts
View File

@ -2,9 +2,10 @@ import {CursorPos} from 'app/client/components/Cursor';
import {GristDoc} from 'app/client/components/GristDoc'; import {GristDoc} from 'app/client/components/GristDoc';
import * as dispose from 'app/client/lib/dispose'; import * as dispose from 'app/client/lib/dispose';
import {MinimalActionGroup} from 'app/common/ActionGroup'; import {MinimalActionGroup} from 'app/common/ActionGroup';
import {PromiseChain} from 'app/common/gutil'; import {PromiseChain, setDefault} from 'app/common/gutil';
import {fromKo, Observable} from 'grainjs'; import {fromKo, Observable} from 'grainjs';
import * as ko from 'knockout'; import * as ko from 'knockout';
import sortBy = require('lodash/sortBy');
export interface ActionGroupWithCursorPos extends MinimalActionGroup { export interface ActionGroupWithCursorPos extends MinimalActionGroup {
cursorPos?: CursorPos; cursorPos?: CursorPos;
@ -27,7 +28,7 @@ export class UndoStack extends dispose.Disposable {
private _gristDoc: GristDoc; private _gristDoc: GristDoc;
private _stack: ActionGroupWithCursorPos[]; private _stack: ActionGroupWithCursorPos[];
private _pointer: number; private _pointer: number;
private _linkMap: {[actionNum: number]: MinimalActionGroup}; private _linkMap: Map<number, MinimalActionGroup[]>;
// Chain of promises which send undo actions to the server. This delays the execution of the // 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. // next action until the current one has been received and moved the pointer index.
@ -43,7 +44,7 @@ export class UndoStack extends dispose.Disposable {
this._pointer = 0; this._pointer = 0;
// Map leading from actionNums to the action groups which link to them. // Map leading from actionNums to the action groups which link to them.
this._linkMap = {}; this._linkMap = new Map();
// Observables for when there is nothing to undo/redo. // Observables for when there is nothing to undo/redo.
this.undoDisabledObs = ko.observable(true); this.undoDisabledObs = ko.observable(true);
@ -74,7 +75,7 @@ export class UndoStack extends dispose.Disposable {
if (ag.linkId) { if (ag.linkId) {
// Link action. Add the action to the linkMap, but not to any stacks. // Link action. Add the action to the linkMap, but not to any stacks.
this._linkMap[ag.linkId] = ag; setDefault(this._linkMap, ag.linkId, []).push(ag);
} else if (otherIndex > -1) { } else if (otherIndex > -1) {
// Undo/redo action from the current session. // Undo/redo action from the current session.
this._pointer = ag.isUndo ? otherIndex : otherIndex + 1; this._pointer = ag.isUndo ? otherIndex : otherIndex + 1;
@ -136,13 +137,18 @@ export class UndoStack extends dispose.Disposable {
private _findActionBundle(ag: MinimalActionGroup) { private _findActionBundle(ag: MinimalActionGroup) {
const prevNums = new Set(); const prevNums = new Set();
const actionGroups = []; const actionGroups = [];
const queue = [ag];
// Follow references through the linkMap adding items to the array bundle. // Follow references through the linkMap adding items to the array bundle.
while (ag && !prevNums.has(ag.actionNum)) { while (queue.length) {
ag = queue.pop()!;
// Checking that actions are only accessed once prevents an infinite circular loop. // Checking that actions are only accessed once prevents an infinite circular loop.
if (prevNums.has(ag.actionNum)) {
break;
}
actionGroups.push(ag); actionGroups.push(ag);
prevNums.add(ag.actionNum); prevNums.add(ag.actionNum);
ag = this._linkMap[ag.actionNum]; queue.push(...this._linkMap.get(ag.actionNum) || []);
} }
return actionGroups; return sortBy(actionGroups, group => group.actionNum);
} }
} }