(core) Fix values ordering in column filter menu

Summary:
Column filter menu use to mess up the ordering of the items for
numeric and dates values, and also for ref/reflist columns when the
visible column is a numeric a date column.

Solution was to:
 - use the actual value of the visible column for comparison.
 - use native comparison.
 - tweak the native comparison to make blanks appears before valid value. Indeed, it came up several time that it's convenient to have invalid values show up first in the filter panel, it makes for a convenient way to detect them.

Test Plan: Adds new nbrowser test

Reviewers: alexmojaki

Reviewed By: alexmojaki

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

app/client/models/ColumnFilter.ts

@@ -24,7 +24,8 @@ export class ColumnFilter extends Disposable {
   private _include: boolean;
   private _values: Set<CellValue>;
 
-  constructor(private _initialFilterJson: string, private _columnType?: string) {
+  constructor(private _initialFilterJson: string, private _columnType: string = '',
+              public visibleColumnType: string = '') {
     super();
     this.setState(_initialFilterJson);
   }

app/client/models/ColumnFilterMenuModel.ts

@@ -1,18 +1,28 @@
 import { ColumnFilter } from "app/client/models/ColumnFilter";
-import { localeCompare, nativeCompare } from "app/common/gutil";
 import { CellValue } from "app/plugin/GristData";
 import { Computed, Disposable, Observable } from "grainjs";
 import escapeRegExp = require("lodash/escapeRegExp");
+import isNull = require("lodash/isNull");
 
 const MAXIMUM_SHOWN_FILTER_ITEMS = 500;
 
 export interface IFilterCount {
+
+  // label is the formatted value
   label: string;
+
+  // number of occurences in the table
   count: number;
+
+  // displayValue is the underlying value (from the display column, if any), useful to perform
+  // comparison
+  displayValue: any;
 }
 
 type ICompare<T> = (a: T, b: T) => number
 
+const localeCompare = new Intl.Collator('en-US', {numeric: true}).compare;
+
 export class ColumnFilterMenuModel extends Disposable {
 
   public readonly searchValue = Observable.create(this, '');
@@ -22,7 +32,7 @@ export class ColumnFilterMenuModel extends Disposable {
   // computes a set of all keys that matches the search text.
   public readonly filterSet = Computed.create(this, this.searchValue, (_use, searchValue) => {
     const searchRegex = new RegExp(escapeRegExp(searchValue), 'i');
-    const showAllOptions = ['Bool', 'Choice', 'ChoiceList'].includes(this.columnFilter.columnType!);
+    const showAllOptions = ['Bool', 'Choice', 'ChoiceList'].includes(this.columnFilter.columnType);
     return new Set(
       this._valueCount
         .filter(([_, {label, count}]) => (showAllOptions ? true : count) && searchRegex.test(label))
@@ -34,12 +44,21 @@ export class ColumnFilterMenuModel extends Disposable {
   public readonly filteredValues = Computed.create(
     this, this.filterSet, this.isSortedByCount,
     (_use, filter, isSortedByCount) => {
-      const comparator: ICompare<[CellValue, IFilterCount]> = isSortedByCount ?
+      const prop: keyof IFilterCount = isSortedByCount ? 'count' : 'displayValue';
-        (a, b) => nativeCompare(b[1].count, a[1].count) :
+      let isShownFirst: (val: any) => boolean = isNull;
-        (a, b) => localeCompare(a[1].label, b[1].label);
+      if (['Date', 'DateTime', 'Numeric', 'Int'].includes(this.columnFilter.visibleColumnType)) {
+        isShownFirst = (val) => isNull(val) || isNaN(val);
+      }
+
+      const comparator: ICompare<any> = (a, b) => {
+        if (isShownFirst(a)) { return -1; }
+        if (isShownFirst(b)) { return 1; }
+        return localeCompare(a,  b);
+      };
+
       return this._valueCount
         .filter(([key]) => filter.has(key))
-        .sort(comparator);
+        .sort((a, b) => comparator(a[1][prop], b[1][prop]));
     }
   );
 

app/client/ui/ColumnFilterMenu.ts

@@ -290,7 +290,7 @@ function getEmptyCountMap(fieldOrColumn: ViewFieldRec|ColumnRec): Map<CellValue,
     const options = fieldOrColumn.origCol().widgetOptionsJson;
     values = options.prop('choices')();
   }
-  return new Map(values.map((v) => [v, {label: String(v), count: 0}]));
+  return new Map(values.map((v) => [v, {label: String(v), count: 0, displayValue: v}]));
 }
 
 /**
@@ -301,7 +301,8 @@ export function createFilterMenu(openCtl: IOpenController, sectionFilter: Sectio
   // Go through all of our shown and hidden rows, and count them up by the values in this column.
   const fieldOrColumn = filterInfo.fieldOrColumn;
   const columnType = fieldOrColumn.origCol.peek().type.peek();
-  const {keyMapFunc, labelMapFunc} = getMapFuncs(columnType, tableData, filterInfo.fieldOrColumn);
+  const visibleColumnType = fieldOrColumn.visibleColModel.peek()?.type.peek() || columnType;
+  const {keyMapFunc, labelMapFunc, valueMapFunc} = getMapFuncs(columnType, tableData, filterInfo.fieldOrColumn);
   const activeFilterBar = sectionFilter.viewSection.activeFilterBar;
 
   function getFilterFunc(col: ViewFieldRec|ColumnRec, colFilter: ColumnFilterFunc|null) {
@@ -310,14 +311,15 @@ export function createFilterMenu(openCtl: IOpenController, sectionFilter: Sectio
   const filterFunc = Computed.create(null, use => sectionFilter.buildFilterFunc(getFilterFunc, use));
   openCtl.autoDispose(filterFunc);
 
-  const columnFilter = ColumnFilter.create(openCtl, filterInfo.filter.peek(), columnType);
+  const columnFilter = ColumnFilter.create(openCtl, filterInfo.filter.peek(), columnType, visibleColumnType);
   sectionFilter.setFilterOverride(fieldOrColumn.getRowId(), columnFilter); // Will be removed on menu disposal
 
   const [allRows, hiddenRows] = partition(Array.from(rowSource.getAllRows()), filterFunc.get());
   const valueCounts = getEmptyCountMap(fieldOrColumn);
-  addCountsToMap(valueCounts, allRows, {keyMapFunc, labelMapFunc, columnType});
+  addCountsToMap(valueCounts, allRows, {keyMapFunc, labelMapFunc, columnType,
+                                        valueMapFunc});
   addCountsToMap(valueCounts, hiddenRows, {keyMapFunc, labelMapFunc, columnType,
-                                                               areHiddenRows: true});
+                                           areHiddenRows: true, valueMapFunc});
 
   const model = ColumnFilterMenuModel.create(openCtl, columnFilter, Array.from(valueCounts));
 
@@ -341,8 +343,9 @@ export function createFilterMenu(openCtl: IOpenController, sectionFilter: Sectio
 }
 
 /**
- * Returns two callback functions, `keyMapFunc` and `labelMapFunc`,
+ * Returns three callback functions, `keyMapFunc`, `labelMapFunc`
- * which map row ids to cell values and labels respectively.
+ * and `valueMapFunc`, which map row ids to cell values, labels
+ * and visible col value respectively.
  *
  * The functions vary based on the `columnType`. For example,
  * Reference Lists have a unique `labelMapFunc` that returns a list
@@ -357,6 +360,8 @@ function getMapFuncs(columnType: string, tableData: TableData, fieldOrColumn: Vi
   const formatter = fieldOrColumn.visibleColFormatter();
 
   let labelMapFunc: (rowId: number) => string | string[];
+  const valueMapFunc: (rowId: number) => any = (rowId: number) => decodeObject(labelGetter(rowId)!);
+
   if (isRefListType(columnType)) {
     labelMapFunc = (rowId: number) => {
       const maybeLabels = labelGetter(rowId);
@@ -367,8 +372,7 @@ function getMapFuncs(columnType: string, tableData: TableData, fieldOrColumn: Vi
   } else {
     labelMapFunc = (rowId: number) => formatter.formatAny(labelGetter(rowId));
   }
-
+  return {keyMapFunc, labelMapFunc, valueMapFunc};
-  return {keyMapFunc, labelMapFunc};
 }
 
 /**
@@ -412,14 +416,19 @@ function getRenderFunc(columnType: string, fieldOrColumn: ViewFieldRec|ColumnRec
 
 interface ICountOptions {
   columnType: string;
+  // returns the indexing key for the filter
   keyMapFunc?: (v: any) => any;
+  // returns the string representation of the value (can involves some formatting).
   labelMapFunc?: (v: any) => any;
+  // returns the underlying value (useful for comparison)
+  valueMapFunc: (v: any) => any;
   areHiddenRows?: boolean;
 }
 
 /**
- * For each row id in Iterable, adds a key mapped with `keyMapFunc` and a value object with a `label` mapped
+ * For each row id in Iterable, adds a key mapped with `keyMapFunc` and a value object with a
- * with `labelMapFunc` and a `count` representing the total number of times the key has been encountered.
+ * `label` mapped with `labelMapFunc` and a `count` representing the total number of times the key
+ * has been encountered and a `displayValues` mapped with `valueMapFunc`.
  *
  * The optional column type controls how complex cell values are decomposed into keys (e.g. Choice Lists have
  * the possible choices as keys).
@@ -427,7 +436,7 @@ interface ICountOptions {
  */
 function addCountsToMap(valueMap: Map<CellValue, IFilterCount>, rowIds: RowId[],
                         { keyMapFunc = identity, labelMapFunc = identity, columnType,
-                          areHiddenRows = false }: ICountOptions) {
+                          areHiddenRows = false, valueMapFunc }: ICountOptions) {
 
   for (const rowId of rowIds) {
     let key = keyMapFunc(rowId);
@@ -436,7 +445,7 @@ function addCountsToMap(valueMap: Map<CellValue, IFilterCount>, rowIds: RowId[],
     if (isList(key) && (columnType === 'ChoiceList')) {
       const list = decodeObject(key) as unknown[];
       for (const item of list) {
-        addSingleCountToMap(valueMap, item, () => item, areHiddenRows);
+        addSingleCountToMap(valueMap, item, () => item, () => item, areHiddenRows);
       }
       continue;
     }
@@ -445,14 +454,15 @@ function addCountsToMap(valueMap: Map<CellValue, IFilterCount>, rowIds: RowId[],
     if (isList(key) && isRefListType(columnType)) {
       const refIds = decodeObject(key) as unknown[];
       const refLabels = labelMapFunc(rowId);
+      const displayValues = valueMapFunc(rowId);
       refIds.forEach((id, i) => {
-        addSingleCountToMap(valueMap, id, () => refLabels[i], areHiddenRows);
+        addSingleCountToMap(valueMap, id, () => refLabels[i], () => displayValues[i], areHiddenRows);
       });
       continue;
     }
     // For complex values, serialize the value to allow them to be properly stored
     if (Array.isArray(key)) { key = JSON.stringify(key); }
-    addSingleCountToMap(valueMap, key, () => labelMapFunc(rowId), areHiddenRows);
+    addSingleCountToMap(valueMap, key, () => labelMapFunc(rowId), () => valueMapFunc(rowId), areHiddenRows);
   }
 }
 
@@ -460,10 +470,10 @@ function addCountsToMap(valueMap: Map<CellValue, IFilterCount>, rowIds: RowId[],
  * Adds the `value` to `valueMap` using `labelGetter` to get the label and increments `count` unless
  * isHiddenRow is true.
  */
-function addSingleCountToMap(valueMap: Map<CellValue, IFilterCount>, value: any, labelGetter: () => any,
+function addSingleCountToMap(valueMap: Map<CellValue, IFilterCount>, value: any, label: () => any,
-                       isHiddenRow: boolean) {
+                             displayValue: () => any, isHiddenRow: boolean) {
   if (!valueMap.has(value)) {
-    valueMap.set(value, { label: labelGetter(), count: 0 });
+    valueMap.set(value, { label: label(), count: 0, displayValue: displayValue() });
   }
   if (!isHiddenRow) {
     valueMap.get(value)!.count++;