Skip to content

fix(sorting): also check attributes #1285

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.

Sign up for GitHub

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

Merged
skjnldsv merged 1 commit into from
Jul 14, 2025
Merged

fix(sorting): also check attributes #1285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions __tests__/utils/fileSorting.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
import type { Attribute } from '../../lib/files/nodeData'

import { ArgumentsType, describe, expect, test } from 'vitest'
import { File, FilesSortingMode, Folder, sortNodes as originalSortNodes } from '../../lib'

const file = (name: string, size?: number, modified?: number, favorite = false) => new File({
const file = (name: string, size?: number, modified?: number, favorite = false, attributes: Attribute = {}) => new File({
source: `https://cloud.domain.com/remote.php/dav/${name}`,
mime: 'text/plain',
owner: 'jdoe',
Expand All @@ -15,7 +17,7 @@ const file = (name: string, size?: number, modified?: number, favorite = false)
? {
favorite: 1,
}
: undefined,
: attributes,
})

const folder = (name: string, size?: number, modified?: number, favorite = false) => new Folder({
Expand Down Expand Up @@ -285,4 +287,14 @@ describe('sortNodes', () => {
),
).toEqual(['file.a', 'file.b', 'file.c', 'file.d'])
})

test('Can sort by random attribute', () => {
const array = [
file('a', 500, 100, false, { order: 3 }),
file('b', 100, 100, false, { order: 2 }),
file('c', 100, 500, false, { order: 1 }),
]

expect(sortNodes(array, { sortingMode: 'order' })).toEqual(['c', 'b', 'a'])
})
})
2 changes: 1 addition & 1 deletion lib/dav/dav.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export const getFavoriteNodes = (davClient: WebDAVClient, path = '/', davRoot =
}

/**
* Covert DAV result `FileStat` to `Node`
* Convert DAV result `FileStat` to `Node`
*
* @param node The DAV result
* @param filesRoot The DAV files root path
Expand Down
4 changes: 2 additions & 2 deletions lib/utils/fileSorting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export interface FilesSortingOptions {
* They key to order the files by
* @default FilesSortingMode.Name
*/
sortingMode?: FilesSortingMode
sortingMode?: FilesSortingMode | string

/**
* @default 'asc'
Expand Down Expand Up @@ -63,7 +63,7 @@ export function sortNodes(nodes: readonly INode[], options: FilesSortingOptions
// 2: Sort folders first if sorting by name
...(sortingOptions.sortFoldersFirst ? [(v: INode) => v.type !== 'folder'] : []),
// 3: Use sorting mode if NOT basename (to be able to use display name too)
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode]] : []),
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode] || v.attributes[sortingOptions.sortingMode]] : []),
Copy link
Contributor

@susnux susnux Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes is clearer, otherwise this is also evaluated if the first value is just falsy like an empty string or the number 0.

Suggested change
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode] || v.attributes[sortingOptions.sortingMode]] : []),
...(sortingOptions.sortingMode !== FilesSortingMode.Name ? [(v: INode) => v[sortingOptions.sortingMode] ?? v.attributes[sortingOptions.sortingMode]] : []),

Copy link
Contributor Author

@skjnldsv skjnldsv Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes is clearer, otherwise this is also evaluated if the first value is just falsy like an empty string or the number 0.

Wouldn't that be better then ?
We don't really want to sort my an empty string or a 0 🤔

Copy link
Contributor

@susnux susnux Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it should not make a difference, due to the top level things we have defined. But it could.
E.g. if you have order of value 0 you want to sort by it.

In general if there is a value for that key it should be sorted by it, only if there is no such key on the node it should sort by the key on the node's attributes, no?

Copy link
Contributor Author

@skjnldsv skjnldsv Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, basically :)
Let me create a followup 👍

// 4: Use display name if available, fallback to name
(v: INode) => basename(v.displayname || v.attributes?.displayname || v.basename || ''),
// 5: Finally, use basename if all previous sorting methods failed
Expand Down