Skip to content

Commit

Permalink
Respect number order when sorting assets by title (#12030)
Browse files Browse the repository at this point in the history
- Respect number order when sorting assets by title
- `New Project 1` < `New Project 2` < `New Project 10` rather than lexicographic sort (`1` < `10` < `2`)
- Use user's locale when sorting names

# Important Notes
None
  • Loading branch information
somebody1234 authored Jan 10, 2025
1 parent 4ace1b2 commit b02e228
Show file tree
Hide file tree
Showing 6 changed files with 166 additions and 27 deletions.
48 changes: 39 additions & 9 deletions app/gui/integration-test/dashboard/sort.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,17 @@ test('sort', ({ page }) =>
const date2 = toRfc3339(new Date(START_DATE_EPOCH_MS + 1 * MIN_MS))
const date3 = toRfc3339(new Date(START_DATE_EPOCH_MS + 2 * MIN_MS))
const date4 = toRfc3339(new Date(START_DATE_EPOCH_MS + 3 * MIN_MS))
const date4a = toRfc3339(new Date(START_DATE_EPOCH_MS + 3 * MIN_MS + 1))
const date4b = toRfc3339(new Date(START_DATE_EPOCH_MS + 3 * MIN_MS + 2))
const date5 = toRfc3339(new Date(START_DATE_EPOCH_MS + 4 * MIN_MS))
const date5a = toRfc3339(new Date(START_DATE_EPOCH_MS + 4 * MIN_MS + 1))
const date6 = toRfc3339(new Date(START_DATE_EPOCH_MS + 5 * MIN_MS))
const date7 = toRfc3339(new Date(START_DATE_EPOCH_MS + 6 * MIN_MS))
const date8 = toRfc3339(new Date(START_DATE_EPOCH_MS + 7 * MIN_MS))
api.addDirectory({ modifiedAt: date4, title: 'a directory' })
api.addDirectory({ modifiedAt: date4, title: 'a directory 1' })
api.addDirectory({ modifiedAt: date4a, title: 'a directory 10' })
api.addDirectory({ modifiedAt: date4b, title: 'a directory 2' })
api.addDirectory({ modifiedAt: date5a, title: 'a directory 11' })
api.addDirectory({ modifiedAt: date6, title: 'G directory' })
api.addProject({ modifiedAt: date7, title: 'C project' })
api.addSecret({ modifiedAt: date2, title: 'H secret' })
Expand All @@ -61,8 +67,11 @@ test('sort', ({ page }) =>
// b project
// h secret
// f secret
// a directory
// a directory 1
// a directory 10
// a directory 2
// e file
// a directory 11
// g directory
// c project
// d file
Expand All @@ -81,7 +90,10 @@ test('sort', ({ page }) =>
// Assets in each group are ordered by insertion order.
await expect(rows).toHaveText([
/^G directory/,
/^a directory/,
/^a directory 11/,
/^a directory 2/,
/^a directory 10/,
/^a directory 1/,
/^C project/,
/^b project/,
/^d file/,
Expand All @@ -97,7 +109,10 @@ test('sort', ({ page }) =>
})
.driveTable.withRows(async (rows) => {
await expect(rows).toHaveText([
/^a directory/,
/^a directory 1/,
/^a directory 2/,
/^a directory 10/,
/^a directory 11/,
/^b project/,
/^C project/,
/^d file/,
Expand All @@ -121,7 +136,10 @@ test('sort', ({ page }) =>
/^d file/,
/^C project/,
/^b project/,
/^a directory/,
/^a directory 11/,
/^a directory 10/,
/^a directory 2/,
/^a directory 1/,
])
})
// Sorting should be unset.
Expand All @@ -136,7 +154,10 @@ test('sort', ({ page }) =>
.driveTable.withRows(async (rows) => {
await expect(rows).toHaveText([
/^G directory/,
/^a directory/,
/^a directory 11/,
/^a directory 2/,
/^a directory 10/,
/^a directory 1/,
/^C project/,
/^b project/,
/^d file/,
Expand All @@ -155,8 +176,11 @@ test('sort', ({ page }) =>
/^b project/,
/^H secret/,
/^f secret/,
/^a directory/,
/^a directory 1/,
/^a directory 10/,
/^a directory 2/,
/^e file/,
/^a directory 11/,
/^G directory/,
/^C project/,
/^d file/,
Expand All @@ -172,8 +196,11 @@ test('sort', ({ page }) =>
/^d file/,
/^C project/,
/^G directory/,
/^a directory 11/,
/^e file/,
/^a directory/,
/^a directory 2/,
/^a directory 10/,
/^a directory 1/,
/^f secret/,
/^H secret/,
/^b project/,
Expand All @@ -191,7 +218,10 @@ test('sort', ({ page }) =>
.driveTable.withRows(async (rows) => {
await expect(rows).toHaveText([
/^G directory/,
/^a directory/,
/^a directory 11/,
/^a directory 2/,
/^a directory 10/,
/^a directory 1/,
/^C project/,
/^b project/,
/^d file/,
Expand Down
94 changes: 94 additions & 0 deletions app/gui/src/dashboard/layouts/Drive/__test__/compareAssets.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/** @file Tests for comparing assets. */
import { Column, type SortableColumn } from '#/components/dashboard/column/columnUtils'
import { assetCompareFunction } from '#/layouts/Drive/compareAssets'
import { SortDirection, type SortInfo } from '#/utilities/sorting'
import * as fc from '@fast-check/vitest'
import { DirectoryId, createPlaceholderFileAsset } from 'enso-common/src/services/Backend'
import { toRfc3339 } from 'enso-common/src/utilities/data/dateTime'
import { merge } from 'enso-common/src/utilities/data/object'
import { expect } from 'vitest'

const SORT_BY_NAME_ASCENDING: SortInfo<SortableColumn> = {
field: Column.name,
direction: SortDirection.ascending,
}

const SORT_BY_NAME_DESCENDING: SortInfo<SortableColumn> = {
field: Column.name,
direction: SortDirection.descending,
}

const SORT_BY_MODIFIED_ASCENDING: SortInfo<SortableColumn> = {
field: Column.modified,
direction: SortDirection.ascending,
}

const SORT_BY_MODIFIED_DESCENDING: SortInfo<SortableColumn> = {
field: Column.modified,
direction: SortDirection.descending,
}

fc.test.prop({
prefix: fc.fc.string(),
numbers: fc.fc.array(fc.fc.integer({ min: 0 })),
})('numbers should be sorted with dictionary sort', ({ prefix, numbers }) => {
const names = numbers.map((number) => `${prefix} ${number}`)
const assets = names.map((name) =>
createPlaceholderFileAsset(name, DirectoryId('directory-'), []),
)
const compareByNameAscending = assetCompareFunction(SORT_BY_NAME_ASCENDING, 'en')
const sorted = assets.sort(compareByNameAscending)
const sortedNames = sorted.map((asset) => asset.title)
const expectedNames = numbers.sort((a, b) => a - b).map((number) => `${prefix} ${number}`)
expect(sortedNames).toStrictEqual(expectedNames)

const compareByNameDescending = assetCompareFunction(SORT_BY_NAME_DESCENDING, 'en')
const sortedDescending = assets.sort(compareByNameDescending)
const sortedDescendingNames = sortedDescending.map((asset) => asset.title)
const expectedDescendingNames = numbers
.sort((a, b) => b - a)
.map((number) => `${prefix} ${number}`)
expect(sortedDescendingNames).toStrictEqual(expectedDescendingNames)
})

fc.test.prop({
names: fc.fc.array(fc.fc.string().map((s) => s.replace(/\d+/g, ''))),
})('sort by name', ({ names }) => {
const assets = names.map((name) =>
createPlaceholderFileAsset(name, DirectoryId('directory-'), []),
)

const compareByModifiedAscending = assetCompareFunction(SORT_BY_NAME_ASCENDING, 'en')
const sorted = assets.sort(compareByModifiedAscending)
const sortedNames = sorted.map((asset) => asset.title)
const expectedNames = names.sort((a, b) => a.localeCompare(b, 'en'))
expect(sortedNames).toStrictEqual(expectedNames)

const compareByModifiedDescending = assetCompareFunction(SORT_BY_NAME_DESCENDING, 'en')
const sortedDescending = assets.sort(compareByModifiedDescending)
const sortedDescendingNames = sortedDescending.map((asset) => asset.title)
const expectedDescendingNames = names.sort((a, b) => -a.localeCompare(b, 'en'))
expect(sortedDescendingNames).toStrictEqual(expectedDescendingNames)
})

fc.test.prop({
dates: fc.fc.array(fc.fc.integer({ min: 0 })).map((numbers) => numbers.map((n) => new Date(n))),
})('sort by modified', ({ dates }) => {
const assets = dates.map((date) =>
merge(createPlaceholderFileAsset('', DirectoryId('directory-'), []), {
modifiedAt: toRfc3339(date),
}),
)

const compareByModifiedAscending = assetCompareFunction(SORT_BY_MODIFIED_ASCENDING, 'en')
const sorted = assets.sort(compareByModifiedAscending)
const sortedDates = sorted.map((asset) => new Date(asset.modifiedAt))
const expectedDates = dates.sort((a, b) => Number(a) - Number(b))
expect(sortedDates).toStrictEqual(expectedDates)

const compareByModifiedDescending = assetCompareFunction(SORT_BY_MODIFIED_DESCENDING, 'en')
const sortedDescending = assets.sort(compareByModifiedDescending)
const sortedDescendingDates = sortedDescending.map((asset) => new Date(asset.modifiedAt))
const expectedDescendingDates = dates.sort((a, b) => Number(b) - Number(a))
expect(sortedDescendingDates).toStrictEqual(expectedDescendingDates)
})
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ import { AssetType, getAssetPermissionName } from 'enso-common/src/services/Back
import { PermissionAction } from 'enso-common/src/utilities/permissions'

import type { SortableColumn } from '#/components/dashboard/column/columnUtils'
import { Column } from '#/components/dashboard/column/columnUtils'
import { assetCompareFunction } from '#/layouts/Drive/compareAssets'
import { useText } from '#/providers/TextProvider'
import type { DirectoryId } from '#/services/ProjectManager'
import type AssetQuery from '#/utilities/AssetQuery'
import type { AnyAssetTreeNode } from '#/utilities/AssetTreeNode'
import Visibility from '#/utilities/Visibility'
import { fileExtension } from '#/utilities/fileInfo'
import type { SortInfo } from '#/utilities/sorting'
import { SortDirection } from '#/utilities/sorting'
import { regexEscape } from '#/utilities/string'
import { createStore, useStore } from '#/utilities/zustand.ts'
import invariant from 'tiny-invariant'
Expand Down Expand Up @@ -61,6 +61,7 @@ export function useAssetStrict(id: AssetId) {
export function useAssetsTableItems(options: UseAssetsTableOptions) {
const { assetTree, sortInfo, query, expandedDirectoryIds } = options

const { locale } = useText()
const setAssetItems = useStore(ASSET_ITEMS_STORE, (store) => store.setItems)

const filter = (() => {
Expand Down Expand Up @@ -212,22 +213,8 @@ export function useAssetsTableItems(options: UseAssetsTableOptions) {

return flatTree
} else {
const multiplier = sortInfo.direction === SortDirection.ascending ? 1 : -1
let compare: (a: AnyAssetTreeNode, b: AnyAssetTreeNode) => number
switch (sortInfo.field) {
case Column.name: {
compare = (a, b) => multiplier * a.item.title.localeCompare(b.item.title, 'en')
break
}
case Column.modified: {
compare = (a, b) => {
const aOrder = Number(new Date(a.item.modifiedAt))
const bOrder = Number(new Date(b.item.modifiedAt))
return multiplier * (aOrder - bOrder)
}
break
}
}
const compareAssets = assetCompareFunction(sortInfo, locale)
const compare = (a: AnyAssetTreeNode, b: AnyAssetTreeNode) => compareAssets(a.item, b.item)
const flatTree = assetTree.preorderTraversal((tree) =>
[...tree]
.filter((child) => expandedDirectoryIds.includes(child.item.parentId))
Expand Down
28 changes: 28 additions & 0 deletions app/gui/src/dashboard/layouts/Drive/compareAssets.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/** @file Functions related to comparing assets. */
import { Column, type SortableColumn } from '#/components/dashboard/column/columnUtils'
import { SortDirection, type SortInfo } from '#/utilities/sorting'
import type { AnyAsset } from 'enso-common/src/services/Backend'

/** Return a function to compare two assets. */
export function assetCompareFunction(
sortInfo: SortInfo<SortableColumn>,
locale: string | undefined,
) {
const multiplier = sortInfo.direction === SortDirection.ascending ? 1 : -1
let compare: (a: AnyAsset, b: AnyAsset) => number
switch (sortInfo.field) {
case Column.name: {
compare = (a, b) => multiplier * a.title.localeCompare(b.title, locale, { numeric: true })
break
}
case Column.modified: {
compare = (a, b) => {
const aOrder = Number(new Date(a.modifiedAt))
const bOrder = Number(new Date(b.modifiedAt))
return multiplier * (aOrder - bOrder)
}
break
}
}
return compare
}

0 comments on commit b02e228

Please sign in to comment.