Implement bookmark support #542

jelly · 2024-06-10T02:37:58Z

Closes: #522

write tests

Bookmark support

Users can now bookmark the current directory and easily navigate to previously bookmarked directories.

allisonkarlitskaya

First thought: https://www.freedesktop.org/wiki/Specifications/desktop-bookmark-spec/

Let's not make this per-browser?

allisonkarlitskaya · 2024-06-10T05:20:55Z

First thought: https://www.freedesktop.org/wiki/Specifications/desktop-bookmark-spec/

Let's not make this per-browser?

I take it back. Nothing seems to use this. Nautilus stores this in ~/.config/gtk-3.0/bookmarks in a flatfile, one per line.

I think it makes sense to match GNOME behaviour where possible, and despite the odd filename, I think it's appropriate in this case too.

jelly · 2024-06-10T10:00:26Z

First thought: https://www.freedesktop.org/wiki/Specifications/desktop-bookmark-spec/
Let's not make this per-browser?

I take it back. Nothing seems to use this. Nautilus stores this in ~/.config/gtk-3.0/bookmarks in a flatfile, one per line.

I think it makes sense to match GNOME behaviour where possible, and despite the odd filename, I think it's appropriate in this case too.

You mean, Cockpit would write to .config/gtk-3.0/bookmarks? That feels fairly odd and non-discoverable. In reality all our settings use localStorage so this would deviate from that which I don't think is worth it.

jelly · 2024-06-10T10:02:02Z

wait_js_cond(ph_is_present(".breadcrumb-button:nth-of-type(2):not([disabled]):not([aria-disabled=true])")): Uncaught (in promise) Error: condition did not become true

This ain't working, lets not make this dependent on a fixed offset.

jelly · 2024-06-11T12:24:04Z

First thought: https://www.freedesktop.org/wiki/Specifications/desktop-bookmark-spec/
Let's not make this per-browser?

I take it back. Nothing seems to use this. Nautilus stores this in ~/.config/gtk-3.0/bookmarks in a flatfile, one per line.

I think it makes sense to match GNOME behaviour where possible, and despite the odd filename, I think it's appropriate in this case too.

This is really not something we should do, it is relevant for the desktop but Cockpit isn't the desktop we store all our settings in localstorage so this would deviate from it (for no real reason)

jelly · 2024-06-18T13:56:24Z

~/.config/gtk-3.0/bookmarks

Thinking about this again, I don't like this solution at all, it is super random and a GNOME implementation detail.
The nautilus used location is of course going to change in the future by gnome and we have no way to chase this.

The format is not a simple "flat file" at all it is formatted as:

file:///home/admin/foo
nfs:///whatever/share

jelly · 2024-06-18T16:22:30Z

src/files-breadcrumbs.tsx

+                    if (old_content === null) {
+                        return `${currentPath}\n`;
+                    } else {
+                        return old_content + `${currentPath}\n`;


This should be tested, is this a coverage issue or a missing test?

jelly · 2024-06-18T16:32:35Z

src/files-breadcrumbs.tsx

+        } catch (err) {
+            console.error("Unable to save bookmark", err);


We could fire an addAlert here but testing this might be tricky but we could slap a chattr +i on bookmarks

garrett · 2024-06-19T08:58:44Z

Nautilus allows renaming bookmarks. We don't parse the file correctly to handle that:

It tries to use the renamed version as part of the path, and we get a broken page like this:

We should support the renames (possibly even by not using the special name, but just the path). It's URI, then space, then the display name (which may include spaces). The URI has encoded spaces, like %20.
We should verify the paths exist before showing them in the UI

I do see that a remote path (ssh) does not show up, which is expected.

Additionally, there are a few problems related to the implementation:

Spacing is super weird on the bookmarks dropdown. I mentioned this on Matrix. I'll look into it. This is likely a PF bug, probably related to the double padding issue we filed (and worked around, possibly too specifically) a few weeks ago.
The first item always has a highlight, even when hovering over other items. This is 100% related to another issue we filed with PF a few weeks ago that they still haven't fixed, and we do not have a workaround for, as this is a JS issue, not a CSS one. I'm not sure if there's anything we can do about this, except complain to PatternFly even more loudly?
Clicking on the button when it's expanded doesn't dismiss the dropdown as one would expect. There's currently no way to dismiss the dropdown unless you select an item or click somewhere else (which may have unintended consiquences).

And, for a separate PR:

The "not found" error page is really odd with regard to the layout; it doesn't have proper alignment and spacing. It's also clearly a directory that's not found, not a file. Related: When you add a file to the path, it says "empty directory" (but has a different, but proper layout).

garrett · 2024-06-19T09:07:08Z

Additionally, when you have a space in a path (which is encoded in the URI as %20) and bookmark that, the UI is broken.

Do this:

create a directory like "this is a test" (with spaces)
enter that directory
bookmark it
reload the page

jelly · 2024-06-19T09:11:54Z

Nautilus allows renaming bookmarks. We don't parse the file correctly to handle that:

It tries to use the renamed version as part of the path, and we get a broken page like this:
1. We should support the renames (possibly even by not using the special name, but just the path). It's URI, then space, then the display name (which may include spaces). The URI has encoded spaces, like `%20`.

So renaming bookmarks means not changing the entry but giving it a different name

2. We should verify the paths exist before showing them in the UI

That is rather expensive and we have no UI for it. What do we do, hide them? Mark them invalid.

Additionally, there are a few problems related to the implementation:

3. Spacing is super weird on the bookmarks dropdown. I mentioned this on Matrix. I'll look into it. This is likely a PF bug, probably related to the double padding issue we filed (and worked around, possibly too specifically) a few weeks ago.

Right, this is patternfly/patternfly#6632

4. The first item always has a highlight, even when hovering over other items. This is 100% related to another issue we filed with PF a few weeks ago that they still haven't fixed, and we _do not_ have a workaround for, as this is a JS issue, not a CSS one. I'm not sure if there's anything we can do about this, except complain to PatternFly even more loudly?

This is a PF design, it is intentionally done in their dropdown demo. Nothing I can do here.

5. Clicking on the button when it's expanded doesn't dismiss the dropdown as one would expect. There's currently no way to dismiss the dropdown unless you select an item or click somewhere else (which may have unintended consiquences).

That is a bug, I'll look into it!

And, for a separate PR:

6. The "not found" error page is really odd with regard to the layout; it doesn't have proper alignment and spacing. It's also clearly a directory that's not found, not a file. Related: When you add a file to the path, it says "empty directory" (but has a different, but proper layout).

It has been broken by the re-design/layout, needs fixing. #568

Meanwhile there is a PR to add pixel tests to catch these issues.

garrett · 2024-06-19T09:24:06Z

So renaming bookmarks means not changing the entry but giving it a different name

Yes, but we can just drop the space and everything after, only caring about the path. We don't have to show the custom name; we just can't break the path due to the custom name.

That is rather expensive and we have no UI for it. What do we do, hide them? Mark them invalid.

Well, at least it seems you can still browse to the directory (with a page with a broken layout) and remove the bookmark. I guess that's OK for this PR.

The first item always has a highlight, even when hovering over other items. This is 100% related to another issue we filed with PF a few weeks ago that they still haven't fixed, and we do not have a workaround for, as this is a JS issue, not a CSS one. I'm not sure if there's anything we can do about this, except complain to PatternFly even more loudly?

This is a PF design, it is intentionally done in their dropdown demo. Nothing I can do here.

It's literally broken — but, yes, they need to fix it. Issue @ cockpit-project/cockpit#20526

A side effect is that there are two items that have highlights, not just the first one having it by default:

It's outside the scope of this PR, agreed.

garrett · 2024-06-19T15:57:56Z

I had to work around widgets being inside the breadcrumb bar. I don't think that's supposed to happen. I think it's supposed to be a top bar with layout that has buttons, then the breadcrumbs, and the rest.

I think we have something like this:

flex
- breadcrumbs
  - bookmarks
  - edit
  - breadcrumb 0
  - breadcrumb 1
  - etc.
- actions

But it's supposed to be more like this:

topbar
- flex layout
  - bookmarks
  - edit
  - breadcrumbs (flex: auto)
    - breadcrumb 0
    - breadcrumb 1
    - etc.
  - actions

jelly · 2024-06-19T16:00:55Z

src/app.scss

@@ -78,6 +78,11 @@

 button.pf-m-secondary.breadcrumb-edit-button {
    margin-inline-end: var(--pf-v5-global--spacer--sm);
+
+    // PatternFly doesn't expect icons in buttons in breadcrumbs, so we need to adjust for that


This comment seems wrong? This is the edit path button.

The edit path button is a button with an icon and it's nested under the breadcrumb component... that's exactly what this comment is talking about.

I'm pretty sure the breadcrumb component is supposed to contain breadcrumbs and nothing else. See my comment above @ #542 (comment)

I added some workaround CSS meanwhile, which is what this comment is talking about.

(The thing is, it also affects the bookmark dropdown, which also is in the breadcrumb widget, which also should probably not be. And to fix one, I needed to fix both and refactor the CSS a bit, which is why it's in this PR.)

allisonkarlitskaya

I mostly just reviewed the "inside" bits, and they look good (but see optional suggestions). I don't feel able to sign off on the UI.

src/files-breadcrumbs.tsx

allisonkarlitskaya · 2024-06-24T14:01:52Z

src/files-breadcrumbs.tsx

+
+        try {
+            await bookmarkHandle.modify((old_content: string) => {
+                const newBoomark = `${encodeURI(currentPath)}\n`;


I see what you do now. This seems strange. We add the file:/// in one place and then carry a weird quasi-URI around for a while and then add the escaping once we get here. Why not just do everything in one spot? I also still think it would make sense to do the encoding on the path components (with encodeURIComponent) and then join them together and add file:///.

allisonkarlitskaya · 2024-06-24T14:02:33Z

src/files-breadcrumbs.tsx

+        try {
+            await bookmarkHandle.modify((old_content: string) => {
+                const newBoomark = `${encodeURI(currentPath)}\n`;
+                if (bookmarks.includes(currentPath)) {


I think this will fail to correctly detect a bookmark which has escaped characters in it, since this is still the unescaped version. Probably in that case we'll add the bookmark twice?

jelly · 2024-06-25T09:57:28Z

src/files-breadcrumbs.tsx

+                    <Divider key="bookmark-divider" />}
+                {bookmarks.map((bookmark: string) => (
+                    <DropdownItem key={bookmark} value={bookmark}>
+                        {bookmark.replace("file://", "")}


No longer needed now.

allisonkarlitskaya

This looks substantially improved from previous versions, thanks.

A couple comments below.

src/files-breadcrumbs.tsx

allisonkarlitskaya · 2024-06-25T10:44:47Z

src/files-breadcrumbs.tsx

+                const newBoomark = path.map(part => encodeURIComponent(part))
+                        .join('/');
+                if (bookmarks.includes(currentPath)) {
+                    return old_content.split('\n').filter(line => !line.startsWith(`file://${newBoomark}`))


This will fail to detect an entry in the bookmarks file which had a different encoding than the one we'd have selected. We'll match our bookmarks.includes() check above (because we parsed the entries out for that one) but fail to match on our encoding.

I think it might be interesting to reuse the parser code from our .watch() handler, which is quite good. It could find the line that matches, and then we could remove that one from the list. We know we're sure to find it, because it was the line that resulted in us having the bookmark in the first place.

That could work as a filter as well, something like

return old_content.split('\n').filter(line => parse_uri(line) !== currentPath).join('\n')

This is probably worth adding a test case over.

Also: this code is slightly weird. It considers the zero bytes after the last newline in the file as if they were a bookmark entry. In that case, it's zero bytes, so it doesn't match, but ... it feels weird. It might be nice to have some "proper" line-splitting code here.

I wonder if you'd consider using a simple "syntax" object on your file handle to take care of the line splitting and re-joining. Then you could just interact directly with lines in both the watcher and the modifier.

I think it might be interesting to reuse the parser code from our .watch() handler, which is quite good. It could find the line that matches, and then we could remove that one from the list. We know we're sure to find it, because it was the line that resulted in us having the bookmark in the first place.

Took me a while to parse this, the parse_uri approach seems to be fine. I'm happy to make those changes.

Also: this code is slightly weird. It considers the zero bytes after the last newline in the file as if they were a bookmark entry. In that case, it's zero bytes, so it doesn't match, but ... it feels weird. It might be nice to have some "proper" line-splitting code here.

The zero byte case, I mean, in reality it is hardly a problem? I have no idea what you mean with "proper" line splitting, just trimming out the junk?

I wonder if you'd consider using a simple "syntax" object on your file handle to take care of the line splitting and re-joining. Then you could just interact directly with lines in both the watcher and the modifier.

Like how would that help or make things easier? Its still going to be JavaScript which does the parsing and it would mean we have to re-construct the whole file when modifying from the state we have, which is imo not what we want.

We don't even care about a zero byte in /etc/passwd, why is this file so special? What we do do is trim whitespace (newline or whitespace character). Which we can also implement.

content = (content || "").trim();

proper line splitting

Like something that converts "a\nb\n" to ['a', 'b'] rather than ['a', 'b', ''] and then ['a', 'b'] to "a\nb\n" rather than "a\nb"`.

allisonkarlitskaya

The "bookmark file handling" code now looks completely correct to me. Thanks for iterating on this. See an idea for a test case, though.

UI/React/PF review needs to come from someone else.

allisonkarlitskaya · 2024-06-25T12:42:16Z

src/files-breadcrumbs.tsx

@@ -122,7 +125,7 @@ function BookmarkButton({ path }: { path: string[] }) {
                const newBoomark = path.map(part => encodeURIComponent(part))


This could probably be moved inside of the } else { case now that you only use the variable on that path. You might also add file:// to it as part of the same process, instead of doing it in two steps.

This is still unaddressed. If you do another round, please move this closer to where it's used.

allisonkarlitskaya · 2024-06-25T12:45:32Z

src/files-breadcrumbs.tsx

@@ -122,7 +125,7 @@ function BookmarkButton({ path }: { path: string[] }) {
                const newBoomark = path.map(part => encodeURIComponent(part))
                        .join('/');
                if (bookmarks.includes(currentPath)) {
-                    return old_content.split('\n').filter(line => !line.startsWith(`file://${newBoomark}`))


This looks like it'll work now.

A sketch for a testcase: take a "special" character that we know won't be escaped, something like - (which can also be written %2d).

Then

mkdir /home/admin/test-dir echo file:///home/admin/test%2ddir >> [bookmarks]

Then verify that:

we can use the bookmark to navigate to the new directory (even though it has a weird encoding)

we can remove the bookmark

we can check the file to see that the bookmark has been removed, and we can check the UI to see that it's been updated as expected

we can add it back again

the encoding that it got added back with is different than the original one (ie: check that test-dir appears in the bookmarks file now)

Right because:

decodeURIComponent("test%2ddir") "test-dir"

encodeURIComponent("test-dir") "test-dir"

I just reworked the test with spaces to use %2d so this is now fully tested.

garrett · 2024-06-25T12:57:04Z

Bugs:

Navigating to a location that no longer exists causes the bookmarks button to become disabled, so you can no longer remove it or use it to navigate elsewhere.
I sometimes get weird issues where Cockpit Files no longer responds when adding directories with spaces, commas, and such. This could be some kind of Heisenbug, and might not even be related to this PR, however. I've seen it a couple of times, but haven't been able to reliably recreate the issue. It could be with adding anything (might not be spaces or commas or parenthesis or such), and it might be something else entirely.

Also:

The screenshot is out of date. This should be updated; it can be updated after the PR lands.
There's no tooltip, but I opened an issue about this and the edit button at Add tooltips for icons, including icon-only action buttons #581, so we can handle it as a follow-up.

allisonkarlitskaya

From me: this is ready to go. It seemed @garrett had some comments on the last round so I'll let him chime in on this one with the official +1/-1.

allisonkarlitskaya · 2024-06-25T15:10:00Z

src/files-breadcrumbs.tsx

@@ -122,7 +125,7 @@ function BookmarkButton({ path }: { path: string[] }) {
                const newBoomark = path.map(part => encodeURIComponent(part))


This is still unaddressed. If you do another round, please move this closer to where it's used.

jelly · 2024-06-25T16:12:01Z

From me: this is ready to go. It seemed @garrett had some comments on the last round so I'll let him chime in on this one with the official +1/-1.

The removal is already fixed and the other case I can't reproduce.

garrett

Approving again; my last approval from a minute or two ago with the screenshot and mentioning the follow-ups still stands. 😉

Allow users to bookmark a current directory (saved in the browsers localstorage) to which they can easily change back via a dropdown. By default the users home directory is added as bookmark. Closes: cockpit-project#522

cockpituous · 2024-06-26T08:10:22Z

src/files-breadcrumbs.tsx

+    return (
+        <Dropdown
+          isOpen={isOpen}
+          onOpenChange={(isOpen: boolean) => setIsOpen(isOpen)}


This added line is not executed by any test.

allisonkarlitskaya

🚀 los geht's

allisonkarlitskaya · 2024-06-26T08:14:50Z

src/files-breadcrumbs.tsx

+                    return old_content.split('\n').filter(line => parse_uri(line) !== currentPath)
+                            .join('\n');
+                } else {
+                    const newBoomark = "file://" + path.map(part => encodeURIComponent(part))


much better, thanks!

jelly added the release-note label Jun 10, 2024

allisonkarlitskaya reviewed Jun 10, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch from d47e7e3 to 141f56c Compare June 10, 2024 09:52

jelly force-pushed the bookmark-feature branch 2 times, most recently from 0b4c263 to 3302a17 Compare June 18, 2024 13:48

jelly force-pushed the bookmark-feature branch 2 times, most recently from 32b78e2 to 4a77ff0 Compare June 18, 2024 16:06

jelly marked this pull request as ready for review June 18, 2024 16:21

jelly requested a review from allisonkarlitskaya June 18, 2024 16:21

jelly commented Jun 18, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch 2 times, most recently from e117ace to 16a53d6 Compare June 19, 2024 12:31

jelly commented Jun 19, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch 4 times, most recently from 3093844 to 53085a7 Compare June 21, 2024 08:06

allisonkarlitskaya reviewed Jun 24, 2024

View reviewed changes

src/files-breadcrumbs.tsx Outdated Show resolved Hide resolved

src/files-breadcrumbs.tsx Show resolved Hide resolved

src/files-breadcrumbs.tsx Outdated Show resolved Hide resolved

src/files-breadcrumbs.tsx Show resolved Hide resolved

allisonkarlitskaya requested changes Jun 24, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch 2 times, most recently from b87c7cb to 2f4c640 Compare June 25, 2024 09:47

jelly commented Jun 25, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch from 2f4c640 to f05c9dd Compare June 25, 2024 10:13

jelly requested a review from allisonkarlitskaya June 25, 2024 10:16

allisonkarlitskaya reviewed Jun 25, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch from 805761b to 54a3fc5 Compare June 25, 2024 14:22

jelly requested a review from allisonkarlitskaya June 25, 2024 14:40

allisonkarlitskaya reviewed Jun 25, 2024

View reviewed changes

jelly force-pushed the bookmark-feature branch from 54a3fc5 to 1443192 Compare June 25, 2024 16:04

jelly requested a review from allisonkarlitskaya June 25, 2024 16:11

jelly force-pushed the bookmark-feature branch 3 times, most recently from 9eac268 to 80e9d1a Compare June 26, 2024 07:42

garrett previously approved these changes Jun 26, 2024

View reviewed changes

jelly requested review from garrett and cockpituous June 26, 2024 07:46

jelly and others added 3 commits June 26, 2024 09:54

Implement bookmark support

1c96bed

Allow users to bookmark a current directory (saved in the browsers localstorage) to which they can easily change back via a dropdown. By default the users home directory is added as bookmark. Closes: cockpit-project#522

Stop using nth-of-type for tests

d0bdd56

topbar: Rework action buttons

0f20b9b

jelly dismissed garrett’s stale review via 0f20b9b June 26, 2024 07:54

jelly force-pushed the bookmark-feature branch from 80e9d1a to 0f20b9b Compare June 26, 2024 07:54

cockpituous reviewed Jun 26, 2024

View reviewed changes

allisonkarlitskaya approved these changes Jun 26, 2024

View reviewed changes

allisonkarlitskaya merged commit ff2f2f7 into cockpit-project:main Jun 26, 2024
16 checks passed

jelly deleted the bookmark-feature branch June 26, 2024 08:24

jelly removed the release-note label Jul 10, 2024

		} catch (err) {
		console.error("Unable to save bookmark", err);

		@@ -122,7 +125,7 @@ function BookmarkButton({ path }: { path: string[] }) {
		const newBoomark = path.map(part => encodeURIComponent(part))

Implement bookmark support #542

Implement bookmark support #542

Conversation

jelly commented Jun 10, 2024 • edited Loading

Bookmark support

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jun 10, 2024

jelly commented Jun 10, 2024

jelly commented Jun 10, 2024

jelly commented Jun 11, 2024

jelly commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented Jun 19, 2024 • edited Loading

garrett commented Jun 19, 2024

jelly commented Jun 19, 2024

garrett commented Jun 19, 2024

garrett commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya 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

allisonkarlitskaya 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

allisonkarlitskaya 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

garrett commented Jun 25, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Jun 25, 2024

garrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Jun 10, 2024 •

edited

Loading

jelly commented Jun 18, 2024 •

edited

Loading

garrett commented Jun 19, 2024 •

edited

Loading