From 65a722501d80038dc29a027332adb4af186e5d46 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Tue, 20 Apr 2021 17:57:45 -0400 Subject: [PATCH] (core) Show count of formula errors in the column config in the right-side panel. Summary: - Cache the count by column, factoring out ColumnCache from ColumnACIndexes, which uses a similar pattern. - Update error counts in response to column selection and to data changes. Test Plan: Adds a test case for the new message Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2780 --- app/client/models/ColumnACIndexes.ts | 32 +++----------- app/client/models/ColumnCache.ts | 42 ++++++++++++++++++ app/client/models/TableData.ts | 16 +++++++ app/client/ui/FieldConfig.ts | 65 +++++++++++++++++++++++++--- 4 files changed, 123 insertions(+), 32 deletions(-) create mode 100644 app/client/models/ColumnCache.ts diff --git a/app/client/models/ColumnACIndexes.ts b/app/client/models/ColumnACIndexes.ts index bf65f21e..0eeb754a 100644 --- a/app/client/models/ColumnACIndexes.ts +++ b/app/client/models/ColumnACIndexes.ts @@ -9,11 +9,10 @@ * It is currently used for auto-complete in the ReferenceEditor widget. */ import {ACIndex, ACIndexImpl} from 'app/client/lib/ACIndex'; +import {ColumnCache} from 'app/client/models/ColumnCache'; import {UserError} from 'app/client/models/errors'; import {TableData} from 'app/client/models/TableData'; -import {DocAction} from 'app/common/DocActions'; -import {isBulkUpdateRecord, isUpdateRecord} from 'app/common/DocActions'; -import {getSetMapValue, localeCompare, nativeCompare} from 'app/common/gutil'; +import {localeCompare, nativeCompare} from 'app/common/gutil'; import {BaseFormatter} from 'app/common/ValueFormatter'; export interface ICellItem { @@ -24,13 +23,9 @@ export interface ICellItem { export class ColumnACIndexes { - private _cachedColIndexes = new Map>(); + private _columnCache = new ColumnCache>(this._tableData); - constructor(private _tableData: TableData) { - // Whenever a table action is applied, consider invalidating per-column caches. - this._tableData.tableActionEmitter.addListener(this._invalidateCache, this); - this._tableData.dataLoadedEmitter.addListener(this._clearCache, this); - } + constructor(private _tableData: TableData) {} /** * Returns the column index for the given column, using a cached one if available. @@ -38,7 +33,7 @@ export class ColumnACIndexes { * getColACIndex() is called for the same column with the the same formatter. */ public getColACIndex(colId: string, formatter: BaseFormatter): ACIndex { - return getSetMapValue(this._cachedColIndexes, colId, () => this._buildColACIndex(colId, formatter)); + return this._columnCache.getValue(colId, () => this._buildColACIndex(colId, formatter)); } private _buildColACIndex(colId: string, formatter: BaseFormatter): ACIndex { @@ -56,23 +51,6 @@ export class ColumnACIndexes { items.sort(itemCompare); return new ACIndexImpl(items); } - - private _invalidateCache(action: DocAction): void { - if (isUpdateRecord(action) || isBulkUpdateRecord(action)) { - // If the update only affects existing records, only invalidate affected columns. - const colValues = action[3]; - for (const colId of Object.keys(colValues)) { - this._cachedColIndexes.delete(colId); - } - } else { - // For add/delete actions and all schema changes, drop the cache entirelly to be on the safe side. - this._clearCache(); - } - } - - private _clearCache(): void { - this._cachedColIndexes.clear(); - } } function itemCompare(a: ICellItem, b: ICellItem) { diff --git a/app/client/models/ColumnCache.ts b/app/client/models/ColumnCache.ts new file mode 100644 index 00000000..60238cd7 --- /dev/null +++ b/app/client/models/ColumnCache.ts @@ -0,0 +1,42 @@ +/** + * Implements a cache of values computed from the data in a Grist column. + */ +import {TableData} from 'app/client/models/TableData'; +import {DocAction} from 'app/common/DocActions'; +import {isBulkUpdateRecord, isUpdateRecord} from 'app/common/DocActions'; +import {getSetMapValue} from 'app/common/gutil'; + +export class ColumnCache { + private _cachedColIndexes = new Map(); + + constructor(private _tableData: TableData) { + // Whenever a table action is applied, consider invalidating per-column caches. + this._tableData.tableActionEmitter.addListener(this._invalidateCache, this); + this._tableData.dataLoadedEmitter.addListener(this._clearCache, this); + } + + /** + * Returns the cached value for the given column, or calculates and caches the value using the + * provided calc() function. + */ + public getValue(colId: string, calc: () => T): T { + return getSetMapValue(this._cachedColIndexes, colId, calc); + } + + private _invalidateCache(action: DocAction): void { + if (isUpdateRecord(action) || isBulkUpdateRecord(action)) { + // If the update only affects existing records, only invalidate affected columns. + const colValues = action[3]; + for (const colId of Object.keys(colValues)) { + this._cachedColIndexes.delete(colId); + } + } else { + // For add/delete actions and all schema changes, drop the cache entirely to be on the safe side. + this._clearCache(); + } + } + + private _clearCache(): void { + this._cachedColIndexes.clear(); + } +} diff --git a/app/client/models/TableData.ts b/app/client/models/TableData.ts index d92b639b..06d71be6 100644 --- a/app/client/models/TableData.ts +++ b/app/client/models/TableData.ts @@ -2,8 +2,11 @@ * TableData maintains a single table's data. */ import {ColumnACIndexes} from 'app/client/models/ColumnACIndexes'; +import {ColumnCache} from 'app/client/models/ColumnCache'; import {DocData} from 'app/client/models/DocData'; import {DocAction, ReplaceTableData, TableDataAction, UserAction} from 'app/common/DocActions'; +import {isRaisedException} from 'app/common/gristTypes'; +import {countIf} from 'app/common/gutil'; import {ColTypeMap, TableData as BaseTableData} from 'app/common/TableData'; import {BaseFormatter} from 'app/common/ValueFormatter'; import {Emitter} from 'grainjs'; @@ -19,6 +22,8 @@ export class TableData extends BaseTableData { public readonly columnACIndexes = new ColumnACIndexes(this); + private _columnErrorCounts = new ColumnCache(this); + /** * Constructor for TableData. * @param {DocData} docData: The root DocData object for this document. @@ -92,6 +97,17 @@ export class TableData extends BaseTableData { return ret; } + /** + * Counts and returns the number of error values in the given column. The count is cached to + * keep it faster for large tables, and the cache is cleared as needed on changes to the table. + */ + public countErrors(colId: string): number|undefined { + return this._columnErrorCounts.getValue(colId, () => { + const values = this.getColValues(colId); + return values && countIf(values, isRaisedException); + }); + } + /** * Sends an array of table-specific action to the server to be applied. The tableId should be * omitted from each `action` parameter and will be inserted automatically. diff --git a/app/client/ui/FieldConfig.ts b/app/client/ui/FieldConfig.ts index 17940ef6..4a7d3fc3 100644 --- a/app/client/ui/FieldConfig.ts +++ b/app/client/ui/FieldConfig.ts @@ -7,9 +7,10 @@ import {textInput} from 'app/client/ui2018/editableLabel'; import {cssIconButton, icon} from 'app/client/ui2018/icons'; import {menu, menuItem} from 'app/client/ui2018/menus'; import {sanitizeIdent} from 'app/common/gutil'; -import {Computed, dom, fromKo, IDisposableOwner, Observable, styled} from 'grainjs'; +import {Computed, dom, fromKo, MultiHolder, Observable, styled, subscribe} from 'grainjs'; +import debounce = require('lodash/debounce'); -export function buildNameConfig(owner: IDisposableOwner, origColumn: ColumnRec) { +export function buildNameConfig(owner: MultiHolder, origColumn: ColumnRec) { const untieColId = origColumn.untieColIdFromLabel; const editedLabel = Observable.create(owner, ''); @@ -50,10 +51,11 @@ export function buildNameConfig(owner: IDisposableOwner, origColumn: ColumnRec) type BuildEditor = (cellElem: Element) => void; export function buildFormulaConfig( - owner: IDisposableOwner, origColumn: ColumnRec, gristDoc: GristDoc, buildEditor: BuildEditor + owner: MultiHolder, origColumn: ColumnRec, gristDoc: GristDoc, buildEditor: BuildEditor ) { const clearColumn = () => gristDoc.clearColumns([origColumn.id.peek()]); const convertToData = () => gristDoc.convertFormulasToData([origColumn.id.peek()]); + const errorMessage = createFormulaErrorObs(owner, gristDoc, origColumn); return dom.maybe(use => { if (!use(origColumn.id)) { return null; } // Invalid column, show nothing. @@ -73,7 +75,10 @@ export function buildFormulaConfig( ); } function buildFormulaRow(placeholder = 'Enter formula') { - return cssRow(dom.create(buildFormula, origColumn, buildEditor, placeholder)); + return [ + cssRow(dom.create(buildFormula, origColumn, buildEditor, placeholder)), + dom.maybe(errorMessage, errMsg => cssRow(cssError(errMsg), testId('field-error-count'))), + ]; } if (type === "empty") { return [ @@ -104,7 +109,7 @@ export function buildFormulaConfig( ); } -function buildFormula(owner: IDisposableOwner, column: ColumnRec, buildEditor: BuildEditor, placeholder: string) { +function buildFormula(owner: MultiHolder, column: ColumnRec, buildEditor: BuildEditor, placeholder: string) { return cssFieldFormula(column.formula, {placeholder, maxLines: 2}, dom.cls('formula_field_sidepane'), cssFieldFormula.cls('-disabled', column.disableModify), @@ -115,6 +120,52 @@ function buildFormula(owner: IDisposableOwner, column: ColumnRec, buildEditor: B ); } +/** + * Create and return an observable for the count of errors in a column, which gets updated in + * response to changes in origColumn and in user data. + */ +function createFormulaErrorObs(owner: MultiHolder, gristDoc: GristDoc, origColumn: ColumnRec) { + const errorMessage = Observable.create(owner, ''); + + // Count errors in origColumn when it's a formula column. Counts get cached by the + // tableData.countErrors() method, and invalidated on relevant data changes. + function countErrors() { + if (owner.isDisposed()) { return; } + const tableData = gristDoc.docData.getTable(origColumn.table.peek().tableId.peek()); + if (tableData && origColumn.isRealFormula.peek()) { + const colId = origColumn.colId.peek(); + const numCells = tableData.getColValues(colId)?.length || 0; + const numErrors = tableData.countErrors(colId) || 0; + errorMessage.set( + (numErrors === 0) ? '' : + (numCells === 1) ? `Error in the cell` : + (numErrors === numCells) ? `Errors in all ${numErrors} cells` : + `Errors in ${numErrors} of ${numCells} cells` + ); + } else { + errorMessage.set(''); + } + } + + // Debounce the count calculation to defer it to the end of a bundle of actions. + const debouncedCountErrors = debounce(countErrors, 0); + + // If there is an update to the data in the table, count errors again. Since the same UI is + // reused when different page widgets are selected, we need to re-create this subscription + // whenever the selected table changes. We use a Computed to both react to changes and dispose + // the previous subscription when it changes. + Computed.create(owner, (use) => { + const tableData = gristDoc.docData.getTable(use(use(origColumn.table).tableId)); + return tableData ? use.owner.autoDispose(tableData.tableActionEmitter.addListener(debouncedCountErrors)) : null; + }); + + // The counts depend on the origColumn and its isRealFormula status, but with the debounced + // callback and subscription to data, subscribe to relevant changes manually (rather than using + // a Computed). + owner.autoDispose(subscribe(use => { use(origColumn.id); use(origColumn.isRealFormula); debouncedCountErrors(); })); + return errorMessage; +} + const cssFieldFormula = styled(buildHighlightedCode, ` flex: auto; cursor: pointer; @@ -195,3 +246,7 @@ const cssColTieConnectors = styled('div', ` border-left: none; z-index: -1; `); + +const cssError = styled('div', ` + color: ${colors.error}; +`);