storaged: Adjust stratis fs limits layout in creation dialog

[martinpitt]

Clean up the dialog changes from commit 236ffd3 as per [1]:

• Visually merge both options into "Stratis filesystem" and avoid extra top-level labels.
• Point out that the sizes are Stratis specific, as it doesn't say anywhere else in the dialog.
• Remove redundant "virtual filesystem" words in the options
• Avoid "Manage", pretty much all of Cockpit is "managing" something.

[1] #21039 (comment)

---

The default dialog now looks like this:

The checkbox separation is a bit awkward -- that's a result from them now being two separate components, instead of a single group. I tried for about half an hour to mess around with the CSS, but I can't even find the knob which does the flex component spacing. The more obvious ones are --pf-v5-l-stack--m-gutter--Gap and .pf-v5-l-stack.pf-m-gutter gap, but disabling or changing them to "200px" does not change anything visually. I also looked through all gap, height, size, padding etc. properties, and absolutely none of them influence the gap. But then again this is now by design..

With the limits enabled, it now looks like this:

I did not do anything about the default value (current default is "max"). I can't judge whether this is right, and presumably we also shouldn't -- it's the admin's job to lower them to a suitable size.

One thing which we should fix though is to not allow setting the limit size below the initial size?

[martinpitt]

@garrett FTR, I tested this with

diff --git test/verify/check-storage-stratis test/verify/check-storage-stratis
index 34ed72ce3..8b60b49a9 100755
--- test/verify/check-storage-stratis
+++ test/verify/check-storage-stratis
@@ -496,6 +496,8 @@ systemctl restart stratisd
                                values={"disks": {dev_1: True}})
         self.click_card_row("Storage", name="pool0")
 
+        testlib.sit()
+
         # Create filesystem with initial virtual size and a limit
 
         b.click(self.card_button("Stratis filesystems", "Create new filesystem"))

and

TEST_SHOW_BROWSER=1 test/verify/check-storage-stratis TestStorageStratis.testFilesystemSizes $RUNC

[martinpitt]

The failure is the pixel change. I don't push this yet, I'd like to wait for the review.

[mvollmer]

One thing which we should fix though is to not allow setting the limit size below the initial size?

We could do that by setting the maximum value of the "initial size" slider to the value of the "size limit" slider, dynamically.

[mvollmer]

Code changes look good to me.

[martinpitt]

@mvollmer yes, that was my idea as well, but I hit a wall:

diff --git pkg/storaged/dialog.jsx pkg/storaged/dialog.jsx
index c534e080d..6d2b0097a 100644
--- pkg/storaged/dialog.jsx
+++ pkg/storaged/dialog.jsx
@@ -240,6 +240,7 @@ import { Table, Tbody, Tr, Td } from '@patternfly/react-table';
 import { show_modal_dialog, apply_modal_dialog } from "cockpit-components-dialog.jsx";
 import { ListingTable } from "cockpit-components-table.jsx";
 import { FormHelper } from "cockpit-components-form-helper";
+import { is_function} from "cockpit/_internal/common";
 
 import {
     decode_filename, fmt_size, block_name, format_size_and_text, format_delay, for_each_async, get_byte_units,
@@ -1041,10 +1042,12 @@ class SizeSliderElement extends React.Component {
             this.setState({ unit: Number(u) });
         };
 
+        const max_value = is_function(max) ? max(values) : max;
+
         return (
             <Grid hasGutter className="size-slider">
                 <GridItem span={12} sm={8}>
-                    <Slider showBoundaries={false} min={min} max={max} step={(max - min) / 500}
+                    <Slider showBoundaries={false} min={min} max={max_value} step={(max - min) / 500}
                             value={slider_val} onChange={change_slider} />
                 </GridItem>
                 <GridItem span={6} sm={2}>
diff --git pkg/storaged/stratis/pool.jsx pkg/storaged/stratis/pool.jsx
index c678270fd..fa42e4f38 100644
--- pkg/storaged/stratis/pool.jsx
+++ pkg/storaged/stratis/pool.jsx
@@ -85,6 +85,8 @@ function create_fs(pool) {
         ];
     }
 
+    const max_size = pool.Overprovisioning ? stats.pool_total : stats.pool_free;
+
     dialog_open({
         Title: _("Create filesystem"),
         Fields: [
@@ -105,7 +107,7 @@ function create_fs(pool) {
                        {
                            visible: vals => vals.set_custom_size.enabled,
                            min: fsys_min_size,
-                           max: pool.Overprovisioning ? stats.pool_total : stats.pool_free,
+                           max: vals => vals.set_custom_limit.enabled ? vals.limit : max_size,
                            allow_infinite: pool.Overprovisioning,
                            round: 512
                        }),
@@ -122,7 +124,7 @@ function create_fs(pool) {
                        {
                            visible: vals => vals.set_custom_limit.enabled,
                            min: fsys_min_size,
-                           max: pool.Overprovisioning ? stats.pool_total : stats.pool_free,
+                           max: max_size,
                            allow_infinite: true,
                            round: 512
                        }),

The problem is that the render function of SizeSliderElement doesn't have access to the values of the other elements. This only works for the visible property, which is handled magically in the Row component. So it seems not entirely trivial to add support for this?

[mvollmer]

@mvollmer yes, that was my idea as well, but I hit a wall:

Yes, I think this is a job for update and set_options, see "RUNNING TASKS AND DYNAMIC UPDATES":

cockpit/pkg/storaged/dialog.jsx

Line 139 in 986d11a

RUNNING TASKS AND DYNAMIC UPDATES

I'll give it a go soonish...

[mvollmer]

@mvollmer yes, that was my idea as well, but I hit a wall:

Yes, I think this is a job for update and set_options, see "RUNNING TASKS AND DYNAMIC UPDATES":

cockpit/pkg/storaged/dialog.jsx

Line 139 in 986d11a

RUNNING TASKS AND DYNAMIC UPDATES

I'll give it a go soonish...

@martinpitt, something like this update function should do it:

        update: (dlg, vals, trigger) => {
            if (trigger == "limit") {
                if (vals.size > vals.limit)
                    dlg.set_values({ "size": vals.limit });
                dlg.set_options("size", { max: vals.limit });
            }
            update_at_boot_input(dlg, vals, trigger);
        },

[mvollmer]

Needs pixel test updates.

Let's have @garrett have the final say.

[mvollmer]

Needs pixel test updates.

Done.

[garrett]

I'm having issues building this... Jelle helped me debug it a bit, (turns out it's likely due to some weird git bugs I hit in the past week, including today, which probably corrupted my checkout), and now image prepare doesn't work on this branch still (for other reasons), but now works on main.

https://paste.centos.org/view/867b2abc

However, after merging main into this branch, it seems prepare fine now.

[mvollmer]

I'm having issues building this...

The issue seems to be this:

No match for argument: libssh-devel >= 0.8.5

We recently removed libssh from our dependencies and it is no longer in the CI images. But this PR here still requires it because it is based on a old version of main.

This PR needs to be rebased onto current main (or indeed merged into it).

[garrett]

After spending a few hours figuring out some odd build related issues, I finally got this working.

(I did a clean checkout, as git clean didn't work — I think hitting a weird git+GitHub bug the other day and also today somehow corrupted my repo. Then, after I got main building and saw this branch still wasn't building with the prepare step, I found that merging main to this branch got it building again.)

However, the modal dialog uses a custom abstraction layer on top of React JSX with PF Grid, which adds a lot of complexity and makes fixing the layout very difficult (unless we rewrite things).

This PR is closer to the mockups than what's in main, but there's a lot of odd spacing due to incorrect component usage and there are also misaligned progressive disclosures (the expandy parts).

PF Grid should be avoided everywhere in Cockpit. It's a 12-column relic from float layouts for web documents from decades ago, not a component that's ever needed in any UI layout. We should be using components like Input Group, Split, or Flex. Or even standard CSS, including flex or grid (that is: CSS grid, not PF Grid).

I can try to work around all of this with some CSS hackery, but we really should fix which components we use.

Rewriting the components that are used, especially for the slider, will likely have side effects that affect the other storage modals... so I guess we'll need to do that outside of this PR.

Even with the progressive disclosures hidden away, I do see extra spacing that I guess is probably related to all of the uses of form group? And the limit size group is in a different row than the initial size one, instead of both being part of the "Stratis filesystem" group. (It's really hard to debug in Chrome instead of Firefox, so I'm not entirely sure yet what's going on, but I think this is what's happening.)

So, in other words, I think we should:

1. See if we can fix the grouping issues I pointed out in the previous paragraph (which might require @mvollmer or @martinpitt to do this)
2. Add some CSS to fix it (which I can do once step 1 is done)
3. Then merge this in when we fix the above
4. Do a follow-up to properly fix the components (as that'll impact other modals)

[garrett]

Rebased on main to fix the problems to build the tests.

[mvollmer]

@martinpitt, @garrett, here are two commits that fix the grouping: https://github.com/mvollmer/cockpit/tree/storage-dialog-grouping-experiment

Dialog looks like this with this:

(We could replace all uses of CheckBoxes with Group now, but I guess we should rather retire the whole abstraction and replace ir with the Dialog Implementation Convenience Kit.)

[garrett]

👍 Looks great! Thanks to you both @martinpitt and @mvollmer!

This PR now solves the issues I was mainly concerned about and can be merged.

Everything else I pointed out would be nice-to-have followups, like dropping the use of PF Grid. (It wouldn't be a blocker.) I just checked and see we do use PF Grid in 7 additional places in Cockpit (main; I didn't check sub-projects), so it'd be a general cleanup, not even anything specific to the Storage page either.