From 83e58f0a30ff9856e4e9f6af2706543d5da63907 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Fri, 26 Jul 2024 15:18:54 +0200 Subject: [PATCH] Keep the path as string instead of an array There is no good reason to keep the path as an array of strings, usually we want to just append a file to the path for file operations as most of the code simply joined the path back to a string. This also makes sure the path is properly formatted as /path/to/thing/ with a leading and trailing /. --- src/app.tsx | 34 ++++++++++++++++------- src/common.ts | 6 ++++- src/dialogs/permissions.jsx | 6 ++--- src/dialogs/rename.tsx | 8 +++--- src/files-breadcrumbs.tsx | 54 ++++++++++++++++++++----------------- src/files-card-body.tsx | 8 +++--- src/files-folder-view.tsx | 2 +- src/header.tsx | 2 +- src/menu.tsx | 17 ++++++------ src/sidebar.tsx | 8 +++--- src/upload-button.tsx | 10 +++---- test/check-application | 21 +++++++++------ test/reference | 2 +- 13 files changed, 101 insertions(+), 77 deletions(-) diff --git a/src/app.tsx b/src/app.tsx index 3aa58212..caecb3e4 100644 --- a/src/app.tsx +++ b/src/app.tsx @@ -17,7 +17,7 @@ * along with Cockpit; If not, see . */ -import React, { useContext, useEffect, useMemo, useState } from "react"; +import React, { useContext, useEffect, useState } from "react"; import { AlertGroup, Alert, AlertVariant, AlertActionCloseButton @@ -67,6 +67,25 @@ export const FilesContext = React.createContext({ export const useFilesContext = () => useContext(FilesContext); +export const usePath = () => { + const { options } = usePageLocation(); + let currentPath = decodeURIComponent(options.path?.toString() || "/"); + + // Trim all trailing slashes + currentPath = currentPath.replace(/\/+$/, ''); + + // Our path will always be `/foo/` formatted + if (!currentPath.endsWith("/")) { + currentPath += "/"; + } + + if (!currentPath.startsWith("/")) { + currentPath = `/${currentPath}`; + } + + return currentPath; +}; + export const Application = () => { const { options } = usePageLocation(); const [loading, setLoading] = useState(true); @@ -79,10 +98,7 @@ export const Application = () => { const [alerts, setAlerts] = useState([]); const [cwdInfo, setCwdInfo] = useState(null); - const currentPath = decodeURIComponent(options.path?.toString() || ""); - // the function itself is not expensive, but `path` is later used in expensive computation - // and changing its reference value on every render causes performance issues - const path = useMemo(() => currentPath?.split("/"), [currentPath]); + const path = usePath(); useEffect(() => { cockpit.user().then(user => { @@ -102,7 +118,7 @@ export const Application = () => { setSelected([]); const client = new FsInfoClient( - `/${currentPath}`, + path, ["type", "mode", "size", "mtime", "user", "group", "target", "entries", "targets"], { superuser: 'try' } ); @@ -126,7 +142,7 @@ export const Application = () => { client.close(); }; }, - [options, currentPath] + [options, path] ); if (loading) @@ -159,9 +175,7 @@ export const Application = () => { ))} - + diff --git a/src/common.ts b/src/common.ts index a72c0ef4..569dbfe0 100644 --- a/src/common.ts +++ b/src/common.ts @@ -53,7 +53,11 @@ export function * map_permissions(func: (value: number, label: string) => T) } export function basename(path: string) { - const elements = path.split('/'); + if (path === '/') { + return path; + } + + const elements = path.replace(/\/$/, '').split('/'); if (elements.length === 0) { return '/'; } else { diff --git a/src/dialogs/permissions.jsx b/src/dialogs/permissions.jsx index 64d65c25..8529ca00 100644 --- a/src/dialogs/permissions.jsx +++ b/src/dialogs/permissions.jsx @@ -32,7 +32,7 @@ import { etc_group_syntax, etc_passwd_syntax } from 'pam_user_parser'; import { superuser } from 'superuser'; import { useFilesContext } from '../app'; -import { map_permissions, inode_types } from '../common'; +import { map_permissions, inode_types, basename } from '../common'; const _ = cockpit.gettext; @@ -41,7 +41,7 @@ const EditPermissionsModal = ({ dialogResult, selected, path }) => { // Nothing selected means we act on the current working directory if (!selected) { - const directory_name = path[path.length - 1]; + const directory_name = basename(path); selected = { ...cwdInfo, isCwd: true, name: directory_name }; } @@ -82,7 +82,7 @@ const EditPermissionsModal = ({ dialogResult, selected, path }) => { const ownerChanged = owner !== selected.user || group !== selected.group; try { - const directory = selected?.isCwd ? path.join("/") : path.join("/") + "/" + selected.name; + const directory = selected?.isCwd ? path : path + selected.name; if (permissionChanged) await cockpit.spawn(["chmod", mode.toString(8), directory], { superuser: "try", err: "message" }); diff --git a/src/dialogs/rename.tsx b/src/dialogs/rename.tsx index ef29afdf..5c8bb4a7 100644 --- a/src/dialogs/rename.tsx +++ b/src/dialogs/rename.tsx @@ -74,7 +74,7 @@ function checkCanOverride(candidate: string, entries: Record, const RenameItemModal = ({ dialogResult, path, selected } : { dialogResult: DialogResult - path: string[], + path: string, selected: FolderFileInfo, }) => { const { cwdInfo } = useFilesContext(); @@ -84,12 +84,12 @@ const RenameItemModal = ({ dialogResult, path, selected } : { const [overrideFileName, setOverrideFileName] = useState(false); const renameItem = (force = false) => { - const newPath = path.join("/") + "/" + name; + const newPath = path + name; const mvCmd = ["mv", "--no-target-directory"]; if (force) { mvCmd.push("--force"); } - mvCmd.push(path.join("/") + "/" + selected.name, newPath); + mvCmd.push(path + selected.name, newPath); cockpit.spawn(mvCmd, { superuser: "try", err: "message" }) .then(() => { @@ -167,6 +167,6 @@ const RenameItemModal = ({ dialogResult, path, selected } : { ); }; -export function show_rename_dialog(dialogs: Dialogs, path: string[], selected: FolderFileInfo) { +export function show_rename_dialog(dialogs: Dialogs, path: string, selected: FolderFileInfo) { dialogs.run(RenameItemModal, { path, selected }); } diff --git a/src/files-breadcrumbs.tsx b/src/files-breadcrumbs.tsx index 0f6fca3a..fe9511f4 100644 --- a/src/files-breadcrumbs.tsx +++ b/src/files-breadcrumbs.tsx @@ -37,7 +37,7 @@ import { basename } from "./common"; const _ = cockpit.gettext; -function BookmarkButton({ path }: { path: string[] }) { +function BookmarkButton({ path }: { path: string }) { const [isOpen, setIsOpen] = React.useState(false); const [user, setUser] = React.useState(null); const [bookmarks, setBookmarks] = React.useState([]); @@ -45,10 +45,12 @@ function BookmarkButton({ path }: { path: string[] }) { const { addAlert, cwdInfo } = useFilesContext(); - const currentPath = path.join("/") || "/"; const defaultBookmarks = []; - if (user?.home) - defaultBookmarks.push({ name: _("Home"), loc: user?.home }); // TODO: add trash + if (user?.home) { + // Ensure the home dir has a trailing / like the path + const home_dir = user.home.endsWith('/') ? user.home : `${user.home}/`; + defaultBookmarks.push({ name: _("Home"), loc: home_dir }); // TODO: add trash + } const parse_uri = (line: string) => { // Drop everything after the space, we don't show renames @@ -58,8 +60,15 @@ function BookmarkButton({ path }: { path: string[] }) { line = line.replace('file://', ''); // Nautilus decodes urls as paths can contain spaces - return line.split('/').map(part => decodeURIComponent(part)) + let bookmark_path = line.split('/').map(part => decodeURIComponent(part)) .join('/'); + + // Ensure the bookmark has a trailing slash + if (!bookmark_path.endsWith('/')) { + bookmark_path = `${bookmark_path}/`; + } + + return bookmark_path; }; useInit(async () => { @@ -99,11 +108,11 @@ function BookmarkButton({ path }: { path: string[] }) { try { await bookmarkHandle.modify((old_content: string) => { - if (bookmarks.includes(currentPath)) { - return old_content.split('\n').filter(line => parse_uri(line) !== currentPath) + if (bookmarks.includes(path)) { + return old_content.split('\n').filter(line => parse_uri(line) !== path) .join('\n'); } else { - const newBoomark = "file://" + path.map(part => encodeURIComponent(part)) + const newBoomark = "file://" + path.split('/').map(part => encodeURIComponent(part)) .join('/') + "\n"; return (old_content || '') + newBoomark; } @@ -129,8 +138,8 @@ function BookmarkButton({ path }: { path: string[] }) { return null; let actionText = null; - if (currentPath !== user.home) { - if (bookmarks.includes(currentPath)) { + if (!defaultBookmarks.some(bkmark => bkmark.loc === path)) { + if (bookmarks.includes(path)) { actionText = _("Remove current directory"); } else if (cwdInfo !== null) { actionText = _("Add bookmark"); @@ -182,16 +191,11 @@ function BookmarkButton({ path }: { path: string[] }) { ); } -const PathBreadcrumbs = ({ path }: { path: string[] }) => { - // HACK: strip extraneous slashes as PF's breadcrumb can't handle them - // Refactor the path to be the fullpath not an array of strings - if (path.length >= 2 && path[0] === '' && path[1] === '') { - path = path.slice(1); - } - - if (path.length > 1 && path[path.length - 1] === '') { - path = path.slice(path.length - 1); - } +const PathBreadcrumbs = ({ path }: { path: string }) => { + // Strip the trailing slash as it gets in the way when splitting paths and + // adds an extra trailing slash in the UI which we don't want to show and + // causes duplicate keys for the path `/`. + const path_array = path.replace(/\/$/, "").split("/"); function navigate(event: React.MouseEvent) { const { button, ctrlKey, metaKey } = event; @@ -213,8 +217,8 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => { return ( navigate(event)}> - {path.map((dir, i) => { - const url_path = path.slice(0, i + 1).join("/") || '/'; + {path_array.map((dir, i) => { + const url_path = path_array.slice(0, i + 1).join("/") || '/'; // We can't use a relative path as that will use the iframe's // url while we want the outer shell url. And we can't obtain // the full path of the shell easily, so a middle click will @@ -226,7 +230,7 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => { key={url_path} data-location={url_path} to={link} - isActive={i === path.length - 1} + isActive={i === path_array.length - 1} > {i === 0 && { }; // eslint-disable-next-line max-len -export function FilesBreadcrumbs({ path }: { path: string[] }) { +export function FilesBreadcrumbs({ path }: { path: string }) { const [editMode, setEditMode] = React.useState(false); const [newPath, setNewPath] = React.useState(null); @@ -265,7 +269,7 @@ export function FilesBreadcrumbs({ path }: { path: string[] }) { const enableEditMode = () => { setEditMode(true); - setNewPath(path.join("/") || "/"); + setNewPath(path); }; const changePath = () => { diff --git a/src/files-card-body.tsx b/src/files-card-body.tsx index 486111bd..b3710250 100644 --- a/src/files-card-body.tsx +++ b/src/files-card-body.tsx @@ -75,7 +75,7 @@ function compare(sortBy: Sort): (a: FolderFileInfo, b: FolderFileInfo) => number } const ContextMenuItems = ({ path, selected, setSelected, clipboard, setClipboard } : { - path: string[], + path: string, selected: FolderFileInfo[], setSelected: React.Dispatch>, clipboard: string[], setClipboard: React.Dispatch>, }) => { @@ -124,7 +124,7 @@ export const FilesCardBody = ({ setCurrentFilter: React.Dispatch>, files: FolderFileInfo[], isGrid: boolean, - path: string[], + path: string, selected: FolderFileInfo[], setSelected: React.Dispatch>, sortBy: Sort, setSortBy: React.Dispatch>, loadingFiles: boolean, @@ -159,7 +159,7 @@ export const FilesCardBody = ({ } const onDoubleClickNavigate = useCallback((file: FolderFileInfo) => { - const newPath = [...path, file.name].join("/"); + const newPath = path + file.name; if (file.to === "dir") { cockpit.location.go("/", { path: encodeURIComponent(newPath) }); } @@ -281,7 +281,7 @@ export const FilesCardBody = ({ } else if (e.key === "Enter" && selected.length === 1) { onDoubleClickNavigate(selected[0]); } else if (e.key === "Delete" && selected.length !== 0) { - confirm_delete(dialogs, path.join("/") + "/", selected, setSelected); + confirm_delete(dialogs, path, selected, setSelected); } }; diff --git a/src/files-folder-view.tsx b/src/files-folder-view.tsx index e7c0cae2..8f388020 100644 --- a/src/files-folder-view.tsx +++ b/src/files-folder-view.tsx @@ -37,7 +37,7 @@ export const FilesFolderView = ({ setClipboard, setShowHidden, }: { - path: string[], + path: string, files: FolderFileInfo[], loadingFiles: boolean, showHidden: boolean, diff --git a/src/header.tsx b/src/header.tsx index bc34caba..7e773a67 100644 --- a/src/header.tsx +++ b/src/header.tsx @@ -124,7 +124,7 @@ export const FilesCardHeader = ({ isGrid: boolean, setIsGrid: React.Dispatch>, sortBy: Sort, setSortBy: React.Dispatch> showHidden: boolean, setShowHidden: React.Dispatch>, - path: string[], + path: string, }) => { return ( diff --git a/src/menu.tsx b/src/menu.tsx index b21b83ce..e79d0ce8 100644 --- a/src/menu.tsx +++ b/src/menu.tsx @@ -45,14 +45,13 @@ type MenuItem = { type: "divider" } | { }; export function get_menu_items( - path: string[], + path: string, selected: FolderFileInfo[], setSelected: React.Dispatch>, clipboard: string[], setClipboard: React.Dispatch>, cwdInfo: FileInfo | null, addAlert: (title: string, variant: AlertVariant, key: string, detail?: string) => void, dialogs: Dialogs, ) { - const currentPath = path.join("/") + "/"; const menuItems: MenuItem[] = []; if (selected.length === 0) { @@ -73,7 +72,7 @@ export function get_menu_items( "cp", "-R", ...clipboard, - currentPath + path ]).catch(err => addAlert(err.message, AlertVariant.danger, `${new Date().getTime()}`)); } }, @@ -81,7 +80,7 @@ export function get_menu_items( { id: "create-item", title: _("Create directory"), - onClick: () => show_create_directory_dialog(dialogs, currentPath) + onClick: () => show_create_directory_dialog(dialogs, path) }, { type: "divider" }, { @@ -95,7 +94,7 @@ export function get_menu_items( { id: "copy-item", title: _("Copy"), - onClick: () => setClipboard([currentPath + selected[0].name]), + onClick: () => setClipboard([path + selected[0].name]), }, { type: "divider" }, { @@ -113,7 +112,7 @@ export function get_menu_items( id: "delete-item", title: _("Delete"), className: "pf-m-danger", - onClick: () => confirm_delete(dialogs, currentPath, selected, setSelected) + onClick: () => confirm_delete(dialogs, path, selected, setSelected) }, ); if (selected[0].type === "reg") @@ -122,7 +121,7 @@ export function get_menu_items( { id: "download-item", title: _("Download"), - onClick: () => downloadFile(currentPath, selected[0]) + onClick: () => downloadFile(path, selected[0]) } ); } else if (selected.length > 1) { @@ -130,13 +129,13 @@ export function get_menu_items( { id: "copy-item", title: _("Copy"), - onClick: () => setClipboard(selected.map(s => path.join("/") + "/" + s.name)), + onClick: () => setClipboard(selected.map(s => path + s.name)), }, { id: "delete-item", title: _("Delete"), className: "pf-m-danger", - onClick: () => confirm_delete(dialogs, currentPath, selected, setSelected) + onClick: () => confirm_delete(dialogs, path, selected, setSelected) } ); } diff --git a/src/sidebar.tsx b/src/sidebar.tsx index b4839ad4..4e0d0e38 100644 --- a/src/sidebar.tsx +++ b/src/sidebar.tsx @@ -37,7 +37,7 @@ import { useDialogs } from "dialogs"; import * as timeformat from "timeformat"; import { FolderFileInfo, useFilesContext } from "./app"; -import { get_permissions } from "./common"; +import { basename, get_permissions } from "./common"; import { edit_permissions } from "./dialogs/permissions"; import { get_menu_items } from "./menu"; @@ -93,7 +93,7 @@ function getDescriptionListItems(selected: FolderFileInfo) { export const SidebarPanelDetails = ({ files, path, selected, setSelected, showHidden, clipboard, setClipboard } : { files: FolderFileInfo[], - path: string[], + path: string, selected: FolderFileInfo[], setSelected: React.Dispatch>, showHidden: boolean, clipboard: string[], setClipboard: React.Dispatch> @@ -103,7 +103,7 @@ export const SidebarPanelDetails = ({ files, path, selected, setSelected, showHi useEffect(() => { if (selected.length === 1) { - const filePath = path.join("/") + "/" + selected[0]?.name; + const filePath = path + selected[0]?.name; cockpit.spawn(["file", "--brief", filePath], { superuser: "try", err: "message" }) .then(res => setInfo(res?.trim())) @@ -112,7 +112,7 @@ export const SidebarPanelDetails = ({ files, path, selected, setSelected, showHi }, [path, selected]); const dialogs = useDialogs(); - const directory_name = path[path.length - 1]; + const directory_name = basename(path); const hidden_count = files.filter(file => file.name.startsWith(".")).length; let shown_items = cockpit.format(cockpit.ngettext("$0 item", "$0 items", files.length), files.length); if (!showHidden) diff --git a/src/upload-button.tsx b/src/upload-button.tsx index c6197190..391e2f0f 100644 --- a/src/upload-button.tsx +++ b/src/upload-button.tsx @@ -55,7 +55,7 @@ const FileConflictDialog = ({ isMultiUpload, dialogResult }: { - path: string[]; + path: string; file: FileInfo, uploadFile: File, isMultiUpload: boolean, @@ -96,7 +96,7 @@ const FileConflictDialog = ({

{cockpit.format( _("A file with the same name already exists in \"$0\". Replacing it will overwrite its content."), - path.join('/') + path )}

{ const ref = useRef(null); const { addAlert, cwdInfo } = useFilesContext(); @@ -209,9 +209,7 @@ export const UploadButton = ({ const cancelledUploads = []; await Promise.allSettled(toUploadFiles.map(async (file: File) => { - const tmp_path = path.slice(); - tmp_path.push(file.name); - const destination = tmp_path.join('/'); + const destination = path + file.name; const abort = new AbortController(); setUploadedFiles(oldFiles => { diff --git a/test/check-application b/test/check-application index d62202aa..93582fc2 100755 --- a/test/check-application +++ b/test/check-application @@ -260,6 +260,10 @@ class TestFiles(testlib.MachineCase): b.wait_in_text(".pf-v5-c-empty-state__body", "No such file or directory") b.wait_visible("#dropdown-menu:disabled") + # Path with multiple slashes is normalized + b.go("/files#/?path=/////") + b.wait_text("li[data-location='/']", "") + def testNavigation(self) -> None: b = self.browser m = self.machine @@ -346,7 +350,7 @@ class TestFiles(testlib.MachineCase): # Via escape b.click(edit_button) - b.wait_val(path_input, "/home") + b.wait_val(path_input, "/home/") b.set_input_text(path_input, "/home/admin") b.wait_visible(path_input) b.focus(path_input) @@ -356,7 +360,7 @@ class TestFiles(testlib.MachineCase): # Via cancel button b.click(edit_button) # Cancelled edit should not save the path - b.wait_val(path_input, "/home") + b.wait_val(path_input, "/home/") b.click(cancel_button) b.wait_not_present(path_input) @@ -382,7 +386,7 @@ class TestFiles(testlib.MachineCase): b.wait_not_present(path_input) b.click(edit_button) b.wait_visible(path_input) - b.wait_val(path_input, "/var") + b.wait_val(path_input, "/var/") b.click(cancel_button) # Editing / shows / in the input @@ -1193,7 +1197,7 @@ class TestFiles(testlib.MachineCase): b.click("button:contains('Edit permissions')") select_access("7") b.click("button.pf-m-primary") - self.wait_modal_inline_alert("chmod: changing permissions of '//home': Operation not permitted") + self.wait_modal_inline_alert("chmod: changing permissions of '/home': Operation not permitted") b.click("button.pf-m-link") b.wait_not_present(".pf-v5-c-modal-box") @@ -1832,6 +1836,7 @@ class TestFiles(testlib.MachineCase): b.wait_not_present(".pf-v5-c-menu .pf-v5-c-menu__list-item") b.click("#bookmark-btn") + # There is only one menu item b.wait_in_text(".pf-v5-c-menu .pf-v5-c-menu__list-item", "Home") # Home directory cannot be added or removed as bookmark b.wait_not_present(".pf-v5-c-menu .pf-v5-c-menu__list-item:contains(Add bookmark)") @@ -1894,7 +1899,7 @@ class TestFiles(testlib.MachineCase): b.click(".pf-v5-c-menu .pf-v5-c-menu__list-item:contains(Add bookmark) button") assert_bookmark("This is a-test", exists=True) - self.assertIn("file:///home/admin/This%20is%20a-test\n", read_config()) + self.assertIn("file:///home/admin/This%20is%20a-test/\n", read_config()) # Removing /tmp bookmark keeps bookmark with spaces b.click("#bookmark-btn") @@ -1906,7 +1911,7 @@ class TestFiles(testlib.MachineCase): b.wait_not_present(".pf-v5-c-menu") assert_bookmark("/tmp", exists=False) - self.assertEqual(read_config(), "file:///etc\nfile:///home/admin/This%20is%20a-test\n") + self.assertEqual(read_config(), "file:///etc/\nfile:///home/admin/This%20is%20a-test/\n") # Add a directory with special characters special_dir = "super#special*1 2><.,ß" @@ -1920,7 +1925,7 @@ class TestFiles(testlib.MachineCase): b.click(".pf-v5-c-menu .pf-v5-c-menu__list-item:contains(Add bookmark) button") assert_bookmark(special_dir, exists=True) - self.assertEqual("file:///etc\nfile:///home/admin/This%20is%20a-test\nfile:///home/admin/super%23special*1%202%3E%3C.%2C%C3%9F\n", + self.assertEqual("file:///etc/\nfile:///home/admin/This%20is%20a-test/\nfile:///home/admin/super%23special*1%202%3E%3C.%2C%C3%9F/\n", read_config()) b.click("li[data-location='/home/admin'] a") @@ -1935,7 +1940,7 @@ class TestFiles(testlib.MachineCase): b.wait_not_present(".pf-v5-c-menu") assert_bookmark(special_dir, exists=False) - self.assertEqual(read_config(), "file:///etc\nfile:///home/admin/This%20is%20a-test\n") + self.assertEqual(read_config(), "file:///etc/\nfile:///home/admin/This%20is%20a-test/\n") # Modifications outside of Cockpit are shown m.execute(""" diff --git a/test/reference b/test/reference index 63d006de..d2b08635 160000 --- a/test/reference +++ b/test/reference @@ -1 +1 @@ -Subproject commit 63d006de304f5912d9a388ea7e1ea74699ba3264 +Subproject commit d2b08635d9ba1c18ba37cd3f87640a9204849a20