fix: Make file list only sortable by one property at the time

* Also add a default sorting (by name ascending)
* Directories are always sorted first
* Sort by display name if available

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

l10n/messages.pot

@@ -55,7 +55,7 @@ msgstr ""
 msgid "Files and folders you recently modified will show up here."
 msgstr ""
 
-#: lib/components/FilePicker/FileList.vue:39
+#: lib/components/FilePicker/FileList.vue:47
 msgid "Modified"
 msgstr ""
 
@@ -67,7 +67,7 @@ msgstr ""
 msgid "Move to {target}"
 msgstr ""
 
-#: lib/components/FilePicker/FileList.vue:19
+#: lib/components/FilePicker/FileList.vue:27
 msgid "Name"
 msgstr ""
 
@@ -80,7 +80,7 @@ msgstr ""
 msgid "Select entry"
 msgstr ""
 
-#: lib/components/FilePicker/FileList.vue:29
+#: lib/components/FilePicker/FileList.vue:37
 msgid "Size"
 msgstr ""
 

lib/components/FilePicker/FileList.spec.ts

@@ -0,0 +1,198 @@
+/**
+ * @copyright Copyright (c) 2023 Ferdinand Thiessen <opensource@fthiessen.de>
+ *
+ * @author Ferdinand Thiessen <opensource@fthiessen.de>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+import { mount } from '@vue/test-utils'
+import { beforeAll, describe, expect, it, vi } from 'vitest'
+
+import FileList from './FileList.vue'
+import { File, Folder } from '@nextcloud/files'
+
+// mock OC.MimeType
+window.OC = {
+	MimeType: {
+		getIconUrl: (mime: string) => `icon/${mime}`,
+	},
+} as never
+
+const exampleNodes = [
+	new File({
+		owner: null,
+		source: 'http://example.com/dav/a-file.txt',
+		mime: 'text/plain',
+		mtime: new Date(),
+		root: '/',
+		size: 200,
+	}),
+	new File({
+		owner: null,
+		source: 'http://example.com/dav/favorite.txt',
+		mime: 'text/plain',
+		mtime: new Date(),
+		root: '/',
+		size: 321,
+		attributes: {
+			favorite: true,
+		},
+	}),
+	new Folder({
+		owner: null,
+		source: 'http://example.com/dav/directory',
+		mtime: new Date(),
+		root: '/',
+		size: 0,
+	}),
+	new File({
+		owner: null,
+		source: 'http://example.com/dav/b-file.txt',
+		mime: 'text/plain',
+		mtime: new Date(),
+		root: '/',
+		size: 100,
+	}),
+]
+
+describe('FilePicker FileList', () => {
+	beforeAll(() => {
+		vi.useFakeTimers()
+	})
+
+	it('is mountable', () => {
+		const consoleError = vi.spyOn(console, 'error')
+		const consoleWarning = vi.spyOn(console, 'warn')
+
+		const wrapper = mount(FileList, {
+			propsData: {
+				multiselect: false,
+				allowPickDirectory: false,
+				loading: false,
+				files: [],
+				selectedFiles: [],
+				path: '/',
+			},
+		})
+		expect(wrapper.html()).toBeTruthy()
+		// No errors or warnings
+		expect(consoleError).not.toBeCalled()
+		expect(consoleWarning).not.toBeCalled()
+	})
+
+	it('header checkbox is not shown if multiselect is `false`', () => {
+		const wrapper = mount(FileList, {
+			propsData: {
+				multiselect: false,
+				allowPickDirectory: false,
+				loading: false,
+				files: [],
+				selectedFiles: [],
+				path: '/',
+			},
+		})
+		expect(wrapper.find('th.row-checkbox').exists()).toBe(false)
+	})
+
+	it('header checkbox is shown if multiselect is `true`', () => {
+		const wrapper = mount(FileList, {
+			propsData: {
+				multiselect: true,
+				allowPickDirectory: false,
+				loading: false,
+				files: [],
+				selectedFiles: [],
+				path: '/',
+			},
+		})
+		expect(wrapper.find('th.row-checkbox').exists()).toBe(true)
+		// there is an aria label
+		expect(wrapper.find('[data-test="file-picker_select-all"]').attributes('aria-label')).toBeTruthy()
+		// no checked
+		expect(wrapper.find('[data-test="file-picker_select-all"]').props('checked')).toBe(false)
+	})
+
+	it('header checkbox is checked when all nodes are selected', async () => {
+		const nodes = [...exampleNodes]
+		const wrapper = mount(FileList, {
+			propsData: {
+				multiselect: true,
+				allowPickDirectory: false,
+				loading: false,
+				files: nodes,
+				selectedFiles: nodes,
+				path: '/',
+			},
+		})
+
+		const selectAll = wrapper.find('[data-test="file-picker_select-all"]')
+		expect(selectAll.props('checked')).toBe(true)
+	})
+
+	describe('file list sorting', () => {
+		it('is sorted initially by name', async () => {
+			const nodes = [...exampleNodes]
+			const wrapper = mount(FileList, {
+				propsData: {
+					multiselect: true,
+					allowPickDirectory: false,
+					loading: false,
+					files: nodes,
+					selectedFiles: [],
+					path: '/',
+				},
+			})
+
+			const rows = wrapper.findAll('.file-picker__row')
+			// all nodes are shown
+			expect(rows.length).toBe(nodes.length)
+			// folder are sorted first
+			expect(rows.at(0).attributes('data-file')).toBe('directory')
+			// other files are ascending
+			expect(rows.at(1).attributes('data-file')).toBe('a-file.txt')
+			expect(rows.at(2).attributes('data-file')).toBe('b-file.txt')
+			expect(rows.at(3).attributes('data-file')).toBe('favorite.txt')
+		})
+
+		it('can sort descending by name', async () => {
+			const nodes = [...exampleNodes]
+			const wrapper = mount(FileList, {
+				propsData: {
+					multiselect: true,
+					allowPickDirectory: false,
+					loading: false,
+					files: nodes,
+					selectedFiles: [],
+					path: '/',
+				},
+			})
+
+			await wrapper.find('[data-test="file-picker_sort-name"]').trigger('click')
+
+			const rows = wrapper.findAll('.file-picker__row')
+			// all nodes are shown
+			expect(rows.length).toBe(nodes.length)
+			// folder are sorted first
+			expect(rows.at(0).attributes('data-file')).toBe('directory')
+			// other files are descending
+			expect(rows.at(1).attributes('data-file')).toBe('favorite.txt')
+			expect(rows.at(2).attributes('data-file')).toBe('b-file.txt')
+			expect(rows.at(3).attributes('data-file')).toBe('a-file.txt')
+		})
+	})
+})

lib/components/FilePicker/FileList.vue

@@ -7,10 +7,18 @@
 						<span class="hidden-visually">
 							{{ t('Select entry') }}
 						</span>
-						<NcCheckboxRadioSwitch v-if="multiselect" :aria-label="t('Select all entries')" :checked="allSelected" @update:checked="onSelectAll" />
+						<NcCheckboxRadioSwitch v-if="multiselect"
+							:aria-label="t('Select all entries')"
+							:checked="allSelected"
+							data-test="file-picker_select-all"
+							@update:checked="onSelectAll" />
 					</th>
 					<th :aria-sort="sortByName" class="row-name">
-						<NcButton :wide="true" type="tertiary" @click="toggleSortByName">
+						<NcButton
+							:wide="true"
+							type="tertiary"
+							data-test="file-picker_sort-name"
+							@click="toggleSortByName">
 							<template #icon>
 								<IconSortAscending v-if="sortByName === 'ascending'" :size="20" />
 								<IconSortDescending v-else-if="sortByName === 'descending'" :size="20" />
@@ -62,7 +70,7 @@
 </template>
 
 <script setup lang="ts">
-import type { Node } from '@nextcloud/files'
+import { FileType, type Node } from '@nextcloud/files'
 
 import { getCanonicalLocale } from '@nextcloud/l10n'
 import { NcButton, NcCheckboxRadioSwitch } from '@nextcloud/vue'
@@ -91,7 +99,7 @@ const emit = defineEmits<{
 
 type ISortingOptions = 'ascending' | 'descending' | undefined
 
-const sortByName = ref<ISortingOptions>(undefined)
+const sortByName = ref<ISortingOptions>('ascending')
 const sortBySize = ref<ISortingOptions>(undefined)
 const sortByModified = ref<ISortingOptions>(undefined)
 
@@ -101,15 +109,17 @@ const ordering = {
 	none: <T>(a: T, b: T, fn: (a: T, b: T) => number) => 0,
 }
 
-const byName = (a: Node, b: Node) => b.basename.localeCompare(a.basename, getCanonicalLocale())
+const byName = (a: Node, b: Node) => (a.attributes?.displayName || a.basename).localeCompare(b.attributes?.displayName || b.basename, getCanonicalLocale())
 const bySize = (a: Node, b: Node) => (b.size || 0) - (a.size || 0)
 const byDate = (a: Node, b: Node) => (a.mtime?.getTime() || 0) - (b.mtime?.getTime() || 0)
 
 const toggleSorting = (variable: Ref<ISortingOptions>) => {
-	if (variable.value === 'ascending') {
+	const old = variable.value
+	// reset
+	sortByModified.value = sortBySize.value = sortByName.value = undefined
+
+	if (old === 'ascending') {
 		variable.value = 'descending'
-	} else if (variable.value === 'descending') {
-		variable.value = undefined
 	} else {
 		variable.value = 'ascending'
 	}
@@ -122,27 +132,28 @@ const toggleSortByModified = () => toggleSorting(sortByModified)
 /**
  * Files sorted by columns
  */
-const sortedFiles = computed(() => {
-	const s = props.files.sort(
+const sortedFiles = computed(() => [...props.files].sort(
 		(a, b) =>
+			// Folders always come above the files
+			(b.type === FileType.Folder ? 1 : 0) - (a.type === FileType.Folder ? 1 : 0) ||
+			// Favorites above other files
+			// (b.attributes?.favorite || false) - (a.attributes?.favorite || false) ||
+			// then sort by name / size / modified
 			ordering[sortByName.value || 'none'](a, b, byName) ||
 			ordering[sortBySize.value || 'none'](a, b, bySize) ||
 			ordering[sortByModified.value || 'none'](a, b, byDate)
 	)
-	console.warn('files sorted')
-	return s
-}
 )
 
 /**
  * Contains the selectable files, filtering out directories if `allowPickDirectory` is not set
  */
-const selectableFiles = computed(() => props.files.filter((file) => props.allowPickDirectory || file.mime !== 'https/unix-directory'))
+const selectableFiles = computed(() => props.files.filter((file) => props.allowPickDirectory || file.type !== FileType.Folder))
 
 /**
  * Whether all selectable files are currently selected
  */
-const allSelected = computed(() => !props.loading && props.selectedFiles.length >= selectableFiles.value.length)
+const allSelected = computed(() => !props.loading && props.selectedFiles.length > 0 && props.selectedFiles.length >= selectableFiles.value.length)
 
 /**
  * Handle the "select all" checkbox

lib/components/FilePicker/FileListRow.vue

@@ -3,6 +3,7 @@
 		:class="['file-picker__row', {
 			'file-picker__row--selected': selected && !showCheckbox
 		}]"
+		:data-file="node.basename"
 		@key-down="handleKeyDown">
 		<td v-if="showCheckbox" class="row-checkbox">
 			<NcCheckboxRadioSwitch :disabled="!isPickable"

vitest.config.ts

@@ -13,6 +13,11 @@ export default defineConfig(async (env) => {
 				include: ['lib/**/*.ts', 'lib/*.ts'],
 				exclude: ['lib/**/*.spec.ts'],
 			},
+			server: {
+				deps: {
+					inline: [/@nextcloud\/vue/],
+				},
+			},
 		},
 	}
 })