From 9c4814e7aa1f833033c6305d8d8fe6de6873afcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Thu, 4 Jul 2024 14:27:49 +0200 Subject: [PATCH] (core) Bundling save funciton in the field editor Summary: Some editors do some async work before saving the value (Ref column can add new records). Those actions were send without bundling, so it wasn't possible to undo those actions with togheter. Test Plan: Added new test Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D4285 --- app/client/widgets/FieldEditor.ts | 97 ++++++++++++++++--------------- test/nbrowser/ReferenceList.ts | 35 +++++++++++ 2 files changed, 84 insertions(+), 48 deletions(-) diff --git a/app/client/widgets/FieldEditor.ts b/app/client/widgets/FieldEditor.ts index e473c154..23634d91 100644 --- a/app/client/widgets/FieldEditor.ts +++ b/app/client/widgets/FieldEditor.ts @@ -380,59 +380,60 @@ export class FieldEditor extends Disposable { if (!editor) { return false; } // Make sure the editor is save ready const saveIndex = this._cursor.rowIndex(); - await editor.prepForSave(); - if (this.isDisposed()) { - // We shouldn't normally get disposed here, but if we do, avoid confusing JS errors. - console.warn(t("Unable to finish saving edited cell")); // tslint:disable-line:no-console - return false; - } - - // Then save the value the appropriate way - // TODO: this isFormula value doesn't actually reflect if editing the formula, since - // editingFormula() is used for toggling column headers, and this is deferred to start of - // typing (a double-click or Enter) does not immediately set it. (This can cause a - // console.warn below, although harmless.) - const isFormula = this._field.editingFormula(); - const col = this._field.column(); - let waitPromise: Promise|null = null; - - if (isFormula) { - const formula = String(editor.getCellValue() ?? ''); - // Bundle multiple changes so that we can undo them in one step. - if (isFormula !== col.isFormula.peek() || formula !== col.formula.peek()) { - waitPromise = this._gristDoc.docData.bundleActions(null, () => Promise.all([ - col.updateColValues({isFormula, formula}), - // If we're saving a non-empty formula, then also add an empty record to the table - // so that the formula calculation is visible to the user. - (!this._detached.get() && this._editRow._isAddRow.peek() && formula !== "" ? - this._editRow.updateColValues({}) : undefined), - ])); + return await this._gristDoc.docData.bundleActions(null, async () => { + await editor.prepForSave(); + if (this.isDisposed()) { + // We shouldn't normally get disposed here, but if we do, avoid confusing JS errors. + console.warn(t("Unable to finish saving edited cell")); // tslint:disable-line:no-console + return false; } - } else { - const value = editor.getCellValue(); - if (col.isRealFormula()) { - // tslint:disable-next-line:no-console - console.warn(t("It should be impossible to save a plain data value into a formula column")); + // Then save the value the appropriate way + // TODO: this isFormula value doesn't actually reflect if editing the formula, since + // editingFormula() is used for toggling column headers, and this is deferred to start of + // typing (a double-click or Enter) does not immediately set it. (This can cause a + // console.warn below, although harmless.) + const isFormula = this._field.editingFormula(); + const col = this._field.column(); + let waitPromise: Promise|null = null; + + if (isFormula) { + const formula = String(editor.getCellValue() ?? ''); + // Bundle multiple changes so that we can undo them in one step. + if (isFormula !== col.isFormula.peek() || formula !== col.formula.peek()) { + waitPromise = Promise.all([ + col.updateColValues({isFormula, formula}), + // If we're saving a non-empty formula, then also add an empty record to the table + // so that the formula calculation is visible to the user. + (!this._detached.get() && this._editRow._isAddRow.peek() && formula !== "" ? + this._editRow.updateColValues({}) : undefined), + ]); + } } else { - // This could still be an isFormula column if it's empty (isEmpty is true), but we don't - // need to toggle isFormula in that case, since the data engine takes care of that. - waitPromise = setAndSave(this._editRow, this._field, value); + const value = editor.getCellValue(); + if (col.isRealFormula()) { + // tslint:disable-next-line:no-console + console.warn(t("It should be impossible to save a plain data value into a formula column")); + } else { + // This could still be an isFormula column if it's empty (isEmpty is true), but we don't + // need to toggle isFormula in that case, since the data engine takes care of that. + waitPromise = setAndSave(this._editRow, this._field, value); + } } - } - const event: FieldEditorStateEvent = { - position : this.cellPosition(), - wasModified : this._editorHasChanged, - currentState : this._editorHolder.get()?.editorState?.get(), - type : this._field.column.peek().pureType.peek() - }; - this.saveEmitter.emit(event); + const event: FieldEditorStateEvent = { + position : this.cellPosition(), + wasModified : this._editorHasChanged, + currentState : this._editorHolder.get()?.editorState?.get(), + type : this._field.column.peek().pureType.peek() + }; + this.saveEmitter.emit(event); - const cursor = this._cursor; - // Deactivate the editor. We are careful to avoid using `this` afterwards. - this.dispose(); - await waitPromise; - return isFormula || (saveIndex !== cursor.rowIndex()); + const cursor = this._cursor; + // Deactivate the editor. We are careful to avoid using `this` afterwards. + this.dispose(); + await waitPromise; + return isFormula || (saveIndex !== cursor.rowIndex()); + }); } } diff --git a/test/nbrowser/ReferenceList.ts b/test/nbrowser/ReferenceList.ts index 7757b8e8..7f0d4a7d 100644 --- a/test/nbrowser/ReferenceList.ts +++ b/test/nbrowser/ReferenceList.ts @@ -13,6 +13,41 @@ describe('ReferenceList', function() { }); describe('other', function() { + it('fix: changing ref list with a new referenced row was not bundled', async function() { + // When user added a new referenced row through the RefList editor, the UI sent two separate + // actions. One for adding the new row, and another for updating the RefList column. + await session.tempNewDoc(cleanup); + await gu.sendActions([ + ['ModifyColumn', 'Table1', 'B', {type: 'RefList:Table1'}], + ['AddRecord', 'Table1', null, {A: 'a'}], + ]); + await gu.openColumnPanel(); + await gu.getCell('B', 1).doClick(); + await gu.setRefShowColumn('A'); + + await gu.getCell('B', 1).click(); + await gu.sendKeys(Key.ENTER, 'b'); + await gu.waitToPass(async () => { + await driver.findWait('.test-ref-editor-new-item', 100).click(); + }); + await gu.sendKeys(Key.ENTER); + await gu.waitForServer(); + + // Check the data - use waitToPass helper, as previously it might have failed + // as 2 separate actions were sent. + await gu.waitToPass(async () => { + assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2]), ['a', 'b']); + assert.deepEqual(await gu.getVisibleGridCells('B', [1, 2]), ['b', '']); + assert.equal(await gu.getGridRowCount(), 3); + }); + + // Now press undo once, and check that the new row is removed and the RefList is updated. + await gu.undo(); + assert.deepEqual(await gu.getVisibleGridCells('A', [1]), ['a']); + assert.deepEqual(await gu.getVisibleGridCells('B', [1]), ['']); + assert.equal(await gu.getGridRowCount(), 2); + }); + it('fix: doesnt break when table is renamed', async function() { // There was a bug in this scenario: // 1. Create a Ref column that targets itself