Merge pull request #57352 from nextcloud/fix/active-files

fix(files): properly handle currently active node and files action hotkeys

Author: susnux

apps/files/src/actions/deleteUtils.ts

@@ -1,9 +1,9 @@
-/**
+/*!
  * SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 
-import type { Node, View } from '@nextcloud/files'
+import type { INode, IView } from '@nextcloud/files'
 import type { Capabilities } from '../types.ts'
 
 import axios from '@nextcloud/axios'
@@ -17,10 +17,9 @@ import { useUserConfigStore } from '../store/userconfig.ts'
 export const isTrashbinEnabled = () => (getCapabilities() as Capabilities)?.files?.undelete === true
 
 /**
- *
  * @param nodes
  */
-export function canUnshareOnly(nodes: Node[]) {
+export function canUnshareOnly(nodes: INode[]) {
 	return nodes.every((node) => node.attributes['is-mount-root'] === true
 		&& node.attributes['mount-type'] === 'shared')
 }
@@ -29,7 +28,7 @@ export function canUnshareOnly(nodes: Node[]) {
  *
  * @param nodes
  */
-export function canDisconnectOnly(nodes: Node[]) {
+export function canDisconnectOnly(nodes: INode[]) {
 	return nodes.every((node) => node.attributes['is-mount-root'] === true
 		&& node.attributes['mount-type'] === 'external')
 }
@@ -38,7 +37,7 @@ export function canDisconnectOnly(nodes: Node[]) {
  *
  * @param nodes
  */
-export function isMixedUnshareAndDelete(nodes: Node[]) {
+export function isMixedUnshareAndDelete(nodes: INode[]) {
 	if (nodes.length === 1) {
 		return false
 	}
@@ -52,25 +51,26 @@ export function isMixedUnshareAndDelete(nodes: Node[]) {
  *
  * @param nodes
  */
-export function isAllFiles(nodes: Node[]) {
+export function isAllFiles(nodes: INode[]) {
 	return !nodes.some((node) => node.type !== FileType.File)
 }
 
 /**
  *
  * @param nodes
  */
-export function isAllFolders(nodes: Node[]) {
+export function isAllFolders(nodes: INode[]) {
 	return !nodes.some((node) => node.type !== FileType.Folder)
 }
 
 /**
+ * Get the display name for the delete action
  *
- * @param root0
- * @param root0.nodes
- * @param root0.view
+ * @param context - The context
+ * @param context.nodes - The nodes to delete
+ * @param context.view - The current view
  */
-export function displayName({ nodes, view }: { nodes: Node[], view: View }) {
+export function displayName({ nodes, view }: { nodes: INode[], view: IView }) {
 	/**
 	 * If those nodes are all the root node of a
 	 * share, we can only unshare them.
@@ -143,7 +143,7 @@ export function shouldAskForConfirmation() {
  * @param nodes
  * @param view
  */
-export async function askConfirmation(nodes: Node[], view: View) {
+export async function askConfirmation(nodes: INode[], view: IView) {
 	const message = view.id === 'trashbin' || !isTrashbinEnabled()
 		? n('files', 'You are about to permanently delete {count} item', 'You are about to permanently delete {count} items', nodes.length, { count: nodes.length })
 		: n('files', 'You are about to delete {count} item', 'You are about to delete {count} items', nodes.length, { count: nodes.length })
@@ -170,7 +170,7 @@ export async function askConfirmation(nodes: Node[], view: View) {
  *
  * @param node
  */
-export async function deleteNode(node: Node) {
+export async function deleteNode(node: INode) {
 	await axios.delete(node.encodedSource)
 
 	// Let's delete even if it's moved to the trashbin

apps/files/src/actions/favoriteAction.ts

@@ -10,7 +10,7 @@ import StarSvg from '@mdi/svg/svg/star.svg?raw'
 import axios from '@nextcloud/axios'
 import { emit } from '@nextcloud/event-bus'
 import { FileAction, Permission } from '@nextcloud/files'
-import { translate as t } from '@nextcloud/l10n'
+import { t } from '@nextcloud/l10n'
 import { encodePath } from '@nextcloud/paths'
 import { generateUrl } from '@nextcloud/router'
 import { isPublicShare } from '@nextcloud/sharing/public'

apps/files/src/actions/sidebarAction.ts

@@ -54,4 +54,9 @@ export const action = new FileAction({
 	},
 
 	order: -50,
+
+	hotkey: {
+		key: 'D',
+		description: t('files', 'Open the details sidebar'),
+	},
 })

apps/files/src/components/FilesAppSettings/FilesAppSettingsShortcuts.vue

@@ -37,7 +37,11 @@ function hotkeyToString(hotkey: IHotkeyConfig): string {
 	if (hotkey.shift) {
 		parts.push('Shift')
 	}
-	parts.push(hotkey.key)
+	if (hotkey.key.match(/^[a-z]$/)) {
+		parts.push(hotkey.key.toUpperCase())
+	} else {
+		parts.push(hotkey.key)
+	}
 	return parts.join(' ')
 }
 </script>
@@ -71,7 +75,6 @@ function hotkeyToString(hotkey: IHotkeyConfig): string {
 
 		<NcHotkeyList :label="t('files', 'View')">
 			<NcHotkey :label="t('files', 'Toggle grid view')" hotkey="V" />
-			<NcHotkey :label="t('files', 'Open file sidebar')" hotkey="D" />
 			<NcHotkey :label="t('files', 'Show those shortcuts')" hotkey="?" />
 		</NcHotkeyList>
 	</NcAppSettingsShortcutsSection>

apps/files/src/components/FilesListVirtual.vue

@@ -301,7 +301,11 @@ export default defineComponent({
 			}
 
 			if (this.fileId) {
-				this.scrollToFile(this.fileId, false)
+				const node = this.nodes.find((node) => node.fileid === this.fileId)
+				if (node) {
+					this.activeStore.activeNode = node
+					this.scrollToFile(this.fileId, false)
+				}
 			}
 		},
 
@@ -342,13 +346,7 @@ export default defineComponent({
 			delete query.openfile
 			delete query.opendetails
 
-			this.activeStore.activeNode = undefined
-			window.OCP.Files.Router.goToRoute(
-				null,
-				{ ...this.$route.params, fileid: String(this.currentFolder.fileid ?? '') },
-				query,
-				true,
-			)
+			this.activeStore.activeNode = this.currentFolder
 		},
 
 		/**
@@ -396,7 +394,7 @@ export default defineComponent({
 			logger.debug('Ignore `openfile` query and replacing with `opendetails` for ' + node.path, { node })
 			window.OCP.Files.Router.goToRoute(
 				null,
-				this.$route.params,
+				window.OCP.Files.Router.params,
 				{ ...this.$route.query, openfile: undefined, opendetails: '' },
 				true, // silent update of the URL
 			)
@@ -431,10 +429,29 @@ export default defineComponent({
 		},
 
 		onKeyDown(event: KeyboardEvent) {
+			if (this.isEmpty) {
+				return
+			}
+
+			if (event.key !== 'ArrowUp' && event.key !== 'ArrowDown'
+				&& (!this.userConfig.grid_view || (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight'))
+			) {
+				// not an arrow key we handle
+				return
+			}
+
+			if (!this.fileId || this.fileId === this.currentFolder.fileid) {
+				// no active node so use either first or last node
+				const index = event.key === 'ArrowUp' || event.key === 'ArrowLeft'
+					? this.nodes.length - 1
+					: 0
+				this.setActiveNode(this.nodes[index] as NcNode & { fileid: number })
+			}
+
+			const index = this.nodes.findIndex((node) => node.fileid === this.fileId) ?? 0
 			// Up and down arrow keys
 			if (event.key === 'ArrowUp' || event.key === 'ArrowDown') {
 				const columnCount = this.$refs.table?.columnCount ?? 1
-				const index = this.nodes.findIndex((node) => node.fileid === this.fileId) ?? 0
 				const nextIndex = event.key === 'ArrowUp' ? index - columnCount : index + columnCount
 				if (nextIndex < 0 || nextIndex >= this.nodes.length) {
 					return
@@ -450,7 +467,6 @@ export default defineComponent({
 
 			// if grid mode, left and right arrow keys
 			if (this.userConfig.grid_view && (event.key === 'ArrowLeft' || event.key === 'ArrowRight')) {
-				const index = this.nodes.findIndex((node) => node.fileid === this.fileId) ?? 0
 				const nextIndex = event.key === 'ArrowLeft' ? index - 1 : index + 1
 				if (nextIndex < 0 || nextIndex >= this.nodes.length) {
 					return
@@ -465,24 +481,21 @@ export default defineComponent({
 			}
 		},
 
-		setActiveNode(node: NcNode & { fileid: number }) {
+		async setActiveNode(node: NcNode & { fileid: number }) {
 			logger.debug('Navigating to file ' + node.path, { node, fileid: node.fileid })
 			this.scrollToFile(node.fileid)
 
 			// Remove openfile and opendetails from the URL
 			const query = { ...this.$route.query }
 			delete query.openfile
 			delete query.opendetails
+			await this.$router.replace({
+				...this.$route,
+				query,
+			})
 
+			// set the new file as active
 			this.activeStore.activeNode = node
-
-			// Silent update of the URL
-			window.OCP.Files.Router.goToRoute(
-				null,
-				{ ...this.$route.params, fileid: String(node.fileid) },
-				query,
-				true,
-			)
 		},
 	},
 })

apps/files/src/composables/useHotKeys.spec.ts

@@ -4,18 +4,14 @@
  */
 
 import type { View } from '@nextcloud/files'
-import type { Mock } from 'vitest'
 import type { Location } from 'vue-router'
 
 import axios from '@nextcloud/axios'
-import { File, Folder, Permission } from '@nextcloud/files'
+import { File, Folder, Permission, registerFileAction } from '@nextcloud/files'
 import { enableAutoDestroy, mount } from '@vue/test-utils'
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
 import { defineComponent, nextTick } from 'vue'
 import { action as deleteAction } from '../actions/deleteAction.ts'
-import { action as favoriteAction } from '../actions/favoriteAction.ts'
-import { action as renameAction } from '../actions/renameAction.ts'
-import { action as sidebarAction } from '../actions/sidebarAction.ts'
 import { useActiveStore } from '../store/active.ts'
 import { useFilesStore } from '../store/files.ts'
 import { getPinia } from '../store/index.ts'
@@ -63,10 +59,23 @@ const TestComponent = defineComponent({
 	template: '<div />',
 })
 
+beforeAll(() => {
+	// @ts-expect-error mocking for tests
+	window.OCP ??= {}
+	// @ts-expect-error mocking for tests
+	window.OCP.Files ??= {}
+	// @ts-expect-error mocking for tests
+	window.OCP.Files.Router ??= {
+		...router,
+		goToRoute: vi.fn(),
+	}
+})
+
 describe('HotKeysService testing', () => {
 	const activeStore = useActiveStore(getPinia())
 
 	let initialState: HTMLInputElement
+	let component: ReturnType<typeof mount>
 
 	enableAutoDestroy(afterEach)
 
@@ -114,54 +123,15 @@ describe('HotKeysService testing', () => {
 		})))
 		document.body.appendChild(initialState)
 
-		mount(TestComponent)
-	})
-
-	it('Pressing d should open the sidebar once', () => {
-		dispatchEvent({ key: 'd', code: 'KeyD' })
-
-		// Modifier keys should not trigger the action
-		dispatchEvent({ key: 'd', code: 'KeyD', ctrlKey: true })
-		dispatchEvent({ key: 'd', code: 'KeyD', altKey: true })
-		dispatchEvent({ key: 'd', code: 'KeyD', shiftKey: true })
-		dispatchEvent({ key: 'd', code: 'KeyD', metaKey: true })
-
-		expect(sidebarAction.enabled).toHaveReturnedWith(true)
-		expect(sidebarAction.exec).toHaveBeenCalledOnce()
-	})
-
-	it('Pressing F2 should rename the file', () => {
-		dispatchEvent({ key: 'F2', code: 'F2' })
-
-		// Modifier keys should not trigger the action
-		dispatchEvent({ key: 'F2', code: 'F2', ctrlKey: true })
-		dispatchEvent({ key: 'F2', code: 'F2', altKey: true })
-		dispatchEvent({ key: 'F2', code: 'F2', shiftKey: true })
-		dispatchEvent({ key: 'F2', code: 'F2', metaKey: true })
-
-		expect(renameAction.enabled).toHaveReturnedWith(true)
-		expect(renameAction.exec).toHaveBeenCalledOnce()
+		component = mount(TestComponent)
 	})
 
-	it('Pressing s should toggle favorite', () => {
-		(favoriteAction.enabled as Mock).mockReturnValue(true);
-		(favoriteAction.exec as Mock).mockImplementationOnce(() => Promise.resolve(null))
+	// tests for register action handling
 
-		vi.spyOn(axios, 'post').mockImplementationOnce(() => Promise.resolve())
-		dispatchEvent({ key: 's', code: 'KeyS' })
-
-		// Modifier keys should not trigger the action
-		dispatchEvent({ key: 's', code: 'KeyS', ctrlKey: true })
-		dispatchEvent({ key: 's', code: 'KeyS', altKey: true })
-		dispatchEvent({ key: 's', code: 'KeyS', shiftKey: true })
-		dispatchEvent({ key: 's', code: 'KeyS', metaKey: true })
-
-		expect(favoriteAction.exec).toHaveBeenCalledOnce()
-	})
-
-	it('Pressing Delete should delete the file', async () => {
-		// @ts-expect-error unit testing - private method access
-		vi.spyOn(deleteAction._action, 'exec').mockResolvedValue(() => true)
+	it('registeres actions', () => {
+		component.destroy()
+		registerFileAction(deleteAction)
+		component = mount(TestComponent)
 
 		dispatchEvent({ key: 'Delete', code: 'Delete' })
 
@@ -175,6 +145,8 @@ describe('HotKeysService testing', () => {
 		expect(deleteAction.exec).toHaveBeenCalledOnce()
 	})
 
+	// actions implemented by the composable
+
 	it('Pressing alt+up should go to parent directory', () => {
 		expect(router.push).toHaveBeenCalledTimes(0)
 		dispatchEvent({ key: 'ArrowUp', code: 'ArrowUp', altKey: true })
@@ -197,9 +169,8 @@ describe('HotKeysService testing', () => {
 	it.each([
 		['ctrlKey'],
 		['altKey'],
-		// those meta keys are still triggering...
-		// ['shiftKey'],
-		// ['metaKey']
+		['shiftKey'],
+		['metaKey'],
 	])('Pressing v with modifier key %s should not toggle grid view', async (modifier: string) => {
 		vi.spyOn(axios, 'put').mockImplementationOnce(() => Promise.resolve())