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

Create file support #794

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Create file support #794

wants to merge 4 commits into from

Conversation

jelly
Copy link
Member

@jelly jelly commented Oct 18, 2024


cockpit-files: File creation support

Files can now create a new file with initial content after creation the dialog changes into editor mode so you can keep editing. When you have "administrative access" the owner of the file is pre-selected based on the owner of the directory so far /home/admin/ it is "admin:admin" and you can also change ownership to root if needed.

image

@jelly jelly added the no-test label Oct 18, 2024
@jelly jelly changed the base branch from create-file to main October 18, 2024 07:53
src/dialogs/editor.tsx Outdated Show resolved Hide resolved
src/dialogs/editor.tsx Outdated Show resolved Hide resolved
src/dialogs/editor.tsx Fixed Show fixed Hide fixed
src/dialogs/create-file.tsx Outdated Show resolved Hide resolved
@jelly jelly changed the title Create file Create file support Nov 1, 2024
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

src/common.ts Outdated Show resolved Hide resolved
src/common.ts Outdated
import { FileInfo } from "cockpit/fsinfo";
import { FileInfo } from "cockpit/fsinfo.ts";
Copy link
Member

Choose a reason for hiding this comment

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

I thought specifying extensions was out of fashion? I don't see a build.js or other config change here which could have triggered this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I just do what eslint wants, I got a bit tired of the constant churn here.

Copy link
Member Author

Choose a reason for hiding this comment

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

# /home/jelle/projects/cockpit-files/src/common.ts
#   21:26  error  Missing file extension "ts" for "cockpit/fsinfo"  import/extensions

🤷 no idea why this happens!

src/dialogs/create-file.tsx Outdated Show resolved Hide resolved
}

async function create_file(filename: string, content?: string, owner?: string) {
// let tag = await read_tag(filename);
Copy link
Member

Choose a reason for hiding this comment

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

Leftover. Perhaps you meant to check here if the file already exists? AFAICS this isn't done anywhere else in the CreateFileModal.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is on save and once you enter the filename in the TextInput using fsinfo's cwdInfo

src/dialogs/create-file.tsx Show resolved Hide resolved

# Create a file as admin, we are superuser and /home owned by admin
b.set_input_text("#file-name", "notes.txt")
b.set_input_text(".file-create-modal textarea", "content")
Copy link
Member

Choose a reason for hiding this comment

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

This should validate that the owner select is visible and contains "admin" and "root".

src/dialogs/create-file.tsx Outdated Show resolved Hide resolved
b.set_input_text(".file-create-modal textarea", "content")
b.click("button.pf-m-primary")

b.wait_visible(".file-editor-modal")
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above: Why does this happen? The creation dialog already sets an initial content, why is the user being thrown into the editor dialog here?

test/check-application Show resolved Hide resolved

# Create a file as admin, we are superuser and /home owned by admin
b.set_input_text("#file-name", "notes.txt")
b.set_input_text(".file-create-modal textarea", "content")
Copy link
Member

Choose a reason for hiding this comment

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

This might be a good place for a pixel test? Everything in the dialog should be predictable, and at this point all the fields are nonempty.

@jelly jelly force-pushed the create-file branch 3 times, most recently from 15b6d51 to d8db5a3 Compare November 6, 2024 09:23
Because Files will gain the ability to create a file and create-item
sounds almost like "create file".
The `.file-editor-title-label` class has a workaround for dark mode and
the lack of spacing.
This will be re-used in the file creation dialog.
Allow users to create file with initial text and directly start editing.
Combining the creation of a file in the editor dialog becomes a
stateful mess as the Editor is not really prepared for creating a new
file and with our `dialogResult` we can easily make both dialogs appear
after each other without race conditions.

The fsreplace1 channel does not yet support changing the ownership of a
new file so the current files implementation does this using a
fsreplace1 to touch a file, chown and then set the contents so the file
would never contain contents which another use should not be able to
read.
Comment on lines +51 to +53
} catch (err) {
console.warn("Cannot create new file", err);
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test.

Comment on lines +59 to +61
} catch (err) {
console.warn("Cannot chown new file", err);
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test.

Comment on lines +72 to +74
} catch (err) {
console.warn("Cannot set initial file text", err);
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test.

Comment on lines +104 to +106
const handleEscape = (event: KeyboardEvent) => {
if (filename !== "" || initialText !== "") {
event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 added lines are not executed by any test.

if (filename !== "" || initialText !== "") {
event.preventDefault();
} else {
dialogResult.reject(new Error("no file created"));
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.

position="top"
title={_("Create file")}
isOpen
onClose={() => dialogResult.reject(new Error("no file created"))}
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.

Comment on lines +161 to +165
<Alert
className="file-editor-alert"
variant="danger"
title={createError}
isInline
Copy link
Contributor

Choose a reason for hiding this comment

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

These 5 added lines are not executed by any test.

isRequired
value={filename}
onChange={(_event, value) => {
setFileNameError(checkFilename(value, cwdInfo?.entries || {}, undefined));
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.

@@ -141,7 +123,7 @@ const RenameItemModal = ({ dialogResult, path, selected } : {
if (name !== selected.name)
renameItem();
else {
setNameError(checkName(name, cwdInfo?.entries || {}, selected));
setNameError(checkFilename(name, cwdInfo?.entries || {}, selected));
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.

@@ -151,7 +133,7 @@ const RenameItemModal = ({ dialogResult, path, selected } : {
autoFocus // eslint-disable-line jsx-a11y/no-autofocus
value={name}
onChange={(_, val) => {
setNameError(checkName(val, cwdInfo?.entries || {}, selected));
setNameError(checkFilename(val, cwdInfo?.entries || {}, selected));
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants