packagekit: Add "Reboot after completion" switch to update progress page

[martinpitt]

This allows users to start a manual update and queue a reboot without
having to babysit the progress.

Rework the progress page layout a bit:

• Use a Grid layout, so that the update log has the same alignment than
  the progress bar.

• Stop centering the "Cancel" button, which was a bit dubious wrt. PF
  recommendations anyway. Show it to the right of the progress bar
  instead.

• Hide the cancel button instead of disabling it, once cancellation is
  not allowed any more.

• Rename "Update log" to "View update log" to clarify what the action
  means.

• Increase the spinner size, to make it easier to see.

https://bugzilla.redhat.com/show_bug.cgi?id=2056786

---

• Builds on top of PR packagekit: Show current package when reattaching to running update #17517

Software Updates: Optionally reboot after updating

The update progress page offers you to reboot the machine after applying updates succeeded. If the machine can be rebooted without careful planning, this avoids having to wait and do another interactive step.

[martinpitt]

I fixed this one locally, just didn't push yet -- let's wait for the CSS fix.

[garrett]

Mockup:

When you can no longer cancel, the button could just disappear (visibility: hidden, not display: none)

[garrett]

The button could still be set to disabled and we could have something like

.the-update-ui-thing-here .pf-c-button[disabled] {
   visibility: hidden;
}

(The names of the classes would probably be different, especially that first one. 😉)

Or better yet, add a special class to the button (like software-update-cancel in this example, but pick whatever would fit best with the existing CSS names), then still disable the button the normal PF way:

.software-update-cancel[disabled] {
   visibility: hidden;
}

[martinpitt]

Meh, this is really painful. I'm trying to use a Flex to put the progress bar and cancel button side by side, and the closest I got was this mess:

(yes, other ways of nesting them looked even more broken). ProgressBar isn't really designed to contain actions associated to it. If we want to do that, I think we'll have to dissect the React component and reimplement it ourselves.

[garrett]

Either use a flex for just the progress bar and the cancel button, with align-items: baseline (there's a PF way of doing this too) and keep upgrade log out of it completely.

Or use grid for all the elements, but then the update log would have to span across the bottom cell.

I'd suggest using flex; it's simpler in this case.

[garrett]

Oh, I'm looking at the code and the status around the progress bar is implemented in HTML instead of a PF component?

                <div className="progress-description">
                    <Spinner isSVG size="sm" />
                    <strong>{ PK_STATUS_STRINGS[lastAction.status] || PK_STATUS_STRINGS[PK.Enum.STATUS_UPDATE] }</strong>
                    &nbsp;{lastAction.package}
                </div>
                <Progress title={formatTimeRemaining} value={percentage} />

This should be inside an element, separate from the others. It could/should probably be part of the PF component. (Although perhaps there's a reason it isn't? Is it because it needs to be updated and PF might allow for that?)

[garrett]

I tried this, but haven't been able to test it yet:

        <>
            <Flex className="progress-main-view">
                <FlexItem>
                    <div className="progress-description">
                        <Spinner isSVG size="sm" />
                        <strong>{PK_STATUS_STRINGS[lastAction.status] || PK_STATUS_STRINGS[PK.Enum.STATUS_UPDATE]}</strong>
                        &nbsp;{lastAction.package}
                    </div>
                    <Progress title={formatTimeRemaining} value={percentage} />
                </FlexItem>
                <FlexItem>{cancelButton}</FlexItem>
            </Flex>

            <Switch id="reboot-after" isChecked={rebootAfter}
                    label={ _("Reboot after completion") }
                    onChange={setRebootAfter} />

            <div className="update-log">
...

Update from pitti: Looks like this, almost the same (just the Update log is better):

[garrett]

Something like this:

  <Flex flexWrap={{ default: 'nowrap' }} alignItems={{ default: 'alignItemsFlexEnd'}}>
    <FlexItem grow={{ default: 'grow' }}>Progress</FlexItem>
    <FlexItem>Cancel</FlexItem>
  </Flex>

Update from pitti: That is already a lot more promising! The "Update log" begins to move to the right on wide screen sizes, so I captured a wide one. It's on the left margin on narrower widths.

Unfortunately I seem unable to capture a proper HTML/CSS dump of this. Chromium's "Save as" is broken (clicking it doesn't do anything), Firefox' "save complete webpage" reloads the page from the server and thus just gets a "cockpit is not installed" error message. So I copied the outer page HTML from the whole frame, updates.html.gz and updates.css.gz (gzipped so that I can attach them here).

I can load updates.html into a new browser tab, and aside from the wrong fonts this looks quite faithful.

[martinpitt]

@garrett : I updated your two comments from above. Is that helpful in any way?

[garrett]

The first suggestion is exactly the same as the second suggestion, BTW. The second one is abstracted and has some alignment and size attributes. The first drops the wrapper though, whereas the second one needs to be within it, I guess and is why it's going too wide in your screenshot?

In other words: Go with the second; make sure it's within the wrapper (as it looks like it isn't). I didn't consider the CSS and layout overall when suggesting the things, as I can't even look at it (as all updates are just too fast and I cannot pause them).

[martinpitt]

@garrett: The main difference is the grow={{ default: 'grow' }} bit, which causes the progress bar to be reasonably wide again, instead of being squeezed so narrow.

[martinpitt]

This is an absolute nuisance to develop on right now, due to the broken page reload. That gets stuck forever on the "Initializing.." empty state, instead of properly re-attaching to a running upgrade. That's also quite a major regression, so I'm going to look at that one first.

[martinpitt]

I sent PR #17517 for the issue above, and also included the general code cleanups, so that this one will become less noisy. I'd like to block on that, so that developing this change gets easy. Right now you'll have to restart the VM and re-upload the changed code for each iteration.

[martinpitt]

@garrett : I tweaked the CSS for the cancel button and reboot switch to be center-aligned, and updated the screenshot.

[martinpitt]

@garrett : This can be tested with sitting here:

--- test/verify/check-packagekit
+++ test/verify/check-packagekit
@@ -975,6 +978,7 @@ ExecStart=/usr/local/bin/{packageName}
         b.wait_visible("#app div.pf-c-progress__bar")
         # auto-reboot is off by default
         b.wait_visible("#reboot-after:not(:checked)")
+        sit()
         b.click("#reboot-after")
         b.wait_visible("#reboot-after:checked")

and running

test/verify/check-packagekit -stv TestUpdates.testRebootAfterSuccess

Due to the recent fix, the page can now be reloaded, or you can even log in from another browser. This makes tools/webpack-watch -r c packagekit work against the running VM from the test, and thus it's a lot simpler and faster to iterate.

[garrett]

So the cancel button is underneath when I look at this PR?

Also, the update log expander is outside of the constraint and should be inside (aligned to the same place as the start of the progress bar).

[martinpitt]

@garrett : Right; I've fought with the cancel button for two hours to try and get it to the right of the progress bar in a sensible manner. I got all kinds of "ridiculously broken" and then gave up and left it where it is.

Agreed about the update log -- that's the status quo (I didn't touch that), I am happy to look at that in a separate PR. It's not super-easy to do (that whole page is a big soup of flex and custom layout..).

[garrett]

Agreed about the update log -- that's the status quo (I didn't touch that), I am happy to look at that in a separate PR. It's not super-easy to do (that whole page is a big soup of flex and custom layout..).

This makes it sound like it's using nested flexes and should really be using a grid overall. (We'd need a grid object toplevel and then all major components at the immediate next-level.) With a lot of items, especially more than just in a line, grid is usually less fiddly to get what's wanted.

The order is really weird, wrong, and confusing with the cancel and the toggle button like this. 😢 Otherwise I'd say add it in an do some follow-ups. But it's just so wrong as-is that I think we shouldn't split it apart.

[garrett]

If you can get all the components into one level, I can construct a grid that works.

The grid would basically be:

progress cancel
action
log

(Log itself could be the expander + log, or we could have expander and log as separates parts of the grid. It doesn't matter.)

[martinpitt]

With a grid, it looks like this now while cancelling is allowed:

and once it's not allowed, the button disappears:

With isSmall button:

[martinpitt]

@garrett : I updated the commit message and "official" (relnote) screenshot in the description, and the three screenshots above to the current grid-based design. WDYT?

[martinpitt]

oh, right -- this is a race condition now. We would need to somehow postpone the downloading, or drop that check.

[garrett]

@garrett : I updated the commit message and "official" (relnote) screenshot in the description, and the three screenshots above to the current grid-based design. WDYT?

Oh, did you try what I suggested on IRC?

Use smaller progress bar, add sm margin to the bottom. This would not just thin the bar, but replace some of its thickness with space. This means that the cancel button would probably line up a little better.

It'd look like this:

It's something like this (making sure you have the sizing utility classes loaded):

 <Progress size={ProgressSize.sm} className="pf-u-mb-sm" />

https://www.patternfly.org/v4/utilities/spacing

[martinpitt]

@garrett : Thanks for the hint! That's how it looks with a small (thin) progress bar and the sm margin:

This is how it would look with standard progress size and just the sm border on the bottom:

and with xs margin + small progress:

[garrett]

Thanks!

I think this needs xs for the margin and use small for the progress bar and button.

[martinpitt]

@garrett : Re-pushed with the implementation for the last screenshot (xs border, small progress and button). Thank you for your help!

[marusak]

Not doing design review, I see you went through a few iteration already :) Thanks!

[martinpitt]

This is an unrelated flake, but amplified due to the "3x affected retry". It's an incomplete naughty pattern match for a known crash. I'll look at it, but preferably outside of this PR, so retrying.