systemd: Open path in terminal #21162

ashley-cui · 2024-10-25T04:57:47Z

Allow opening of a path from a URL option, path=path. If the terminal is busy, then warn the user and prompt them to continue.

WIP: changing to a path that does not exist should result in an alert, but there's some css magic that's preventing the alert from showing, and I can't figure out what it is. Also needs tests, but thought I would draft it for feedback so far 😌

Prerequisites:

pkg/systemd/terminal.jsx

pkg/systemd/overview.jsx

pkg/systemd/terminal.jsx

ashley-cui · 2024-10-30T05:32:18Z

Currently in a workable state! Here's what it looks like, both errors are alerts:

@martinpitt If you have any feedback before I polish it and do some URL stuff and write tests, let me know.

martinpitt

Very cool @ashley-cui ! I did a first review round. I can try to help you a bit with the styling, but that's my weak point as well. But until @garrett has time to look at this, we can sort out the code, functionality, and tests.

pkg/systemd/overview.jsx

pkg/systemd/terminal.jsx

martinpitt · 2024-10-30T13:05:54Z

pkg/systemd/terminal.jsx

+            ch.addEventListener("ready", (_, msg) => this.setState({ pid: msg.pid }), { once: true });
+            ch.addEventListener("close", () => this.setState({ pid: null }), { once: true });


once doesn't actually work, please drop. Marius also said that the handlers clean up themselves once the object (channel in this case) gets destroyed, so should be fine.

You resolved this thread, but it's still in the latest push. Reopening.

pkg/systemd/terminal.jsx

martinpitt · 2024-10-30T13:18:47Z

pkg/systemd/terminal.jsx

@@ -186,6 +263,30 @@ const _ = cockpit.gettext;
                            </ToolbarContent>
                        </Toolbar>
                    </div>
+                <div className={"warn"}>


Does this class do anything? I don't see it referenced anywhere. But indeed the alert will need some styling, it should at least have some margin to the terminal and the toolbar. If you plan to keep it, please make it a bit more unique, like ct-terminal-dir-alert

pkg/systemd/terminal.jsx

martinpitt · 2024-10-30T13:20:25Z

pkg/systemd/terminal.scss

-  grid-template-rows: auto 1fr;
+  grid-template-rows: auto auto 1fr;


I don't know enough about grids to see what that does. I figure it's about accomodating the alert, but what if the alert isn't rendered?

ashley-cui · 2024-11-04T14:20:20Z

@martinpitt Here's what's broken, and I can't figure out why. Currently all calls to createChannel() are commented out other than the initial call. Pressing the temporary testing button causes the terminal to disconnect, and doing it several times causes cockpit to disconnect for me.

garrett · 2024-11-06T12:45:57Z

I don't really have time right now to properly test this, but I was looking through my mentions, and it looks pretty good from the screenshots, provided the alerts aren't covering up the terminal content.

The continue action should probably look like a secondary small button.

Allow opening of a path from a URL option, path=path. If the terminal is busy, then warn the user and prompt them to continue. Signed-off-by: Ashley Cui <[email protected]>

ashley-cui · 2024-11-07T05:12:25Z

@martinpitt One more issue: when running with the tests rebased, it fails with error: "Machine" has no attribute "ws_container", it was introduced here, but I don't know how to get it working.

Other than that, should be ready for another review, thanks for bearing with me on this one!

martinpitt · 2024-11-07T05:23:15Z

@ashley-cui Most likely you have an ancient bots/ checkout. Keeping that up to date is our team's morning gymnastics 😆 git -C bots pull. Triggering fedora-40/other here to get an official test run.

martinpitt

Thanks @ashley-cui! Nice work with fixing the issues and adding tests, they look fairly happy. I have a lot of really small stuff, should be quite fast to fix. But let me know if anything is unclear, happy to help!

There's also some outstanding threads from my previous review.

martinpitt · 2024-11-07T06:07:40Z

pkg/systemd/terminal.jsx

@@ -7,6 +7,9 @@ import { createRoot } from "react-dom/client";
 import { FormSelect, FormSelectOption } from "@patternfly/react-core/dist/esm/components/FormSelect/index.js";


Please clean up the commit message:

Should be prefixed by the page ("systemd:" in this case).

Please drop S-o-b, we don't use that in cockpit (and frankly, I don't see the point -- From: is just fine)

martinpitt · 2024-11-07T06:11:48Z

pkg/systemd/terminal.jsx

+            let dir;
+            if (cockpit.location.options.path) {
+                try {
+                    const info = await fsinfo(String(cockpit.location.options.path), ['type']);
+                    if (info.type === "dir") {
+                        dir = cockpit.location.options.path;
+                    } else {
+                        this.setState({ pathError: cockpit.format(_("$0 is not a directory"), cockpit.location.options.path) });
+                    }
+                } catch (err) {
+                    this.setState({ pathError: cockpit.format(_("$0 does not exist"), cockpit.location.options.path) });
+                }
+            }


DRY ("don't repeat yourself") -- please just call this.onNavigate() here.

martinpitt · 2024-11-07T06:15:21Z

pkg/systemd/terminal.jsx

+        dismissError() {
+            this.setState({ pathError: null });
+            cockpit.location.replace("");
+        }
+
+        dismissChange() {
+            this.setState({ changePathBusy: false });
+            cockpit.location.replace("");
+        }
+


This could just be a single dismiss() which resets both states. These errors can't happen at the same time.

However, this is a matter of preference, and I leave this up to you -- feel free to just resolve this thread.

martinpitt · 2024-11-07T06:16:55Z

pkg/systemd/terminal.jsx

+            // Clear old path errors
+            this.setState({ pathError: null });


If this can happen, then changePathBusy could also still be active -- this should probably reset it, too?

martinpitt · 2024-11-07T06:20:18Z

pkg/systemd/terminal.jsx

+                cockpit.script(cmmd, [], { err: "message" })
+                        .then(() => {
+                            this.setState({ changePathBusy: true });
+                        })


Please avoid the unnecessary {} wrappers for arrows, just say () => this.setState(...).

But moreover, this is an async function and you are using await above, so please let's be consistent and also try{ await cockpit.script() } catch {...} here.

Also, personal favour: Please spell "cmmd" as "command", or avoid the const altogether and just inline it into the cockpit.script call.

martinpitt · 2024-11-07T06:42:14Z

test/verify/check-system-terminal

+        b.wait_not_present(".pf-v5-c-alert.pf-m-danger")
+
+        # Error on change to non-dir
+        m.execute("touch /tmp/thisisafile")


Let's please clean up after ourselves, and use self.write_file("/tmp/thisisafile", "") instead (that has an addCleanup to restore or remove it after the test). Or just use an existing file such as /etc/os-release.

martinpitt · 2024-11-07T06:43:18Z

pkg/systemd/terminal.jsx

@@ -186,6 +268,22 @@ const _ = cockpit.gettext;
                            </ToolbarContent>
                        </Toolbar>
                    </div>
+                    <div className="ct-terminal-dir-alert">
+                        {this.state.pathError && <Alert isInline variant='danger'


This isn't really a "danger", there's no potential data loss here. Please change to "warning".

martinpitt · 2024-11-07T06:43:57Z

pkg/systemd/terminal.jsx

+                        title={cockpit.format(_("Error opening directory: $0"), this.state.pathError)}
+                        actionClose={<AlertActionCloseButton onClose={this.dismissError} />} />}
+                        {this.state.changePathBusy && <Alert title={_("There is still a process running in this terminal. Changing the directory will kill it.")}
+                            variant="warning"


This on the other hand should be "danger", as killing a running process may lead to some data loss.

martinpitt · 2024-11-07T06:45:15Z

test/verify/check-system-terminal

+        b.wait_js_cond('window.location.hash == "#/"')
+
+        # Prompt for change if terminal busy
+        b.input_text("vi busybusybusy\n")


I'm not sure if all our images have vi (I only triggered on fedora-40 so far). Let's just use "sleep 100"?

martinpitt · 2024-11-07T06:46:14Z

test/verify/check-system-terminal

+        # Cancel change
+        b.click("button:contains('Cancel')")
+        b.wait_not_present(".pf-v5-c-alert.pf-m-danger")
+        b.wait_js_cond('window.location.hash == "#/"')


Please check here that the "busy" process is still running, and the terminal still shows it. This changes the location hash, so it could very well accidentally destroy the process anyway.

github-advanced-security bot found potential problems Oct 25, 2024

View reviewed changes

pkg/systemd/terminal.jsx Fixed Show fixed Hide fixed

martinpitt added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 25, 2024

cockpituous reviewed Oct 25, 2024

View reviewed changes

Shades51272 approved these changes Oct 27, 2024

View reviewed changes

ashley-cui force-pushed the termpath branch from f74eec6 to e63b08f Compare October 30, 2024 05:11

github-advanced-security bot found potential problems Oct 30, 2024

View reviewed changes

pkg/systemd/terminal.jsx Fixed Show fixed Hide fixed

ashley-cui force-pushed the termpath branch from e63b08f to b3cb9a4 Compare October 30, 2024 05:29

github-advanced-security bot found potential problems Oct 30, 2024

View reviewed changes

pkg/systemd/terminal.jsx Fixed Show fixed Hide fixed

pkg/systemd/terminal.jsx Fixed Show fixed Hide fixed

martinpitt requested changes Oct 30, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

This was referenced Nov 5, 2024

fsinfo / new-style channels mess up Shell message routing #21219

Closed

sosreport: Port to fsinfo, fix fsinfo for <script>ed cockpit.js #21059

Merged

ashley-cui force-pushed the termpath branch from b3cb9a4 to 5e2aaf5 Compare November 7, 2024 05:06

Open path in terminal

c114eb0

Allow opening of a path from a URL option, path=path. If the terminal is busy, then warn the user and prompt them to continue. Signed-off-by: Ashley Cui <[email protected]>

ashley-cui force-pushed the termpath branch from 5e2aaf5 to c114eb0 Compare November 7, 2024 05:07

ashley-cui marked this pull request as ready for review November 7, 2024 05:13

martinpitt self-requested a review November 7, 2024 05:26

martinpitt changed the title ~~[WIP] terminal: Open path in terminal~~ systemd: Open path in terminal Nov 7, 2024

martinpitt requested changes Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemd: Open path in terminal #21162

systemd: Open path in terminal #21162

ashley-cui commented Oct 25, 2024 •

edited by martinpitt

Loading

ashley-cui commented Oct 30, 2024 •

edited

Loading

martinpitt left a comment

martinpitt Oct 30, 2024

martinpitt Nov 7, 2024

martinpitt Oct 30, 2024

martinpitt Oct 30, 2024

ashley-cui commented Nov 4, 2024

This comment was marked as resolved.

This comment was marked as outdated.

garrett commented Nov 6, 2024

ashley-cui commented Nov 7, 2024

martinpitt commented Nov 7, 2024

martinpitt left a comment

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

martinpitt Nov 7, 2024

		ch.addEventListener("ready", (_, msg) => this.setState({ pid: msg.pid }), { once: true });
		ch.addEventListener("close", () => this.setState({ pid: null }), { once: true });

		grid-template-rows: auto 1fr;
		grid-template-rows: auto auto 1fr;

		@@ -7,6 +7,9 @@ import { createRoot } from "react-dom/client";
		import { FormSelect, FormSelectOption } from "@patternfly/react-core/dist/esm/components/FormSelect/index.js";

systemd: Open path in terminal #21162

Are you sure you want to change the base?

systemd: Open path in terminal #21162

Conversation

ashley-cui commented Oct 25, 2024 • edited by martinpitt Loading

ashley-cui commented Oct 30, 2024 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashley-cui commented Nov 4, 2024

This comment was marked as resolved.

This comment was marked as outdated.

garrett commented Nov 6, 2024

ashley-cui commented Nov 7, 2024

martinpitt commented Nov 7, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashley-cui commented Oct 25, 2024 •

edited by martinpitt

Loading

ashley-cui commented Oct 30, 2024 •

edited

Loading