(core) Allow adding rows to widgets filtered by a link using a formula column

Summary:
When a widget `A` is selected by a widget `B` so that `A` is filtered, adding a new row to `A` uses the values in the selected row of `B` and the columns relevant to the linking as default values for the new row. This ensures that the new row matches the current linking filter and remains visible. However this would previously cause a sandbox error when one of the linking columns was a formula column, which doesn't allow setting values. This diff ignores formula columns when picking default values.

Since the value of the formula column in the new row typically won't match the linking filter, extra measures are needed to avoid the new row immediately disappearing. Regular filters already have a mechanism for this, but I didn't manage to extend it to also work for linking. Thanks @dsagal for creating `UnionRowSource` (originally in D4017) which is now used as the solution for temporarily exempting rows from both kinds of filtering.

While testing, I also came across another bug in linking summary tables that caused incorrect filtering, which I fixed with some changes to `DynamicQuerySet`.

Test Plan: Extended an nbrowser test, which both tests for the main change as well as the secondary bugfix.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Subscribers: dsagal

Differential Revision: https://phab.getgrist.com/D4135

app/client/models/QuerySet.ts

@@ -117,6 +117,22 @@ export class QuerySetManager extends Disposable {
  * RowSource.
  */
 export class DynamicQuerySet extends RowSource {
+  /**
+   * Replace the query represented by this DynamicQuerySet. If multiple makeQuery() calls are made
+   * quickly (while waiting for the server), cb() may only be called for the latest one.
+   *
+   * If there is an error fetching data, cb(err) will be called with that error. The second
+   * argument to cb() is true if any data was changed, and false if not. Note that for a series of
+   * makeQuery() calls, cb() is always called at least once, and always asynchronously.
+   *
+   * It's possible for this to be called very quickly in succession,
+   * e.g. when selecting another row of a linked summary table grouped by multiple columns,
+   * as an observable gets triggered for each one.
+   * We only want to keep the last call, and _updateQuerySetDebounced may not be called
+   * in the correct order, so we first debounce here.
+   */
+  public readonly makeQuery = debounce(tbind(this._makeQuery, this), 0);
+
   // Holds a reference to the currently active QuerySet.
   private _holder = Holder.create<IRefCountSub<QuerySet>>(this);
 
@@ -147,17 +163,13 @@ export class DynamicQuerySet extends RowSource {
     return this._querySet ? this._querySet.isTruncated : false;
   }
 
-  /**
-   * Replace the query represented by this DynamicQuerySet. If multiple makeQuery() calls are made
-   * quickly (while waiting for the server), cb() may only be called for the latest one.
-   *
-   * If there is an error fetching data, cb(err) will be called with that error. The second
-   * argument to cb() is true if any data was changed, and false if not. Note that for a series of
-   * makeQuery() calls, cb() is always called at least once, and always asynchronously.
-   */
-  public makeQuery(filters: {[colId: string]: any[]},
-                   operations: {[colId: string]: QueryOperation},
-                   cb: (err: Error|null, changed: boolean) => void): void {
+  private _makeQuery(filters: { [colId: string]: any[] },
+                     operations: {[colId: string]: QueryOperation},
+                     cb: (err: Error|null, changed: boolean) => void): void {
+    if (this.isDisposed()) {
+      cb(new Error('Disposed'), false);
+      return;
+    }
     const query: ClientQuery = {tableId: this._tableModel.tableData.tableId, filters, operations};
     const newQuerySet = this._querySetManager.useQuerySet(this._holder, query);
 

app/client/models/SectionFilter.ts

@@ -4,7 +4,7 @@ import {TableData} from 'app/client/models/TableData';
 import {buildColFilter, ColumnFilterFunc} from 'app/common/ColumnFilterFunc';
 import {buildRowFilter, RowFilterFunc, RowValueFunc } from 'app/common/RowFilterFunc';
 import {UIRowId} from 'app/plugin/GristAPI';
-import {Computed, Disposable, MutableObsArray, obsArray, Observable, UseCB} from 'grainjs';
+import {Computed, Disposable, Observable, UseCB} from 'grainjs';
 
 export type {ColumnFilterFunc};
 
@@ -21,20 +21,21 @@ type ColFilterCB = (fieldOrColumn: ViewFieldRec|ColumnRec, colFilter: ColumnFilt
  * subscribe to in order to update their FilteredRowSource.
  *
  * Additionally, `setFilterOverride()` provides a way to override the current filter for a given colRef,
- * to reflect the changes in an open filter dialog. Also, `addTemporaryRow()` allows to add a rowId
- * that should be present regardless of filters. These rows are removed automatically when an update to the filter
- * results in their being displayed (obviating the need to maintain their rowId explicitly).
+ * to reflect the changes in an open filter dialog.
  */
 export class SectionFilter extends Disposable {
   public readonly sectionFilterFunc: Observable<RowFilterFunc<UIRowId>>;
 
   private _openFilterOverride: Observable<OpenColumnFilter|null> = Observable.create(this, null);
-  private _tempRows: MutableObsArray<UIRowId> = obsArray();
 
-  constructor(public viewSection: ViewSectionRec, private _tableData: TableData) {
+  constructor(
+    public viewSection: ViewSectionRec,
+    private _tableData: TableData,
+    private _resetExemptRows: () => void,
+  ) {
     super();
 
-    const columnFilterFunc = Computed.create(this, this._openFilterOverride, (use, openFilter) => {
+    this.sectionFilterFunc = Computed.create(this, this._openFilterOverride, (use, openFilter) => {
       const openFilterFilterFunc = openFilter && use(openFilter.colFilter.filterFunc);
       function getFilterFunc(fieldOrColumn: ViewFieldRec|ColumnRec, colFilter: ColumnFilterFunc|null) {
         if (openFilter?.colRef === fieldOrColumn.origCol().getRowId()) {
@@ -42,16 +43,8 @@ export class SectionFilter extends Disposable {
         }
         return colFilter;
       }
-      return this._buildPlainFilterFunc(getFilterFunc, use);
+      return this.buildFilterFunc(getFilterFunc, use);
     });
-
-    this.sectionFilterFunc = Computed.create(this, columnFilterFunc, this._tempRows,
-      (_use, filterFunc, tempRows) => this._addRowsToFilter(filterFunc, tempRows));
-
-    // Prune temporary rowIds that are no longer being filtered out.
-    this.autoDispose(columnFilterFunc.addListener(f => {
-      this._tempRows.set(this._tempRows.get().filter(rowId => !f(rowId)));
-    }));
   }
 
   /**
@@ -68,37 +61,14 @@ export class SectionFilter extends Disposable {
     });
   }
 
-  public addTemporaryRow(rowId: number) {
-    // Only add the rowId if it would otherwise be filtered out
-    if (!this.sectionFilterFunc.get()(rowId)) {
-      this._tempRows.push(rowId);
-    }
-  }
-
-  public resetTemporaryRows() {
-    this._tempRows.set([]);
-  }
-
   /**
    * Builds a filter function that combines the filter function of all the columns. You can use
-   * `getFilterFunc(column, colFilter)` to customize the filter func for each columns. It calls
-   * `getFilterFunc` right away. Also, all the rows that were added with `addTemporaryRow()` bypass
-   * the filter.
+   * `getFilterFunc(column, colFilter)` to customize the filter func for each column. It calls
+   * `getFilterFunc` right away.
+   * This also immediately resets rows that were temporarily exempted from filtering.
    */
   public buildFilterFunc(getFilterFunc: ColFilterCB, use: UseCB) {
-    return this._addRowsToFilter(this._buildPlainFilterFunc(getFilterFunc, use), this._tempRows.get());
-  }
-
-  private _addRowsToFilter(filterFunc: RowFilterFunc<UIRowId>, rows: UIRowId[]) {
-    return (rowId: UIRowId) => rows.includes(rowId) || (typeof rowId !== 'number') || filterFunc(rowId);
-  }
-
-  /**
-   * Internal that helps build a filter function that combines the filter function of all
-   * columns. You can use `getFilterFunc(column, colFilter)` to customize the filter func for each
-   * column. It calls `getFilterFunc` right away.
-   */
-  private _buildPlainFilterFunc(getFilterFunc: ColFilterCB, use: UseCB): RowFilterFunc<UIRowId> {
+    this._resetExemptRows();
     const filters = use(this.viewSection.filters);
     const funcs: Array<RowFilterFunc<UIRowId> | null> = filters.map(({filter, fieldOrColumn}) => {
       const colFilter = buildColFilter(use(filter), use(use(fieldOrColumn.origCol).type));
@@ -111,6 +81,6 @@ export class SectionFilter extends Disposable {
       return buildRowFilter(getter as RowValueFunc<UIRowId>, filterFunc);
     }).filter(f => f !== null); // Filter out columns that don't have a filter
 
-    return (rowId: UIRowId) => funcs.every(f => Boolean(f && f(rowId)));
+    return (rowId: UIRowId) => rowId === 'new' || funcs.every(f => Boolean(f && f(rowId)));
   }
 }

app/client/models/UnionRowSource.ts

@@ -0,0 +1,59 @@
+import {RowList, RowListener, RowSource} from 'app/client/models/rowset';
+import {UIRowId} from "app/plugin/GristAPI";
+
+export class UnionRowSource extends RowListener implements RowSource {
+  protected _allRows = new Map<UIRowId, Set<RowSource>>();
+
+  constructor(parentRowSources: RowSource[]) {
+    super();
+    for (const p of parentRowSources) {
+      this.subscribeTo(p);
+    }
+  }
+
+  public getAllRows(): RowList {
+    return this._allRows.keys();
+  }
+
+  public getNumRows(): number {
+    return this._allRows.size;
+  }
+
+  public onAddRows(rows: RowList, rowSource: RowSource) {
+    const outputRows = [];
+    for (const r of rows) {
+      let sources = this._allRows.get(r);
+      if (!sources) {
+        sources = new Set();
+        this._allRows.set(r, sources);
+        outputRows.push(r);
+      }
+      sources.add(rowSource);
+    }
+    if (outputRows.length > 0) {
+      this.trigger('rowChange', 'add', outputRows);
+    }
+  }
+
+  public onRemoveRows(rows: RowList, rowSource: RowSource) {
+    const outputRows = [];
+    for (const r of rows) {
+      const sources = this._allRows.get(r);
+      if (!sources) {
+        continue;
+      }
+      sources.delete(rowSource);
+      if (sources.size === 0) {
+        outputRows.push(r);
+        this._allRows.delete(r);
+      }
+    }
+    if (outputRows.length > 0) {
+      this.trigger('rowChange', 'remove', outputRows);
+    }
+  }
+
+  public onUpdateRows(rows: RowList) {
+    this.trigger('rowChange', 'update', rows);
+  }
+}

app/client/models/rowset.ts

@@ -82,10 +82,10 @@ export class RowListener extends DisposableWithEvents {
    * Subscribes to the given rowSource and adds the rows currently in it.
    */
   public subscribeTo(rowSource: RowSource): void {
-    this.onAddRows(rowSource.getAllRows());
+    this.onAddRows(rowSource.getAllRows(), rowSource);
     this.listenTo(rowSource, 'rowChange', (changeType: ChangeType, rows: RowList) => {
       const method: ChangeMethod = _changeTypes[changeType];
-      this[method](rows);
+      this[method](rows, rowSource);
     });
     this.listenTo(rowSource, 'rowNotify', this.onRowNotify);
   }
@@ -103,17 +103,17 @@ export class RowListener extends DisposableWithEvents {
   /**
    * Process row additions. To be implemented by derived classes.
    */
-  protected onAddRows(rows: RowList) { /* no-op */ }
+  protected onAddRows(rows: RowList, rowSource?: RowSource) { /* no-op */ }
 
   /**
    * Process row removals. To be implemented by derived classes.
    */
-  protected onRemoveRows(rows: RowList) { /* no-op */ }
+  protected onRemoveRows(rows: RowList, rowSource?: RowSource) { /* no-op */ }
 
   /**
    * Process row updates. To be implemented by derived classes.
    */
-  protected onUpdateRows(rows: RowList) { /* no-op */ }
+  protected onUpdateRows(rows: RowList, rowSource?: RowSource) { /* no-op */ }
 
   /**
    * Derived classes may override this event to handle row notifications. By default, it re-triggers
@@ -128,15 +128,6 @@ export class RowListener extends DisposableWithEvents {
 // MappedRowSource
 // ----------------------------------------------------------------------
 
-/**
- * A trivial RowSource returning a fixed list of rows.
- */
-export abstract class ArrayRowSource extends RowSource {
-  constructor(private _rows: UIRowId[]) { super(); }
-  public getAllRows(): RowList { return this._rows; }
-  public getNumRows(): number { return this._rows.length; }
-}
-
 /**
  * MappedRowSource wraps any other RowSource, and passes through all rows, replacing each row
  * identifier with the result of mapperFunc(row) call.
@@ -773,3 +764,42 @@ function _allRowsSorted<T>(array: T[], allRows: Set<T>, sortedRows: Iterable<T>,
   }
   return true;
 }
+
+
+/**
+ * Track rows that should temporarily be visible even if they don't match filters.
+ * This is so that a newly added row doesn't immediately disappear, which would be confusing.
+ * This doesn't have much to do with BaseFilteredRowSource, it's just reusing some implementation.
+ */
+export class ExemptFromFilterRowSource extends BaseFilteredRowSource {
+  public constructor() {
+    super(() => false);
+  }
+
+  /**
+   * Call this when one or more new rows are added to keep them temporarily visible.
+   */
+  public addExemptRows(rows: RowList) {
+    const newRows = [];
+    for (const r of rows) {
+      if (!this._matchingRows.has(r)) {
+        this._matchingRows.add(r);
+        newRows.push(r);
+      }
+    }
+    if (newRows.length > 0) {
+      this.trigger('rowChange', 'add', newRows);
+    }
+  }
+
+  public addExemptRow(rowId: number) {
+    this.addExemptRows([rowId]);
+  }
+
+  /**
+   * Call this when linking or filters change to clear out the temporary rows.
+   */
+  public reset() {
+    this.onRemoveRows(this.getAllRows());
+  }
+}