Skip to content
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.

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

Keep the path as string instead of an array #678

Merged
merged 1 commit into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 24 additions & 10 deletions src/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* along with Cockpit; If not, see <http://www.gnu.org/licenses/>.
*/

import React, { useContext, useEffect, useMemo, useState } from "react";
import React, { useContext, useEffect, useState } from "react";

import {
AlertGroup, Alert, AlertVariant, AlertActionCloseButton
Expand Down Expand Up @@ -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}`;
Comment on lines +82 to +83
Copy link
Member Author

Choose a reason for hiding this comment

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

Needs a test :)

Comment on lines +82 to +83
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 added lines are not executed by any test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Test this in a follow up!

}

return currentPath;
};

export const Application = () => {
const { options } = usePageLocation();
const [loading, setLoading] = useState(true);
Expand All @@ -79,10 +98,7 @@ export const Application = () => {
const [alerts, setAlerts] = useState<Alert[]>([]);
const [cwdInfo, setCwdInfo] = useState<FileInfo | null>(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 => {
Expand All @@ -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' }
);
Expand All @@ -126,7 +142,7 @@ export const Application = () => {
client.close();
};
},
[options, currentPath]
[options, path]
);

if (loading)
Expand Down Expand Up @@ -159,9 +175,7 @@ export const Application = () => {
</Alert>
))}
</AlertGroup>
<FilesBreadcrumbs
path={path}
/>
<FilesBreadcrumbs path={path} />
<PageSection>
<Sidebar isPanelRight hasGutter>
<SidebarContent>
Expand Down
6 changes: 5 additions & 1 deletion src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ export function * map_permissions<T>(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 {
Expand Down
6 changes: 3 additions & 3 deletions src/dialogs/permissions.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 };
}

Expand Down Expand Up @@ -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" });
Expand Down
8 changes: 4 additions & 4 deletions src/dialogs/rename.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ function checkCanOverride(candidate: string, entries: Record<string, FileInfo>,

const RenameItemModal = ({ dialogResult, path, selected } : {
dialogResult: DialogResult<void>
path: string[],
path: string,
selected: FolderFileInfo,
}) => {
const { cwdInfo } = useFilesContext();
Expand All @@ -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(() => {
Expand Down Expand Up @@ -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 });
}
54 changes: 29 additions & 25 deletions src/files-breadcrumbs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,20 @@ 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<cockpit.UserInfo | null>(null);
const [bookmarks, setBookmarks] = React.useState<string[]>([]);
const [bookmarkHandle, setBookmarkHandle] = React.useState<cockpit.FileHandle<string> | null>(null);

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}/`;
Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, never seen user.home be /home/admin/ it always is /home/admin so that's logical.

Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

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
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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");
Expand Down Expand Up @@ -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<HTMLElement>) {
const { button, ctrlKey, metaKey } = event;
Expand All @@ -213,8 +217,8 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => {

return (
<Breadcrumb onClick={(event) => 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
Expand All @@ -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 &&
<Tooltip
Expand All @@ -244,7 +248,7 @@ const PathBreadcrumbs = ({ path }: { path: string[] }) => {
};

// 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<string | null>(null);

Expand All @@ -265,7 +269,7 @@ export function FilesBreadcrumbs({ path }: { path: string[] }) {

const enableEditMode = () => {
setEditMode(true);
setNewPath(path.join("/") || "/");
setNewPath(path);
};

const changePath = () => {
Expand Down
8 changes: 4 additions & 4 deletions src/files-card-body.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<React.SetStateAction<FolderFileInfo[]>>,
clipboard: string[], setClipboard: React.Dispatch<React.SetStateAction<string[]>>,
}) => {
Expand Down Expand Up @@ -124,7 +124,7 @@ export const FilesCardBody = ({
setCurrentFilter: React.Dispatch<React.SetStateAction<string>>,
files: FolderFileInfo[],
isGrid: boolean,
path: string[],
path: string,
selected: FolderFileInfo[], setSelected: React.Dispatch<React.SetStateAction<FolderFileInfo[]>>,
sortBy: Sort, setSortBy: React.Dispatch<React.SetStateAction<Sort>>,
loadingFiles: boolean,
Expand Down Expand Up @@ -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) });
}
Expand Down Expand Up @@ -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);
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/files-folder-view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const FilesFolderView = ({
setClipboard,
setShowHidden,
}: {
path: string[],
path: string,
files: FolderFileInfo[],
loadingFiles: boolean,
showHidden: boolean,
Expand Down
2 changes: 1 addition & 1 deletion src/header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export const FilesCardHeader = ({
isGrid: boolean, setIsGrid: React.Dispatch<React.SetStateAction<boolean>>,
sortBy: Sort, setSortBy: React.Dispatch<React.SetStateAction<Sort>>
showHidden: boolean, setShowHidden: React.Dispatch<React.SetStateAction<boolean>>,
path: string[],
path: string,
}) => {
return (
<CardHeader className="card-actionbar">
Expand Down
Loading