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);
        },