From 6e844a2e76a12f47b1095db7038b01b61b7a8cd3 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Fri, 12 Mar 2021 21:25:44 -0500 Subject: [PATCH] (core) Use unicode-aware comparisons for user-visible strings. Summary: - Switch code that compares user strings to use localeCompare() based on Intl.Collator. - Use en-US locale for now. (Ideally should be a document property.) - Note that with this change, sorting is also becoming case-insensitive (which seems an improvement) - Updated a sorted test fixture - Updated a browser test with lots of unicode to expect different order. - Added a bit of unicode to test ordering in Reference autocomplete dropdown. Test Plan: Fixed / updated tests Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D2758 --- app/client/lib/ACIndex.ts | 4 ++-- app/client/models/ColumnACIndexes.ts | 6 ++--- app/client/ui/ColumnFilterMenu.ts | 4 ++-- app/common/SortFunc.ts | 35 ++++++++++++++++++---------- app/common/gutil.ts | 5 ++++ 5 files changed, 35 insertions(+), 19 deletions(-) diff --git a/app/client/lib/ACIndex.ts b/app/client/lib/ACIndex.ts index ba1042c3..e8ee9543 100644 --- a/app/client/lib/ACIndex.ts +++ b/app/client/lib/ACIndex.ts @@ -7,7 +7,7 @@ * "lush" would only match the "L" in "Lavender". */ -import {nativeCompare, sortedIndex} from 'app/common/gutil'; +import {localeCompare, nativeCompare, sortedIndex} from 'app/common/gutil'; import {DomContents} from 'grainjs'; export interface ACItem { @@ -80,7 +80,7 @@ export class ACIndexImpl implements ACIndex { } } - allWords.sort((a, b) => nativeCompare(a.word, b.word)); + allWords.sort((a, b) => localeCompare(a.word, b.word)); this._words = allWords; } diff --git a/app/client/models/ColumnACIndexes.ts b/app/client/models/ColumnACIndexes.ts index cef4dd95..bf65f21e 100644 --- a/app/client/models/ColumnACIndexes.ts +++ b/app/client/models/ColumnACIndexes.ts @@ -13,7 +13,7 @@ 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, nativeCompare} from 'app/common/gutil'; +import {getSetMapValue, localeCompare, nativeCompare} from 'app/common/gutil'; import {BaseFormatter} from 'app/common/ValueFormatter'; export interface ICellItem { @@ -76,7 +76,7 @@ export class ColumnACIndexes { } function itemCompare(a: ICellItem, b: ICellItem) { - return nativeCompare(a.cleanText, b.cleanText) || - nativeCompare(a.text, b.text) || + return localeCompare(a.cleanText, b.cleanText) || + localeCompare(a.text, b.text) || nativeCompare(a.rowId, b.rowId); } diff --git a/app/client/ui/ColumnFilterMenu.ts b/app/client/ui/ColumnFilterMenu.ts index c8f79dbf..03aef101 100644 --- a/app/client/ui/ColumnFilterMenu.ts +++ b/app/client/ui/ColumnFilterMenu.ts @@ -15,7 +15,7 @@ import {colors, vars} from 'app/client/ui2018/cssVars'; import {icon} from 'app/client/ui2018/icons'; import {menuCssClass, menuDivider, menuIcon} from 'app/client/ui2018/menus'; import {CellValue} from 'app/common/DocActions'; -import {nativeCompare} from 'app/common/gutil'; +import {localeCompare} from 'app/common/gutil'; import {Computed, dom, input, makeTestId, Observable, styled} from 'grainjs'; import escapeRegExp = require('lodash/escapeRegExp'); import identity = require('lodash/identity'); @@ -63,7 +63,7 @@ export function columnFilterMenu({ columnFilter, valueCounts, doSave, onClose }: const filteredValues = Computed.create(null, openSearch, searchValueObs, (_use, isOpen, searchValue) => { const searchRegex = new RegExp(escapeRegExp(searchValue), 'i'); return valueCountArr.filter(([key]) => !isOpen || searchRegex.test(key as string)) - .sort((a, b) => nativeCompare(a[1].label, b[1].label)); + .sort((a, b) => localeCompare(a[1].label, b[1].label)); }); let searchInput: HTMLInputElement; diff --git a/app/common/SortFunc.ts b/app/common/SortFunc.ts index b7d13e6f..140e2c08 100644 --- a/app/common/SortFunc.ts +++ b/app/common/SortFunc.ts @@ -7,7 +7,7 @@ * currently implemented. */ import {ColumnGetters} from 'app/common/ColumnGetters'; -import {nativeCompare} from 'app/common/gutil'; +import {localeCompare, nativeCompare} from 'app/common/gutil'; /** * Compare two cell values, paying attention to types and values. Note that native JS comparison @@ -19,16 +19,27 @@ import {nativeCompare} from 'app/common/gutil'; * because e.g. a numerical column may contain text (alttext) or null values. */ export function typedCompare(val1: any, val2: any): number { - // TODO: We should use Intl.Collator for string comparisons to handle accented strings. - let type1, array1; - return nativeCompare(type1 = typeof val1, typeof val2) || - // We need to worry about Array comparisons because formulas returing Any may return null or - // object values represented as arrays (e.g. ['D', ...] for dates). Comparing those without - // distinguishing types would break the sort. Also, arrays need a special comparator. - (type1 === 'object' && - (nativeCompare(array1 = val1 instanceof Array, val2 instanceof Array) || - (array1 && _arrayCompare(val1, val2)))) || - nativeCompare(val1, val2); + let result: number, type1: string, array1: boolean; + // tslint:disable-next-line:no-conditional-assignment + if ((result = nativeCompare(type1 = typeof val1, typeof val2)) !== 0) { + return result; + } + // We need to worry about Array comparisons because formulas returing Any may return null or + // object values represented as arrays (e.g. ['D', ...] for dates). Comparing those without + // distinguishing types would break the sort. Also, arrays need a special comparator. + if (type1 === 'object') { + // tslint:disable-next-line:no-conditional-assignment + if ((result = nativeCompare(array1 = val1 instanceof Array, val2 instanceof Array)) !== 0) { + return result; + } + if (array1) { + return _arrayCompare(val1, val2); + } + } + if (type1 === 'string') { + return localeCompare(val1, val2); + } + return nativeCompare(val1, val2); } function _arrayCompare(val1: any[], val2: any[]): number { @@ -36,7 +47,7 @@ function _arrayCompare(val1: any[], val2: any[]): number { if (i >= val2.length) { return 1; } - const value = nativeCompare(val1[i], val2[i]); + const value = typedCompare(val1[i], val2[i]); if (value) { return value; } diff --git a/app/common/gutil.ts b/app/common/gutil.ts index cf8b670c..3814d7d5 100644 --- a/app/common/gutil.ts +++ b/app/common/gutil.ts @@ -444,6 +444,11 @@ export function nativeCompare(a: T, b: T): number { return (a < b ? -1 : (a > b ? 1 : 0)); } +// TODO: In the future, locale should be a value associated with the document or the user. +export const defaultLocale = 'en-US'; +export const defaultCollator = new Intl.Collator(defaultLocale); +export const localeCompare = defaultCollator.compare; + /** * A copy of python`s `setdefault` function. * Sets key in mapInst to value, if key is not already set.