From 6793377579d5b12c305fbe1cba16be2fc09a6e0b Mon Sep 17 00:00:00 2001 From: Cyprien P Date: Tue, 24 May 2022 09:10:15 +0200 Subject: [PATCH] (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 | 3 +- app/client/models/ColumnFilterMenuModel.ts | 31 ++++++++++++--- app/client/ui/ColumnFilterMenu.ts | 46 +++++++++++++--------- 3 files changed, 55 insertions(+), 25 deletions(-) diff --git a/app/client/models/ColumnFilter.ts b/app/client/models/ColumnFilter.ts index 73217381..fb7cb68f 100644 --- a/app/client/models/ColumnFilter.ts +++ b/app/client/models/ColumnFilter.ts @@ -24,7 +24,8 @@ export class ColumnFilter extends Disposable { private _include: boolean; private _values: Set; - constructor(private _initialFilterJson: string, private _columnType?: string) { + constructor(private _initialFilterJson: string, private _columnType: string = '', + public visibleColumnType: string = '') { super(); this.setState(_initialFilterJson); } diff --git a/app/client/models/ColumnFilterMenuModel.ts b/app/client/models/ColumnFilterMenuModel.ts index bf46d6a7..e6193b61 100644 --- a/app/client/models/ColumnFilterMenuModel.ts +++ b/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 = (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 ? - (a, b) => nativeCompare(b[1].count, a[1].count) : - (a, b) => localeCompare(a[1].label, b[1].label); + const prop: keyof IFilterCount = isSortedByCount ? 'count' : 'displayValue'; + let isShownFirst: (val: any) => boolean = isNull; + if (['Date', 'DateTime', 'Numeric', 'Int'].includes(this.columnFilter.visibleColumnType)) { + isShownFirst = (val) => isNull(val) || isNaN(val); + } + + const comparator: ICompare = (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])); } ); diff --git a/app/client/ui/ColumnFilterMenu.ts b/app/client/ui/ColumnFilterMenu.ts index d39ff029..93c2e76d 100644 --- a/app/client/ui/ColumnFilterMenu.ts +++ b/app/client/ui/ColumnFilterMenu.ts @@ -290,7 +290,7 @@ function getEmptyCountMap(fieldOrColumn: ViewFieldRec|ColumnRec): Map [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`, - * which map row ids to cell values and labels respectively. + * Returns three callback functions, `keyMapFunc`, `labelMapFunc` + * 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}; + return {keyMapFunc, labelMapFunc, valueMapFunc}; } /** @@ -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 - * with `labelMapFunc` and a `count` representing the total number of times the key has been encountered. + * For each row id in Iterable, adds a key mapped with `keyMapFunc` and a value object with a + * `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, 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, 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, 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, rowIds: RowId[], * Adds the `value` to `valueMap` using `labelGetter` to get the label and increments `count` unless * isHiddenRow is true. */ -function addSingleCountToMap(valueMap: Map, value: any, labelGetter: () => any, - isHiddenRow: boolean) { +function addSingleCountToMap(valueMap: Map, value: any, label: () => any, + 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++;