Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect number order when sorting assets by title #12030

Merged
merged 10 commits into from
Jan 10, 2025
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
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
}
Loading