fix(files): trashbin redirect and default fileid Sidebar open

Signed-off-by: John Molakvoæ <[email protected]>

apps/files/appinfo/routes.php

@@ -139,16 +139,14 @@
 				'verb' => 'GET'
 			],
 			[
-				'name' => 'view#index',
+				'name' => 'view#indexView',
 				'url' => '/{view}',
 				'verb' => 'GET',
-				'postfix' => 'view',
 			],
 			[
-				'name' => 'view#index',
+				'name' => 'view#indexViewFileid',
 				'url' => '/{view}/{fileid}',
 				'verb' => 'GET',
-				'postfix' => 'fileid',
 			],
 		],
 		'ocs' => [

apps/files/lib/Controller/ViewController.php

@@ -153,17 +153,36 @@ protected function getStorageInfo(string $dir = '/') {
 	 *
 	 * @param string $fileid
 	 * @return TemplateResponse|RedirectResponse
-	 * @throws NotFoundException
 	 */
-	public function showFile(string $fileid = null, int $openfile = 1): Response {
+	public function showFile(string $fileid = null): Response {
+		if (!$fileid) {
+			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
+		}
+
 		// This is the entry point from the `/f/{fileid}` URL which is hardcoded in the server.
 		try {
-			return $this->redirectToFile($fileid, $openfile !== 0);
+			return $this->redirectToFile((int) $fileid);
 		} catch (NotFoundException $e) {
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', ['fileNotFound' => true]));
 		}
 	}
 
+
+	/**
+	 * @NoCSRFRequired
+	 * @NoAdminRequired
+	 * @UseSession
+	 * 
+	 * @param string $dir
+	 * @param string $view
+	 * @param string $fileid
+	 * @param bool $fileNotFound
+	 * @return TemplateResponse|RedirectResponse
+	 */
+	public function indexView($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+		return $this->index($dir, $view, $fileid, $fileNotFound);
+	}
+
 	/**
 	 * @NoCSRFRequired
 	 * @NoAdminRequired
@@ -173,11 +192,30 @@ public function showFile(string $fileid = null, int $openfile = 1): Response {
 	 * @param string $view
 	 * @param string $fileid
 	 * @param bool $fileNotFound
-	 * @param string $openfile - the openfile URL parameter if it was present in the initial request
 	 * @return TemplateResponse|RedirectResponse
-	 * @throws NotFoundException
 	 */
-	public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false, $openfile = null) {
+	public function indexViewFileid($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+		return $this->index($dir, $view, $fileid, $fileNotFound);
+	}
+
+	/**
+	 * @NoCSRFRequired
+	 * @NoAdminRequired
+	 * @UseSession
+	 *
+	 * @param string $dir
+	 * @param string $view
+	 * @param string $fileid
+	 * @param bool $fileNotFound
+	 * @return TemplateResponse|RedirectResponse
+	 */
+	public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+		if ($fileid !== null && $view !== 'trashbin') {
+			try {
+				return $this->redirectToFileIfInTrashbin((int) $fileid);
+			} catch (NotFoundException $e) {}
+		}
+
 		// Load the files we need
 		\OCP\Util::addStyle('files', 'merged');
 		\OCP\Util::addScript('files', 'merged-index', 'files');
@@ -192,8 +230,6 @@ public function index($dir = '', $view = '', $fileid = null, $fileNotFound = fal
 			$favElements['folders'] = [];
 		}
 
-		$contentItems = [];
-
 		try {
 			// If view is files, we use the directory, otherwise we use the root storage
 			$storageInfo =  $this->getStorageInfo(($view === 'files' && $dir) ? $dir : '/');
@@ -237,19 +273,19 @@ public function index($dir = '', $view = '', $fileid = null, $fileNotFound = fal
 		$policy->addAllowedWorkerSrcDomain('\'self\'');
 		$response->setContentSecurityPolicy($policy);
 
-		$this->provideInitialState($dir, $openfile);
+		$this->provideInitialState($dir, $fileid);
 
 		return $response;
 	}
 
 	/**
-	 * Add openFileInfo in initialState if $openfile is set.
+	 * Add openFileInfo in initialState.
 	 * @param string $dir - the ?dir= URL param
-	 * @param string $openfile - the ?openfile= URL param
+	 * @param string $fileid - the fileid URL param
 	 * @return void
 	 */
-	private function provideInitialState(string $dir, ?string $openfile): void {
-		if ($openfile === null) {
+	private function provideInitialState(string $dir, ?string $fileid): void {
+		if ($fileid === null) {
 			return;
 		}
 
@@ -261,7 +297,7 @@ private function provideInitialState(string $dir, ?string $openfile): void {
 
 		$uid = $user->getUID();
 		$userFolder = $this->rootFolder->getUserFolder($uid);
-		$nodes = $userFolder->getById((int) $openfile);
+		$nodes = $userFolder->getById((int) $fileid);
 		$node = array_shift($nodes);
 
 		if ($node === null) {
@@ -293,44 +329,70 @@ private function provideInitialState(string $dir, ?string $openfile): void {
 	}
 
 	/**
-	 * Redirects to the file list and highlight the given file id
+	 * Redirects to the trashbin file list and highlight the given file id
 	 *
-	 * @param string $fileId file id to show
-	 * @param bool $setOpenfile - whether or not to set the openfile URL parameter
+	 * @param int $fileId file id to show
 	 * @return RedirectResponse redirect response or not found response
-	 * @throws \OCP\Files\NotFoundException
+	 * @throws NotFoundException
 	 */
-	private function redirectToFile($fileId, bool $setOpenfile = false) {
+	private function redirectToFileIfInTrashbin($fileId): RedirectResponse {
 		$uid = $this->userSession->getUser()->getUID();
 		$baseFolder = $this->rootFolder->getUserFolder($uid);
-		$files = $baseFolder->getById($fileId);
+		$nodes = $baseFolder->getById($fileId);
 		$params = [];
 
-		if (empty($files) && $this->appManager->isEnabledForUser('files_trashbin')) {
+		if (empty($nodes) && $this->appManager->isEnabledForUser('files_trashbin')) {
+			/** @var Folder */
 			$baseFolder = $this->rootFolder->get($uid . '/files_trashbin/files/');
-			$files = $baseFolder->getById($fileId);
+			$nodes = $baseFolder->getById($fileId);
 			$params['view'] = 'trashbin';
+
+			if (!empty($nodes)) {
+				$node = current($nodes);
+				$params['fileid'] = $fileId;
+				if ($node instanceof Folder) {
+					// set the full path to enter the folder
+					$params['dir'] = $baseFolder->getRelativePath($node->getPath());
+				} else {
+					// set parent path as dir
+					$params['dir'] = $baseFolder->getRelativePath($node->getParent()->getPath());
+				}
+				return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', $params));
+			}
 		}
+		throw new NotFoundException();
+	}
 
-		if (!empty($files)) {
-			$file = current($files);
-			if ($file instanceof Folder) {
+	/**
+	 * Redirects to the file list and highlight the given file id
+	 *
+	 * @param int $fileId file id to show
+	 * @return RedirectResponse redirect response or not found response
+	 * @throws NotFoundException
+	 */
+	private function redirectToFile(int $fileId) {
+		$uid = $this->userSession->getUser()->getUID();
+		$baseFolder = $this->rootFolder->getUserFolder($uid);
+		$nodes = $baseFolder->getById($fileId);
+		$params = [];
+
+		try {
+			$this->redirectToFileIfInTrashbin($fileId);
+		} catch (NotFoundException $e) {}
+
+		if (!empty($nodes)) {
+			$node = current($nodes);
+			$params['fileid'] = $fileId;
+			if ($node instanceof Folder) {
 				// set the full path to enter the folder
-				$params['dir'] = $baseFolder->getRelativePath($file->getPath());
+				$params['dir'] = $baseFolder->getRelativePath($node->getPath());
 			} else {
 				// set parent path as dir
-				$params['dir'] = $baseFolder->getRelativePath($file->getParent()->getPath());
-				// and scroll to the entry
-				$params['scrollto'] = $file->getName();
-
-				if ($setOpenfile) {
-					// forward the openfile URL parameter.
-					$params['openfile'] = $fileId;
-				}
+				$params['dir'] = $baseFolder->getRelativePath($node->getParent()->getPath());
 			}
-
-			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', $params));
+			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', $params));
 		}
-		throw new \OCP\Files\NotFoundException();
+
+		throw new NotFoundException();
 	}
 }

apps/files/src/actions/openInFilesAction.spec.ts

@@ -78,7 +78,7 @@ describe('Open in files action execute tests', () => {
 		// Silent action
 		expect(exec).toBe(null)
 		expect(goToRouteMock).toBeCalledTimes(1)
-		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo', openfile: true })
+		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo' })
 	})
 
 	test('Open in files with folder', async () => {
@@ -98,6 +98,6 @@ describe('Open in files action execute tests', () => {
 		// Silent action
 		expect(exec).toBe(null)
 		expect(goToRouteMock).toBeCalledTimes(1)
-		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo/Bar', openfile: true })
+		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo/Bar' })
 	})
 })

apps/files/src/actions/openInFilesAction.ts

@@ -44,7 +44,7 @@ export const action = new FileAction({
 		window.OCP.Files.Router.goToRoute(
 			null, // use default route
 			{ view: 'files', fileid: node.fileid },
-			{ dir, fileid: node.fileid, openfile: true },
+			{ dir, fileid: node.fileid },
 		)
 		return null
 	},

apps/files/src/actions/sidebarAction.ts

@@ -42,6 +42,10 @@ export const action = new FileAction({
 			return false
 		}
 
+		if (!nodes[0]) {
+			return false
+		}
+
 		// Only work if the sidebar is available
 		if (!window?.OCA?.Files?.Sidebar) {
 			return false
@@ -53,7 +57,7 @@ export const action = new FileAction({
 	async exec(node: Node, view: Navigation) {
 		try {
 			// TODO: migrate Sidebar to use a Node instead
-			window?.OCA?.Files?.Sidebar?.open?.(node.path)
+			await window.OCA.Files.Sidebar.open(node.path)
 
 			// Silently update current fileid
 			window.OCP.Files.Router.goToRoute(

apps/files/src/components/FilesListVirtual.vue

@@ -69,15 +69,17 @@
 <script lang="ts">
 import { translate, translatePlural } from '@nextcloud/l10n'
 import { getFileListHeaders, type Node } from '@nextcloud/files'
+import { showError } from '@nextcloud/dialogs'
 import Vue from 'vue'
 import VirtualList from './VirtualList.vue'
 
+import { action as sidebarAction } from '../actions/sidebarAction.ts'
 import FileEntry from './FileEntry.vue'
 import FilesListHeader from './FilesListHeader.vue'
 import FilesListTableFooter from './FilesListTableFooter.vue'
 import FilesListTableHeader from './FilesListTableHeader.vue'
 import filesListWidthMixin from '../mixins/filesListWidth.ts'
-import { showError } from '@nextcloud/dialogs'
+import logger from '../logger.js'
 
 export default Vue.extend({
 	name: 'FilesListVirtual',
@@ -174,10 +176,10 @@ export default Vue.extend({
 		if (document.documentElement.clientWidth > 1024) {
 			// Open the sidebar on the file if it's in the url and
 			// we're just loaded the app for the first time.
-			const Sidebar = window?.OCA?.Files?.Sidebar
-			const node = this.nodes.find(node => node.fileid === this.fileId) as Node
-			if (Sidebar && node) {
-				Sidebar.open(node.path)
+			const node = this.nodes.find(n => n.fileid === this.fileId) as Node
+			if (node && sidebarAction?.enabled?.([node], this.currentView)) {
+				logger.debug('Opening sidebar on file ' + node.path, { node })
+				sidebarAction.exec(node, this.currentView, this.currentFolder)
 			}
 		}
 	},

apps/files/src/views/Sidebar.vue

@@ -451,39 +451,41 @@ export default {
 		 * @throws {Error} loading failure
 		 */
 		async open(path) {
+			if (!path || path.trim() === '') {
+				throw new Error(`Invalid path '${path}'`)
+			}
+
 			// update current opened file
 			this.Sidebar.file = path
 
-			if (path && path.trim() !== '') {
-				// reset data, keep old fileInfo to not reload all tabs and just hide them
-				this.error = null
-				this.loading = true
+			// reset data, keep old fileInfo to not reload all tabs and just hide them
+			this.error = null
+			this.loading = true
 
-				try {
-					this.fileInfo = await FileInfo(this.davPath)
-					// adding this as fallback because other apps expect it
-					this.fileInfo.dir = this.file.split('/').slice(0, -1).join('/')
-
-					// DEPRECATED legacy views
-					// TODO: remove
-					this.views.forEach(view => {
-						view.setFileInfo(this.fileInfo)
-					})
-
-					this.$nextTick(() => {
-						if (this.$refs.tabs) {
-							this.$refs.tabs.updateTabs()
-						}
-						this.setActiveTab(this.Sidebar.activeTab || this.tabs[0].id)
-					})
-				} catch (error) {
-					this.error = t('files', 'Error while loading the file data')
-					console.error('Error while loading the file data', error)
+			try {
+				this.fileInfo = await FileInfo(this.davPath)
+				// adding this as fallback because other apps expect it
+				this.fileInfo.dir = this.file.split('/').slice(0, -1).join('/')
+
+				// DEPRECATED legacy views
+				// TODO: remove
+				this.views.forEach(view => {
+					view.setFileInfo(this.fileInfo)
+				})
 
-					throw new Error(error)
-				} finally {
-					this.loading = false
-				}
+				this.$nextTick(() => {
+					if (this.$refs.tabs) {
+						this.$refs.tabs.updateTabs()
+					}
+					this.setActiveTab(this.Sidebar.activeTab || this.tabs[0].id)
+				})
+			} catch (error) {
+				this.error = t('files', 'Error while loading the file data')
+				console.error('Error while loading the file data', error)
+
+				throw new Error(error)
+			} finally {
+				this.loading = false
 			}
 		},