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

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

pkg/systemd/terminal.scss

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.

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.

pkg/systemd/terminal.jsx

test/verify/check-system-terminal

pkg/systemd/terminal.jsx

test/verify/check-system-terminal

ashley-cui · 2024-11-14T05:49:08Z

Updated, now looks like this! Thanks for the thourough reviews @martinpitt 😺

martinpitt · 2024-11-14T13:44:07Z

I tested this interactively, and in general it works great!

I noticed two visual issues, though: there should be some separation between the alert and the neighbors. For the "missing directory" one it's still okay-ish:

But the danger one has a missing background color, so it's all white (note I use a white terminal theme):

Inline danger alert should have a red-ish tint, like in the docs. I'm not sure where exactly this goes wrong, as this PR changes no CSS.

What could help is to move the alerts above the existing thin separator line, which you can see without an alert:

It may in fact be better to move the alerts above the header bar even?

martinpitt · 2024-11-14T13:51:44Z

With moving the alert group right after the opening of console-ct-container and adding the missing isInline, it looks like this:

which I find a better, WDYT? @garrett ?

martinpitt

Nice, almost there!

test/verify/check-system-terminal

martinpitt · 2024-11-14T13:47:01Z

pkg/systemd/terminal.jsx

+                pathError: null,
+                changePathBusy: false,
+            });
+            cockpit.location.replace("");


This looks a little odd. The effect is that when I cancel a change dir request due to "busy", it doesn't go back to the previous location, but clears the path entirely. It's mostly cosmetic, as the actual terminal session stays intact.

So mostly thinking aloud, ok.

The reasoning behind this: If we don't clear the path but the user clears the error, and then tries to open the path again, there isn't a change because there's no navigation event, so onNavigate() doesn't run.

pkg/systemd/terminal.jsx

pkg/systemd/terminal.scss

test/verify/check-system-terminal

ashley-cui · 2024-11-14T15:10:54Z

Design wise, I still prefer the alert after the title/toolbar. I feel like this is more of a terminal warning over a page warning, ie it doesn't affect the toolbar, so the alert should be placed below the toolbar, before what's going to be affected, but curious to hear what @garrett thinks.

martinpitt

OK, thanks @ashley-cui ! Good enough then, I don't have a strong opinion about the alert placement. I'd just really like to understand that grid row CSS change..

I dropped the no-test label, so your next push will run the full gamut (so far I only triggered fedora-40). I don't expect any fallout somewhere else, but 🤷‍♂️ 😁

@garrett can you please do the final sign-off? @ashley-cui can you please post updated screenshots (with the isInline change)?

ashley-cui · 2024-11-14T15:40:38Z

martinpitt · 2024-11-14T16:08:20Z

Nevermind -- @garrett already signed this off above sans the secondary button -- which you did. Also, this feature isn't being used from anywhere yet, so if necessary we can make amends once we actually start using it IMHO.

martinpitt · 2024-11-14T18:16:49Z

@ashley-cui dang! On CoreOS this is slightly different, the home dir is indeed under /var/ there. Check e.g. this case:

test/verify/check-pages: self.assertEqual(user_info["home"], "/var/home/admin" if m.ostree_image else "/home/admin")

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.

cockpituous · 2024-11-14T19:27:09Z

pkg/systemd/terminal.jsx

                payload: "stream",
                spawn: [user.shell || "/bin/bash"],
                environ: [
                    "TERM=xterm-256color",
                ],
-                directory: user.home || "/",
+                directory: dir || user.home || "/",


This added line is not executed by any test.

cockpituous · 2024-11-14T19:27:09Z

pkg/systemd/terminal.jsx

+            if (cockpit.location.options.path) {
+                const validPath = await this.readyPath();
+                if (validPath) {
+                    dir = cockpit.location.options.path;


These 4 added lines are not executed by any test.

cockpituous · 2024-11-14T19:27:09Z

pkg/systemd/terminal.jsx

+        componentWillUnmount() {
+            cockpit.removeEventListener("locationchanged", this.onNavigate);


These 2 added lines are not executed by any test.

cockpituous · 2024-11-14T19:27:09Z

pkg/systemd/terminal.jsx

+                    return true;
+                }
+            }
+            return true;


This added line is not executed by any test.

cockpituous · 2024-11-14T19:27:10Z

pkg/systemd/terminal.jsx

+                            actionClose={<AlertActionCloseButton onClose={() =>
+                                this.setState({ changePathBusy: false })} />}


These 2 added lines are not executed by any test.

garrett · 2024-11-18T09:26:07Z

Hey! Thanks for working on this feature and congrats on getting it merged!

I noticed a few issue from the screenshots that should be handled in a follow-up:

Have the error in the title instead of "error": "Directory does not exist"

Text needs to be reworked. Title: "Running process prevents directory change", Body: "Changing the directory will forcefully stop the currently running process." We might want to add a hint that this could be interactive by tacking on an extra sentence, with something like: "The process can also be stopped manually in the terminal before continuing."
Action buttons should say what they will do. "Continue" isn't the action that's being performed; it's "Change directory".
If this is a dangerous action, as the alert state indicates, then the button should reflect that; it shouldn't be a secondary button, but a danger button.

Normally, design related changes should be handled before the PR is merged. I was busy with Anaconda lately, but this still should've been held back until it got a few moments for design review. It's somewhat OK with this feature right now, as people aren't going to see it yet (as it needs to be implemented in Cockpit Files too), but the danger is that these changes that I listed might be accidentally ignored or delayed, and additionally strings will get translated for the current state and then the translations will be thrown away when they have to be re-translated. (We don't want to give translators extra busy work if we can help it.) We generally don't have deadlines for most features in Cockpit, so it's usually fine for something to "slip" to the next release if something doesn't make in time for an immediate release.

My quick feedback in a comment above was that the general approach (alert instead of modals, as modals would cover up the actual content that would need to be interacted with, especially dealing running processes) looked fine. It wasn't meant as a sign-off though.

I'll open up a new issue based on this comment.

(Note: We've all merged things a little too early before. It's OK; the implementation looks great; it just needs a little bit more polish in this case.)

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

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 2 times, most recently from 5e2aaf5 to c114eb0 Compare November 7, 2024 05:07

This comment was marked as resolved.

Sign in to view

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

This comment was marked as resolved.

Sign in to view

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

ashley-cui force-pushed the termpath branch 3 times, most recently from f9fff28 to 5e84f7b Compare November 14, 2024 05:44

martinpitt requested changes Nov 14, 2024

View reviewed changes

ashley-cui force-pushed the termpath branch from 5e84f7b to d4f3d5f Compare November 14, 2024 15:10

ashley-cui requested a review from martinpitt November 14, 2024 15:11

martinpitt requested a review from garrett November 14, 2024 15:29

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

martinpitt previously approved these changes Nov 14, 2024

View reviewed changes

test: Drop unused test line

fb713ad

ashley-cui force-pushed the termpath branch from d4f3d5f to 397cd3b Compare November 14, 2024 15:41

martinpitt removed the request for review from garrett November 14, 2024 16:07

systemd: Open path in terminal

2f57d73

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.

ashley-cui dismissed martinpitt’s stale review via 2f57d73 November 14, 2024 18:33

ashley-cui force-pushed the termpath branch from 397cd3b to 2f57d73 Compare November 14, 2024 18:33

martinpitt approved these changes Nov 14, 2024

View reviewed changes

cockpituous reviewed Nov 14, 2024

View reviewed changes

martinpitt merged commit eb3c0a5 into cockpit-project:main Nov 15, 2024
82 of 85 checks passed

garrett mentioned this pull request Nov 18, 2024

follow-up: systemd: open path in terminal improvements #21281

Closed

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

garrett commented Nov 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

martinpitt left a comment

ashley-cui commented Nov 14, 2024

martinpitt commented Nov 14, 2024

martinpitt commented Nov 14, 2024

martinpitt left a comment

martinpitt Nov 14, 2024

ashley-cui Nov 14, 2024

ashley-cui commented Nov 14, 2024

martinpitt left a comment

ashley-cui commented Nov 14, 2024

martinpitt commented Nov 14, 2024

martinpitt commented Nov 14, 2024

cockpituous Nov 14, 2024

cockpituous Nov 14, 2024

cockpituous Nov 14, 2024

cockpituous Nov 14, 2024

cockpituous Nov 14, 2024

garrett commented Nov 18, 2024 •

edited

Loading

		componentWillUnmount() {
		cockpit.removeEventListener("locationchanged", this.onNavigate);

		actionClose={<AlertActionCloseButton onClose={() =>
		this.setState({ changePathBusy: false })} />}

systemd: Open path in terminal #21162

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

garrett commented Nov 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

martinpitt left a comment

Choose a reason for hiding this comment

ashley-cui commented Nov 14, 2024

martinpitt commented Nov 14, 2024

martinpitt commented Nov 14, 2024

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Nov 14, 2024

Choose a reason for hiding this comment

ashley-cui Nov 14, 2024

Choose a reason for hiding this comment

ashley-cui commented Nov 14, 2024

martinpitt left a comment

Choose a reason for hiding this comment

ashley-cui commented Nov 14, 2024

martinpitt commented Nov 14, 2024

martinpitt commented Nov 14, 2024

cockpituous Nov 14, 2024

Choose a reason for hiding this comment

cockpituous Nov 14, 2024

Choose a reason for hiding this comment

cockpituous Nov 14, 2024

Choose a reason for hiding this comment

cockpituous Nov 14, 2024

Choose a reason for hiding this comment

cockpituous Nov 14, 2024

Choose a reason for hiding this comment

garrett commented Nov 18, 2024 • edited Loading

ashley-cui commented Oct 25, 2024 •

edited by martinpitt

Loading

ashley-cui commented Oct 30, 2024 •

edited

Loading

garrett commented Nov 18, 2024 •

edited

Loading