Fall back to sorting by name on ties #535

allisonkarlitskaya · 2024-06-07T10:28:12Z

If we tie on the mtime or the file size then fall back to sorting based on name — it's always unique.

This should give us a fully-defined sort order for all directories.

jelly

Nice!

allisonkarlitskaya · 2024-06-17T10:16:01Z

I think the debian fail is relevant, so probably don't merge this yet...

jelly · 2024-06-17T10:17:32Z

I think the debian fail is relevant, so probably don't merge this yet...

Seems so! I'll download and take a look

jelly · 2024-06-18T09:02:12Z

So on debian testing the ordering on size is correct except for the first directory:

eee
Eee
ddd

Should be

ddd
eee
Eee

ddd is a symlink to /tmp so should be treated as a directory:

{
    "type": "lnk",
    "group": "root",
    "size": 4,
    "target": "/tmp",
    "mode": 511,
    "mtime": 1718701039.496,
    "user": "root",
    "name": "ddd",
    "to": "dir",
    "category": null
}

In fedora VM

{
    "size": 4,
    "mtime": 1718701354.1252325,
    "user": "root",
    "target": "/tmp",
    "mode": 511,
    "group": "root",
    "type": "lnk",
    "name": "ddd",
    "to": "dir",
    "category": null
}

So we first so a dir_sort and then a sort on size. As this is a symlink its size is 4 and the other directory's size is bigger, interestingly this does not happen on Fedora, I assume that's because of btrfs.

Btrfs

lrwxrwxrwx. 1 root root   4 Jun 18 09:02 ddd -> /tmp
drwxr-xr-x. 1 root root   0 Jun 18 09:02 eee
drwxr-xr-x. 1 root root   0 Jun 18 09:02 Eee

Debian

-rw-r--r-- 1 root root  10M Jun 18 08:57 BBB
drwxr-xr-x 2 root root 4.0K Jun 18 08:57 Eee
-rw-r--r-- 1 root root    4 Jun 18 08:57 aaa
-rw-r--r-- 1 root root    0 Jun 18 06:57 ccc
lrwxrwxrwx 1 root root    4 Jun 18 08:57 ddd -> /tmp
drwxr-xr-x 2 root root 4.0K Jun 18 08:57 eee

jelly

Needs a fix for sorting on directories as they are compared by size when they are both a directory.

jelly

Ironically this fails on typescript checks :-)

jelly · 2024-06-19T15:03:24Z

src/files-card-body.tsx

@@ -423,7 +444,7 @@ const Row = React.memo(function Item({ file, isSelected }) {
              dataLabel="date"
              modifier="nowrap"
            >
-                {timeformat.dateTime(file.mtime * 1000)}
+                {file.mtime ? timeformat.dateTime(file.mtime * 1000) : null}


null would show nothing, that's probably fine. Wondering if we might want to show no date.

jelly · 2024-06-19T15:04:47Z

test/check-application

@@ -354,6 +354,11 @@ class TestFiles(testlib.MachineCase):

        self.enter_files()

+        # set a bogus sort value in localStorage to make sure we handle it gracefully
+        b.eval_js("""window.localStorage.setItem("files:sort", 'bzzt')""")


src/files-card-body.tsx

allisonkarlitskaya · 2024-06-19T20:05:22Z

The pixel test fails are kinda interesting. Sometimes the / separators in the path bar get rendered a little bit thicker or thinner. I wonder what's causing that...

https://cockpit-logs.us-east-1.linodeobjects.com/pull-535-2f0f2494-20240619-195556-fedora-40/log.html

allisonkarlitskaya · 2024-06-19T21:45:39Z

The pixel failures on fedora-40 are also broken on main and fixed by #576.

jelly · 2024-06-20T08:15:41Z

src/fileActions.jsx

@@ -474,7 +474,7 @@ export const fileActions = (path, selected, setSelected, clipboard, setClipboard
                    );
                },
            },
-            { type: "divider" },
+            null,


This feels super non-obvious. null doesn't give me a hint that we are rendering a divider.

Okay. I brought back { type: "divider" } with a bit of discriminated union magic (and a bonus of some extra code in typescript for my efforts).

src/files-card-body.tsx

This was trivial.

The following commit will port files-card-body to TypeScript, revealing several safety issues with the menu items handling code. Fix them up before we do that: - it's difficult to reason about the types of menu items when we have two similar but different things: items and dividers. Instead of using { type: "divider" } as a placeholder for a divider, we could use null, but this would have a negative impact on readabilty. Fortunately, with explicit typing we can form what is essentially discriminated union. Move the menu code to its own TypeScript file, continuing the shrinking of fileActions.jsx, and add a type for menu items. Now TypeScript can reason about the properties that are present on all non-divider items (`title` and `onClick`). This prevents TypeScript from complaining that `onClick` might be `undefined`, for example. The explicit typing also means that if we make a mistake when defining a menu item, we'll see the type error directly at the point of the mistake. - className is only defined on some items, but we do an unconditional concatenation of `"context-menu-option " + item.className`. That means that we set the class to `context-menu-option undefined`, which is definitely not what we wanted - strictly speaking we're not allowed to pass isDisabled={undefined}, but we're doing that for most items. Provide a default of false. - not really a type issue, but strange: we're using a translated string as a key= on our <MenuItem/> elements when we have a unique ID. Use the id instead, which is what the sidebar does.

Port files-card-body.{jsx -> tsx} in preparation for work on the sorting function. Create an enum type for our various sort options and use it in preference to hard-coded string values: that let's us drop the `default:` on our `switch()` over the possible sort types: TypeScript is now doing exhaustiveness checking for us. This found some bugs: - an invalid sort order value in localestore of the browser could cause an oops by returning an undefined sort function (due to the explicit return in default case of our `switch` in `compare()`. We now use an as_sort() function when reading the value to make sure it's one of our supported sort orders. - fsinfo says that the `.size` and `.mtime` fields might be missing, so we need to handle those cases, both in sorting and on display. `.name` is guaranteed to be present, since we set it ourselves - there were some very fine edge cases in our handling of selection: it's not clear if these could lead to errors in practice, but let's clean them up

If we tie on the mtime or the file size then fall back to sorting based on name — it's always unique. This should give us a fully-defined sort order for all directories. We can now remove a TODO: from a testcase.

cockpituous · 2024-06-20T11:16:20Z

src/files-card-body.tsx

+
+    // treat non-regular files and infos with missing 'size' field as having size of zero
+    const size = (a: FolderFileInfo) => (a.type === "reg" && a.size) || 0;
+    const mtime = (a: FolderFileInfo) => a.mtime || 0; // fallbak for missing .mtime field
