Webui pre blivet dialogs #5113

rvykydal · 2023-08-31T12:10:09Z

Addresses https://issues.redhat.com/browse/INSTALLER-3638

TODO

tests
decide what to do in case blivet-gui window is lost / hidden (see the variants below)

The first commit is independent, I'll post it separately.

I started with version of dialog flow designed in the jira issue (1.) but then I tried some other solutions mainly to handle the situation of blivet-gui window ~~disappearing~~ hidden behind installer window. I am posting screencasts of 2 other solutions (2., 3.). ~~As another alternative it may be also possible to try to force the blivet-gui window always on top but I guess this can be convoluted.~~ UPDATE: looking at it again the blivet-gui window hidden behind installer window is perhaps not such a big issue (I was perhaps mislead by the situation in Gtk GUI and nm-connection-editor where it is more difficult to get the window back.)

following the design (https://issues.redhat.com/browse/INSTALLER-3638?focusedId=22823818&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-22823818).
The Rescan dialog appears shortly after blivet-gui is launched (we need to use some delay, we don't have an event)
Normal flow:
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-mockup.webm
Lost window flow:
Crash on second run of blivet-gui.
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-mockup-lost-window.webm
Move to the Rescan dialog when blivet-gui exits
(commit webui: move to rescan dialog only after quitting blivet-gui)
Normal flow:
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-rescan-on-exit.webm
Lost window flow:
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-rescan-on-exit-lost-window.webm
It is not possible to continue until user finds ~~(via Alt-Tab)~~ and closes the window, hence the variant 3)
Don't disable Cancel so it can be used escape (and kill blivet-gui) lost blivet-gui window. Same as 2) Move to Rescan dialog when blivet-gui exits (now either by finishing or on Cancel/kill).
(commit webui: allow to kill blivet-gui with Cancel button)
Normal flow:
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-kill-on-cancel.webm
Lost window flow:
Lost blivet-gui can be killed by Cancel.
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-kill-on-cancel-lost-window.webm
A blivet-gui crash should be still handled as Critical Error:
https://rvykydal.fedorapeople.org/webui/pre-blivet/pre-blivet-kill-on-cancel-crash.webm

rvykydal · 2023-09-01T09:14:18Z

Added the tests.

garrett · 2023-09-04T08:56:11Z

Thanks for adding this and exploring the various options.

Delay (option # 1) is the best approach.

The other options are a bit confusing if you switch windows between the two. (Most people wouldn't switch back, nor should they really.) It's not obvious why it would still be launching (with a spinner) when the application is already launched, or why clicking cancel (which, by definition, should back out and do nothing) would stop the program. Additionally, if Blivet is doing some long-running action and someone switched back and clicked cancel, it would stop the operation and leave their system in an inconsistent and unusable state.

While the delay is not really tied to launching, if the delay is long enough, switching the dialog won't be detected, as Blivet will be above it. The trick is to make delay long enough but not too long. It seems fine there.

Is there a way to know if the window closed and to make sure it's at the second dialog if Blivet has exited? Then we could have the short delay and be sure that someone didn't just open and immediately quit Blivet and still have the launching screen. Ideally, if Blivet didn't make any changes, then we could even skip the rescan dialog and go directly back to Anaconda without any dialog at all.

rvykydal · 2023-09-04T14:01:13Z

Thanks for adding this and exploring the various options.

Delay (option # 1) is the best approach.

The other options are a bit confusing if you switch windows between the two. (Most people wouldn't switch back, nor should they really.)

Yes, that is my point, in current state switching back is in majority of cases an accident ~~with no obvious way how to get the window back (keyboard)~~, which I was trying to address - perhaps too early - with 2. or 3. this PR.

It's not obvious why it would still be launching (with a spinner) when the application is already launched, or why clicking cancel (which, by definition, should back out and do nothing) would stop the program. Additionally, if Blivet is doing some long-running action and someone switched back and clicked cancel, it would stop the operation and leave their system in an inconsistent and unusable state.

Agreed, perhaps it seemed more obvious to me because I implemented it:). Another approach could be to kill blivet-gui on the next "Modify storage" if it is running in a lost/hidden window (that is what we are using in Gtk GUI and nm-connection-editor app used there, but in this case the window is lost for good if moved to the background). But perhaps the most safe would be just scenario 1. - either user needs try hard to bring back the blivet-gui window (and we can maybe try to make it easier somehow), or just crash hard if it runs blivet-gui second time, but do not kill it (secretly) so we don't leave the system in some inconsistent / unstable state for following installation.

M4rtinK

Looks good to me, thanks! :)

Just some small wording suggestions.

M4rtinK · 2023-09-04T17:16:23Z

ui/webui/src/components/storage/InstallationMethod.jsx

+ }>
+ <TextContent>
+ <Text component={TextVariants.p}>
+ {_("Blivet is and advanced storage editor that lets you resize, delete, and create partitions. It can set up LVM and much more.")}


I guess this should be "Blivet GUI" instead of just "Blivet" ? I'm thinking that saying "Blivet GUI" also makes it possible users might find relevant documentation/HOWTOs instead of possibly finding the Blivet library refferrence.

Updated in a separate commit.

M4rtinK · 2023-09-04T17:18:50Z

ui/webui/src/components/storage/InstallationMethod.jsx

+ blivetProcessRef.current = null;
+ cockpit.spawn(["killall", "blivet-gui"], { err: "message" })
+ .then(() => {
+ console.log("Blivet stopped.");


Maybe "Blivet GUI" there as well ? Could be otherwise easily confused with something going wrong the Blivet library itself.

Updated in a separate commit.

rvykydal · 2023-09-05T07:30:24Z

I am going to update the PR for the version 1.

rvykydal · 2023-09-05T08:08:41Z

Is there a way to know if the window closed and to make sure it's at the second dialog if Blivet has exited? Then we could have the short delay and be sure that someone didn't just open and immediately quit Blivet and still have the launching screen.

Yes, this should be easy, I'll add it.

https://issues.redhat.com/browse/INSTALLER-3638

Blivet is a library, Blivet-gui is the name of the application run.

rvykydal · 2023-09-05T11:15:50Z

I am going to update the PR for the version 1.

Done

Is there a way to know if the window closed and to make sure it's at the second dialog if Blivet has exited? Then we could have the short delay and be sure that someone didn't just open and immediately quit Blivet and still have the launching screen.

Yes, this should be easy, I'll add it.

Added.

Also I've replaced Blivet with Blivet-gui name (webui: use Blivet-gui name instead of Blivet).

rvykydal · 2023-09-05T11:20:25Z

/kickstart-test --waive webui-only

AdamWill · 2023-09-06T15:36:49Z

ui/webui/src/components/storage/InstallationMethod.jsx

+ }>
+ <TextContent>
+ <Text component={TextVariants.p}>
+ {_("Blivet-gui is and advanced storage editor that lets you resize, delete, and create partitions. It can set up LVM and much more.")}


typo: "an advanced storage editor", not "and advanced storage editor"

AdamWill · 2023-09-06T15:37:45Z

Could we extend this to also show information on the required partitions for the current configuration? This would help with https://bugzilla.redhat.com/show_bug.cgi?id=2237527 ...

garrett · 2023-09-11T08:03:52Z

Do we not have the required partitions popover implemented yet? 🤔 (I was away on PTO and the end of last month and had a face to face meeting with the Cockpit team last week, so I hadn't had the chance to double check the current state of the installer.)

If it is missing (and it sounds like it), then we could integrate it into the "Modify storage" modal as @AdamWill suggests, like this:

Reminder: I made up these partitions and sizes. They might happen be accurate, but probably aren't. And there might be other partitions (such as /boot/efi and so on).

The only down side of this, is that the reference will go away when Blivet is launched, unless we change how the modal changes from launch to rescan. (It would have to drop the spinner state after the delay instead of switching. And it would have to switch to the rescan modal when Blivet-gui exits.) But it would be clearer what's needed.

github-actions bot added webui f40 labels Aug 31, 2023

rvykydal added master Please, use the `f39` label instead. and removed f40 labels Aug 31, 2023

rvykydal force-pushed the webui-pre-blivet-dialogs branch from 3db5a23 to d794140 Compare August 31, 2023 12:11

rvykydal requested a review from garrett August 31, 2023 12:14

rvykydal marked this pull request as draft August 31, 2023 12:15

rvykydal requested a review from M4rtinK August 31, 2023 13:02

rvykydal force-pushed the webui-pre-blivet-dialogs branch from d794140 to 760b0c3 Compare September 1, 2023 09:13

rvykydal force-pushed the webui-pre-blivet-dialogs branch from 760b0c3 to bd6baea Compare September 1, 2023 09:15

rvykydal marked this pull request as ready for review September 1, 2023 09:30

rvykydal force-pushed the webui-pre-blivet-dialogs branch from bd6baea to 3c9ced2 Compare September 1, 2023 09:37

rvykydal requested a review from KKoukiou September 4, 2023 08:20

rvykydal force-pushed the webui-pre-blivet-dialogs branch from 3c9ced2 to d89c94c Compare September 4, 2023 08:27

rvykydal requested a review from vojtechtrefny September 4, 2023 12:04

M4rtinK approved these changes Sep 4, 2023

View reviewed changes

rvykydal force-pushed the webui-pre-blivet-dialogs branch from d89c94c to 7f1274d Compare September 5, 2023 10:06

rvykydal added 8 commits September 5, 2023 13:13

webui: show error name in Critical Error dialog only if it exists

46cd8fa

webui: add dialogs around launching blivet-gui

80fe29c

https://issues.redhat.com/browse/INSTALLER-3638

webui: catch errors when spawning blivet-gui

ac17d07

webui: make blivet-gui start failure a Critical Error

2536b3d

webui: do not show rescan dialog in case of critical error

b1c037d

webui: if blivet-gui exits earlier then the delay update the dialog

fd6f313

webui: add tests for modify storage dialogs

f0ce7df

webui: use Blivet-gui name instead of Blivet

76a0c82

Blivet is a library, Blivet-gui is the name of the application run.

webui: add pixel test for modify storage test

4476af5

rvykydal force-pushed the webui-pre-blivet-dialogs branch from 7f1274d to 4476af5 Compare September 5, 2023 11:14

rvykydal added the f40 label Sep 6, 2023

rvykydal merged commit caea6ad into rhinstaller:master Sep 6, 2023
17 checks passed

rvykydal mentioned this pull request Sep 6, 2023

F39 webui pre blivet dialogs #5132

Merged

AdamWill reviewed Sep 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webui pre blivet dialogs #5113

Webui pre blivet dialogs #5113

rvykydal commented Aug 31, 2023 •

edited

Loading

rvykydal commented Sep 1, 2023

garrett commented Sep 4, 2023

rvykydal commented Sep 4, 2023 •

edited

Loading

M4rtinK left a comment •

edited

Loading

M4rtinK Sep 4, 2023

rvykydal Sep 6, 2023

M4rtinK Sep 4, 2023

rvykydal Sep 6, 2023

rvykydal commented Sep 5, 2023

rvykydal commented Sep 5, 2023 •

edited

Loading

rvykydal commented Sep 5, 2023 •

edited

Loading

rvykydal commented Sep 5, 2023

AdamWill Sep 6, 2023

AdamWill commented Sep 6, 2023

garrett commented Sep 11, 2023

Webui pre blivet dialogs #5113

Webui pre blivet dialogs #5113

Conversation

rvykydal commented Aug 31, 2023 • edited Loading

rvykydal commented Sep 1, 2023

garrett commented Sep 4, 2023

rvykydal commented Sep 4, 2023 • edited Loading

M4rtinK left a comment • edited Loading

Choose a reason for hiding this comment

M4rtinK Sep 4, 2023

Choose a reason for hiding this comment

rvykydal Sep 6, 2023

Choose a reason for hiding this comment

M4rtinK Sep 4, 2023

Choose a reason for hiding this comment

rvykydal Sep 6, 2023

Choose a reason for hiding this comment

rvykydal commented Sep 5, 2023

rvykydal commented Sep 5, 2023 • edited Loading

rvykydal commented Sep 5, 2023 • edited Loading

rvykydal commented Sep 5, 2023

AdamWill Sep 6, 2023

Choose a reason for hiding this comment

AdamWill commented Sep 6, 2023

garrett commented Sep 11, 2023

rvykydal commented Aug 31, 2023 •

edited

Loading

rvykydal commented Sep 4, 2023 •

edited

Loading

M4rtinK left a comment •

edited

Loading

rvykydal commented Sep 5, 2023 •

edited

Loading

rvykydal commented Sep 5, 2023 •

edited

Loading