(core) Reference and ReferenceList formatters

Summary:
Previously, ref/reflist columns were formatted entirely based on their visible column, since they received values from the visible or display columns rather than the actual row IDs. This creates `ReferenceFormatter` and `ReferenceListFormatter` which still delegate most of the formatting work to a visible column formatter but fix a few issues:

- ReferenceList columns now actually use the options (e.g. date format) of the visible column to format their elements. Previously they were formatted generically because the visible column formatter wasn't expecting a list.
- Invalid references aren't formatted with an `#Invalid Ref` prefix.
- When the ref column displays the Row ID, it doesn't have a visible or display column. Previously this led to the references being formatted as just numbers in most cases, with special code in the widget to display them like `Table1[2]`. Now they are consistently formatted in that style throughout.

Test Plan: Updated existing tests.

Reviewers: jarek

Reviewed By: jarek

Subscribers: dsagal

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

app/client/components/ChartView.ts

@@ -897,7 +897,7 @@ export const chartTypes: {[name: string]: ChartFunc} = {
 
     function format(val: number) {
       if (dataOptions.totalFormatter) {
-        return dataOptions.totalFormatter.format(val);
+        return dataOptions.totalFormatter.formatAny(val);
       }
       return String(val);
     }

app/client/components/CopySelection.ts

@@ -33,7 +33,7 @@ export class CopySelection {
     this.rowStyle = options.rowStyle;
     this.colStyle = options.colStyle;
     this.columns = fields.map((f, i) => {
-      const formatter = f.visibleColFormatter();
+      const formatter = f.formatter();
       const _fmtGetter = tableData.getRowPropFunc(this.displayColIds[i])!;
       const _rawGetter = tableData.getRowPropFunc(this.colIds[i])!;
 

app/client/lib/ReferenceUtils.ts

@@ -11,7 +11,7 @@ import {BaseFormatter} from 'app/common/ValueFormatter';
 export class ReferenceUtils {
   public readonly refTableId: string;
   public readonly tableData: TableData;
-  public readonly formatter: BaseFormatter;
+  public readonly visibleColFormatter: BaseFormatter;
   public readonly visibleColModel: ColumnRec;
   public readonly visibleColId: string;
   public readonly isRefList: boolean;
@@ -30,7 +30,7 @@ export class ReferenceUtils {
     }
     this.tableData = tableData;
 
-    this.formatter = field.visibleColFormatter();
+    this.visibleColFormatter = field.visibleColFormatter();
     this.visibleColModel = field.visibleColModel();
     this.visibleColId = this.visibleColModel.colId() || 'id';
     this.isRefList = isRefListType(colType);
@@ -38,13 +38,13 @@ export class ReferenceUtils {
 
   public idToText(value: unknown) {
     if (typeof value === 'number') {
-      return this.formatter.formatAny(this.tableData.getValue(value, this.visibleColId));
+      return this.visibleColFormatter.formatAny(this.tableData.getValue(value, this.visibleColId));
     }
     return String(value || '');
   }
 
   public autocompleteSearch(text: string) {
-    const acIndex = this.tableData.columnACIndexes.getColACIndex(this.visibleColId, this.formatter);
+    const acIndex = this.tableData.columnACIndexes.getColACIndex(this.visibleColId, this.visibleColFormatter);
     return acIndex.search(text);
   }
 }

app/client/models/SearchModel.ts

@@ -218,7 +218,7 @@ class FinderImpl implements IFinder {
     this._sectionTableData = tableModel.tableData;
 
     this._fieldStepper.array = section.viewFields().peek();
-    this._fieldFormatters = this._fieldStepper.array.map(f => f.visibleColFormatter.peek());
+    this._fieldFormatters = this._fieldStepper.array.map(f => f.formatter.peek());
     return tableModel;
   }
 

app/client/models/entities/ColumnRec.ts

@@ -2,7 +2,7 @@ import {KoArray} from 'app/client/lib/koArray';
 import {DocModel, IRowModel, recordSet, refRecord, TableRec, ViewFieldRec} from 'app/client/models/DocModel';
 import {jsonObservable, ObjObservable} from 'app/client/models/modelUtil';
 import * as gristTypes from 'app/common/gristTypes';
-import {getReferencedTableId} from 'app/common/gristTypes';
+import {getReferencedTableId, isFullReferencingType} from 'app/common/gristTypes';
 import {BaseFormatter, createFormatter} from 'app/common/ValueFormatter';
 import * as ko from 'knockout';
 
@@ -56,6 +56,13 @@ export interface ColumnRec extends IRowModel<"_grist_Tables_column"> {
   // to the visibleCol associated with column.
   visibleColFormatter: ko.Computed<BaseFormatter>;
 
+  // A formatter for values of this column.
+  // The difference between visibleColFormatter and formatter is especially important for ReferenceLists:
+  // `visibleColFormatter` is for individual elements of a list, sometimes hypothetical
+  // (i.e. they aren't actually referenced but they exist in the visible column and are relevant to e.g. autocomplete)
+  // `formatter` formats actual cell values, e.g. a whole list from the display column.
+  formatter: ko.Computed<BaseFormatter>;
+
   // Helper which adds/removes/updates column's displayCol to match the formula.
   saveDisplayFormula(formula: string): Promise<void>|undefined;
 }
@@ -118,6 +125,8 @@ export function createColumnRec(this: ColumnRec, docModel: DocModel): void {
   // Helper for Reference/ReferenceList columns, which returns a formatter according to the visibleCol
   // associated with this column. If no visible column available, return formatting for the column itself.
   this.visibleColFormatter = ko.pureComputed(() => visibleColFormatterForRec(this, this, docModel));
+
+  this.formatter = ko.pureComputed(() => formatterForRec(this, this, docModel, this.visibleColFormatter()));
 }
 
 export function visibleColFormatterForRec(
@@ -125,7 +134,28 @@ export function visibleColFormatterForRec(
 ): BaseFormatter {
   const vcol = rec.visibleColModel();
   const documentSettings = docModel.docInfoRow.documentSettingsJson();
-  return (vcol.getRowId() !== 0) ?
-    createFormatter(vcol.type(), vcol.widgetOptionsJson(), documentSettings) :
-    createFormatter(colRec.type(), rec.widgetOptionsJson(), documentSettings);
+  const type = colRec.type();
+  if (isFullReferencingType(type)) {
+    if (vcol.getRowId() === 0) {
+      // This column displays the Row ID, e.g. Table1[2]
+      // referencedTableId may actually be empty if the table is hidden
+      const referencedTableId: string = colRec.refTable()?.tableId() || "";
+      return createFormatter('Id', {tableId: referencedTableId}, documentSettings);
+    } else {
+      return createFormatter(vcol.type(), vcol.widgetOptionsJson(), documentSettings);
+    }
+  } else {
+    // For non-reference columns, there's no 'visible column' and we just return a regular formatter
+    return createFormatter(type, rec.widgetOptionsJson(), documentSettings);
+  }
+}
+
+export function formatterForRec(
+  rec: ColumnRec | ViewFieldRec, colRec: ColumnRec, docModel: DocModel, visibleColFormatter: BaseFormatter
+): BaseFormatter {
+  const type = colRec.type();
+  // Ref/RefList columns delegate most formatting to the visibleColFormatter
+  const widgetOpts = {...rec.widgetOptionsJson(), visibleColFormatter};
+  const documentSettings = docModel.docInfoRow.documentSettingsJson();
+  return createFormatter(type, widgetOpts, documentSettings);
 }

app/client/models/entities/ViewFieldRec.ts

@@ -1,5 +1,5 @@
 import {ColumnRec, DocModel, IRowModel, refRecord, ViewSectionRec} from 'app/client/models/DocModel';
-import {visibleColFormatterForRec} from 'app/client/models/entities/ColumnRec';
+import {formatterForRec, visibleColFormatterForRec} from 'app/client/models/entities/ColumnRec';
 import * as modelUtil from 'app/client/models/modelUtil';
 import * as UserType from 'app/client/widgets/UserType';
 import {DocumentSettings} from 'app/common/DocumentSettings';
@@ -74,6 +74,13 @@ export interface ViewFieldRec extends IRowModel<"_grist_Views_section_field"> {
   // to the visibleCol associated with field.
   visibleColFormatter: ko.Computed<BaseFormatter>;
 
+  // A formatter for values of this column.
+  // The difference between visibleColFormatter and formatter is especially important for ReferenceLists:
+  // `visibleColFormatter` is for individual elements of a list, sometimes hypothetical
+  // (i.e. they aren't actually referenced but they exist in the visible column and are relevant to e.g. autocomplete)
+  // `formatter` formats actual cell values, e.g. a whole list from the display column.
+  formatter: ko.Computed<BaseFormatter>;
+
   createValueParser(): (value: string) => any;
 
   // Helper which adds/removes/updates field's displayCol to match the formula.
@@ -167,6 +174,8 @@ export function createViewFieldRec(this: ViewFieldRec, docModel: DocModel): void
   // associated with this field. If no visible column available, return formatting for the field itself.
   this.visibleColFormatter = ko.pureComputed(() => visibleColFormatterForRec(this, this.column(), docModel));
 
+  this.formatter = ko.pureComputed(() => formatterForRec(this, this.column(), docModel, this.visibleColFormatter()));
+
   this.createValueParser = function() {
     const fieldRef = this.useColOptions.peek() ? undefined : this.id.peek();
     return createParser(docModel.docData, this.colRef.peek(), fieldRef);

app/client/widgets/ChoiceTextBox.ts

@@ -49,7 +49,7 @@ export class ChoiceTextBox extends NTextBox {
         dom.domComputed((use) => {
           if (this.isDisposed() || use(row._isAddRow)) { return null; }
 
-          const formattedValue = use(this.valueFormatter).format(use(value));
+          const formattedValue = use(this.valueFormatter).formatAny(use(value));
           if (formattedValue === '') { return null; }
 
           const choiceOptions = use(this._choiceOptionsByName).get(formattedValue);

app/client/widgets/NTextBox.ts

@@ -45,7 +45,7 @@ export class NTextBox extends NewAbstractWidget {
     return dom('div.field_clip',
       dom.style('text-align', this.alignment),
       dom.cls('text_wrapping', this.wrapping),
-      dom.domComputed((use) => use(row._isAddRow) ? null : makeLinks(use(this.valueFormatter).format(use(value))))
+      dom.domComputed((use) => use(row._isAddRow) ? null : makeLinks(use(this.valueFormatter).formatAny(use(value))))
     );
   }
 

app/client/widgets/NewAbstractWidget.ts

@@ -43,7 +43,7 @@ export abstract class NewAbstractWidget extends Disposable {
     )).onWrite((val) => this.field.textColor(val === defaultTextColor ? undefined : val));
     this.fillColor = fromKo(this.field.fillColor);
 
-    this.valueFormatter = fromKo(field.visibleColFormatter);
+    this.valueFormatter = fromKo(field.formatter);
   }
 
   /**

app/client/widgets/Reference.ts

@@ -12,8 +12,6 @@ import {Computed, dom, styled} from 'grainjs';
  * Reference - The widget for displaying references to another table's records.
  */
 export class Reference extends NTextBox {
-  protected _formatValue: Computed<(val: any) => string>;
-
   private _visibleColRef: Computed<number>;
   private _validCols: Computed<Array<IOptionFull<number>>>;
 
@@ -37,19 +35,6 @@ export class Reference extends NTextBox {
         }))
         .concat([{label: 'Row ID', value: 0, icon: 'FieldColumn'}]);
     });
-
-    // Computed returns a function that formats cell values.
-    this._formatValue = Computed.create(this, (use) => {
-      // If the field is pulling values from a display column, use a general-purpose formatter.
-      if (use(this.field.displayColRef) !== use(this.field.colRef)) {
-        const fmt = use(this.field.visibleColFormatter);
-        return (val: any) => fmt.formatAny(val);
-      } else {
-        const refTable = use(use(this.field.column).refTable);
-        const refTableId = refTable ? use(refTable.tableId) : "";
-        return (val: any) => val > 0 ? `${refTableId}[${val}]` : "";
-      }
-    });
   }
 
   public buildConfigDom() {
@@ -100,8 +85,8 @@ export class Reference extends NTextBox {
         // but the content of its displayCol has changed.  Postponing doing anything about
         // this until we have three-way information for computed columns.  For now,
         // just showing one version of the cell.  TODO: elaborate.
-        use(this._formatValue)(displayValue[1].local || displayValue[1].parent) :
-        use(this._formatValue)(displayValue);
+        use(this.field.formatter).formatAny(displayValue[1].local || displayValue[1].parent) :
+        use(this.field.formatter).formatAny(displayValue);
 
       hasBlankReference = referenceId.get() !== 0 && value.trim() === '';
 

app/client/widgets/ReferenceList.ts

@@ -37,7 +37,10 @@ export class ReferenceList extends Reference {
         //   return use(this._formatValue)(content[1].local || content[1].parent);
         // }
         const items = isList(content) ? content.slice(1) : [content];
-        return items.map(use(this._formatValue));
+        // Use field.visibleColFormatter instead of field.formatter
+        // because we're formatting each list element to render tokens, not the whole list.
+        const formatter = use(this.field.visibleColFormatter);
+        return items.map(item => formatter.formatAny(item));
       },
       (input) => {
         if (!input) {

app/common/ValueFormatter.ts

@@ -3,6 +3,7 @@
 import {csvEncodeRow} from 'app/common/csvFormat';
 import {CellValue} from 'app/common/DocActions';
 import {DocumentSettings} from 'app/common/DocumentSettings';
+import {getReferencedTableId, isList} from 'app/common/gristTypes';
 import * as gristTypes from 'app/common/gristTypes';
 import * as gutil from 'app/common/gutil';
 import {buildNumberFormat, NumberFormatOptions} from 'app/common/NumberFormat';
@@ -24,15 +25,24 @@ export function formatUnknown(value: CellValue): string {
   return formatDecoded(decodeObject(value));
 }
 
+/**
+ * Returns true if the array contains other arrays or structured objects,
+ * indicating that the list should be formatted like JSON rather than CSV.
+ */
+function hasNestedObjects(value: any[]) {
+  return value.some(v => typeof v === 'object' && v && (Array.isArray(v) || isPlainObject(v)));
+}
+
 /**
  * Formats a decoded Grist value for displaying it. For top-level values, formats them the way we
- * like to see them in a cell or in, say, CSV export. For lists and objects, nested values are
- * formatted slighly differently, with quoted strings and ISO format for dates.
+ * like to see them in a cell or in, say, CSV export.
+ * For top-level lists containing only simple values like strings and dates, formats them as a CSV row.
+ * Nested lists and objects are formatted slighly differently, with quoted strings and ISO format for dates.
  */
 export function formatDecoded(value: unknown, isTopLevel: boolean = true): string {
   if (typeof value === 'object' && value) {
     if (Array.isArray(value)) {
-      if (!isTopLevel || value.some(v => typeof v === 'object' && v && (Array.isArray(v) || isPlainObject(v)))) {
+      if (!isTopLevel || hasNestedObjects(value)) {
         return '[' + value.map(v => formatDecoded(v, false)).join(', ') + ']';
       } else {
         return csvEncodeRow(value.map(v => formatDecoded(v, true)), {prettier: true});
@@ -62,14 +72,6 @@ export class BaseFormatter {
       gristTypes.isRightType('Any')!;
   }
 
-  /**
-   * Formats a value that matches the type of this formatter. This should be overridden by derived
-   * classes to handle values in formatter-specific ways.
-   */
-  public format(value: any): string {
-    return String(value);
-  }
-
   /**
    * Formats using this.format() if a value is of the right type for this formatter, or using
    * AnyFormatter otherwise. This method the recommended API. There is no need to override it.
@@ -77,6 +79,14 @@ export class BaseFormatter {
   public formatAny(value: any): string {
     return this.isRightType(value) ? this.format(value) : formatUnknown(value);
   }
+
+  /**
+   * Formats a value that matches the type of this formatter. This should be overridden by derived
+   * classes to handle values in formatter-specific ways.
+   */
+  protected format(value: any): string {
+    return String(value);
+  }
 }
 
 class AnyFormatter extends BaseFormatter {
@@ -178,12 +188,86 @@ class DateTimeFormatter extends DateFormatter {
   }
 }
 
-const formatters: {[name: string]: typeof BaseFormatter} = {
+class RowIdFormatter extends BaseFormatter {
+  public widgetOpts: { tableId: string };
+
+  public format(value: number): string {
+    return value > 0 ? `${this.widgetOpts.tableId}[${value}]` : "";
+  }
+}
+
+interface ReferenceFormatOptions {
+  visibleColFormatter?: BaseFormatter;
+}
+
+class ReferenceFormatter extends BaseFormatter {
+  public widgetOpts: ReferenceFormatOptions;
+  protected visibleColFormatter: BaseFormatter;
+
+  constructor(type: string, widgetOpts: ReferenceFormatOptions, docSettings: DocumentSettings) {
+    super(type, widgetOpts, docSettings);
+    // widgetOpts.visibleColFormatter shouldn't be undefined, but it can be if a referencing column
+    // is displaying another referencing column, which is partially prohibited in the UI but still possible.
+    this.visibleColFormatter = widgetOpts.visibleColFormatter ||
+      createFormatter('Id', {tableId: getReferencedTableId(type)}, docSettings);
+  }
+
+  public formatAny(value: any): string {
+    /*
+    An invalid value in a referencing column is saved as a string and becomes AltText in the data engine.
+    Then the display column formula (e.g. $person.first_name) raises an InvalidTypedValue trying to access
+    an attribute of that AltText.
+    This would normally lead to the formatter displaying `#Invalid Ref[List]: ` before the string value.
+    That's inconsistent with how the cell is displayed (just the string value in pink)
+    and with how invalid values in other columns are formatted (just the string).
+    It's just a result of the formatter receiving a value from the display column, not the actual column.
+    It's also likely to inconvenience users trying to import/migrate/convert data.
+    So we suppress the error here and just show the text.
+    It's still technically possible for the column to display an actual InvalidTypedValue exception from a formula
+    and this will suppress that too, but this is unlikely and seems worth it.
+    */
+    if (
+      Array.isArray(value)
+      && value[0] === GristObjCode.Exception
+      && value[1] === "InvalidTypedValue"
+      && value[2]?.startsWith?.("Ref")
+    ) {
+      return value[3];
+    }
+    return this.formatNotInvalidRef(value);
+  }
+
+  protected formatNotInvalidRef(value: any) {
+    return this.visibleColFormatter.formatAny(value);
+  }
+}
+
+class ReferenceListFormatter extends ReferenceFormatter {
+  protected formatNotInvalidRef(value: any): string {
+    // Part of this repeats the logic in BaseFormatter.formatAny which is overridden in ReferenceFormatter
+    // It also ensures that complex lists (e.g. if this RefList is displaying a ChoiceList)
+    // are formatted as JSON instead of CSV.
+    if (!isList(value) || hasNestedObjects(value)) {
+      return formatUnknown(value);
+    }
+    // In the most common case, lists of simple objects like strings or dates
+    // are formatted like a CSV.
+    // This is similar to formatUnknown except the inner values are
+    // formatted according to the visible column options.
+    const formattedValues = value.slice(1).map(v => super.formatNotInvalidRef(v));
+    return csvEncodeRow(formattedValues, {prettier: true});
+  }
+}
+
+const formatters: { [name: string]: typeof BaseFormatter } = {
   Numeric: NumericFormatter,
   Int: IntFormatter,
   Bool: BaseFormatter,
   Date: DateFormatter,
   DateTime: DateTimeFormatter,
+  Ref: ReferenceFormatter,
+  RefList: ReferenceListFormatter,
+  Id: RowIdFormatter,
   // We don't list anything that maps to AnyFormatter, since that's the default.
 };
 

app/common/ValueParser.ts

@@ -11,6 +11,7 @@ import NumberParse from 'app/common/NumberParse';
 import {parseDateStrict, parseDateTime} from 'app/common/parseDate';
 import {MetaRowRecord, TableData} from 'app/common/TableData';
 import {DateFormatOptions, DateTimeFormatOptions, formatDecoded, FormatOptions} from 'app/common/ValueFormatter';
+import {encodeObject} from 'app/plugin/objtypes';
 import flatMap = require('lodash/flatMap');
 import mapValues = require('lodash/mapValues');
 
@@ -177,7 +178,7 @@ export class ReferenceListParser extends ReferenceParser {
       // csvDecodeRow should never raise an exception
       values = csvDecodeRow(raw);
     }
-    values = values.map(v => typeof v === "string" ? this._visibleColParser(v) : v);
+    values = values.map(v => typeof v === "string" ? this._visibleColParser(v) : encodeObject(v));
 
     if (!values.length || !raw) {
       return null;  // null is the default value for a reference list column