Change the root password spoke GUI design #3511

VladimirSlavik · 2021-07-14T14:34:39Z

This resolves ambiguity of what options are available when root is locked.

~~This is a draft because there are at least these TODOs:~~

make bold labels "clickable"
verify the logic
find bugs etc. to link
consider renaming spoke to "Root account security"
~~add an "empty password" banner~~ automatically switches to no password + locked instead.

See also: #3262 (comment)
Suggested-by: Mairin Duffy mairin@redhat.com @mairin

VladimirSlavik · 2021-07-14T14:35:17Z

@mairin, please comment on the preferred form of attribution, if you want it different.

VladimirSlavik · 2021-07-14T14:39:05Z

Here's how it looks.
(That shall not change substantially, anymore.)

new-root-spoke.mp4

mairin · 2021-07-14T15:08:03Z

Wow that looks wonderful, @VladimirSlavik !!! The padding / margins / spacing etc. are right on and the animation looks slick. 🎉

poncovka

It looks great!

pyanaconda/ui/gui/spokes/root_password.py

poncovka · 2021-07-15T12:02:28Z

pyanaconda/ui/gui/spokes/root_password.py

@@ -179,7 +183,7 @@ def status(self):
        if self._users_module.IsRootAccountLocked:
            # reconfig mode currently allows re-enabling a locked root account if
            # user sets a new root password
-            if is_reconfiguration_mode() and not self._lock.get_active():
+            if is_reconfiguration_mode() and not self._enable_radio.get_active():


This needs to be tested with the initial setup.

poncovka · 2021-07-15T12:07:35Z

Please, test it with rootpw --lock password. I don't think we need to support this combination in UI, but the kickstart installation should still work. You can also run the rootpw-lock kickstart test.

jstodola · 2021-07-16T13:27:46Z

It looks like a great improvement in usability of the spoke!

I would just like to express my dislike for the animation. Although it looks OK when the installation is running locally, animations in the installer in general do not look good when running a remote installation via VNC, where you can see for example just one or two frames from the animation, depending on the network speed. In some case, animations make the installation process even painful, see https://bugzilla.redhat.com/show_bug.cgi?id=1946064. No animation is good animation.

jstodola · 2021-07-16T20:41:59Z

@AdamWill please be aware of this UI change, since it might affect your Fedora installation testing.

poncovka · 2021-07-19T10:57:12Z

Also, add a release note for this change, please (https://anaconda-installer.readthedocs.io/en/latest/contributing.html#release-notes).

VladimirSlavik · 2021-07-19T13:11:34Z

@mairin I'd like to run by you a couple of things?

Previously, I had radio buttons in a separate table column. That worked well in terms of looks: All things moved the same distance needed for the controls. However, the "headings" were not logically part of the radio buttons, in fact they were not associated with them in any way except for being visually on the same "horizontal line". That would be disastrous in terms of accessibility. After thinking it through, I decided there'$s no going around it, and changed things such that the headings are (logically) part of the radio buttons

That, however, means the rest of stuff will no longer automatically go the right distance. Luckily enough, the right distance was easy to guess. However, should the font or the (default/stock gtk) control looks change, the vertical alignment will fall apart: The bold labels might move left or right by a different amount than the long texts and hideable controls.

Is that something that needs any considerations? I guess I can make the alignment intentionally different (see pic below), but I have no opinion on this.

Second, after @jstodola's request, I changed the animation to a fade-in/out. That's the same as on the storage screen, so it should be less offensive in terms of what stays "frozen" onto the screen for VMs used over slow network. Should I do something different, or does it matter at all?

VladimirSlavik · 2021-07-22T10:54:44Z

UI logic now works in absence of kickstart, onwards to that...

edit: Unchangeable kickstart ok. Next, changeable kickstart.

edit: Changeable kickstart works same as before, although I'd say what it does is not sane: Entering the spoke means you have to set a pawssword, or disable. The password from kickstart is irreversibly gone.

VladimirSlavik · 2021-07-22T13:45:00Z

/kickstart-test --testtype smoke

VladimirSlavik · 2021-07-22T14:43:21Z

/kickstart-test --testtype users

mairin · 2021-07-22T15:03:17Z

@VladimirSlavik Hey! Taking a look now -

Re: attribution - looks fine to me :)

Re: alignments... if the change you made to make the alignment intentionally different (e.g. the added indent on the description for each radio button option) prevents the shifting of elements you were worried about, the alignment screenshot you posted looks fine to me! I did examine the GNOME HIG and it does not account for the case where radio buttons have a header message + a non-header description, so this doesn't explicitly violate any design patterns outlined there, and I think it looks fine & clear.

Re: the animation - is there any way you could give me a preview so I could see what it looks like? Probably fade in / fade out will look fine, I just want to see how it comes off!

VladimirSlavik · 2021-07-22T17:32:44Z

Thanks - and here you go!

new-root-spoke2.mp4

mairin · 2021-07-22T17:34:34Z

@VladimirSlavik looks great to me, I see no issue!

jstodola · 2021-07-23T08:29:36Z

The text boxes for entering the password are too wide - if they are as wide as possible, the screen would not look good on fullhd/4k monitors. Could the size of the password text boxes be reduced?
And one note about enabling/disabling remote access mentioned in the spoke - would it make sense to specify that it only affect remote access for the root account? Remote access for non-root users is not affected by this spoke.

jstodola · 2021-07-23T11:42:53Z

It looks great to me, thanks!

jkonecny12

Hi, I really like the new solution, however, I would like to make the variable names a bit more clear.

jkonecny12 · 2021-07-26T16:36:07Z

pyanaconda/ui/gui/spokes/root_password.py

-        self.password_entry.set_sensitive(not lock.get_active())
-        self.password_confirmation_entry.set_sensitive(not lock.get_active())
-        if not lock.get_active():
+    def on_disable_enable_clicked(self, control):


This method name seems pretty confusing to me. Right now I'm confused if it should toggle radio buttons or disable some and enable some. Could you please change the name?

Sorry, I don't agree. It's an event handler - the name conforms to the on_controlname_eventtype pattern most of our code uses. You do not call it, Gtk does. And the next line is a docstring, which also says what it is: Click handler for enable and disable radio buttons.

In that case is disable_enable a good control name?

The handler is used by two controls, one enables, one disables.

Maybe not mentioning the control names would make the name work? Eg. on_root_enabled_changed

That seems much better to understand to me.

Great, I will do that, then.

jkonecny12 · 2021-07-26T16:39:50Z

pyanaconda/ui/gui/spokes/root_password.py

+        self._enable_radio = self.builder.get_object("enable_radio")
+        self._disable_radio = self.builder.get_object("disable_radio")


I would like to change these variable names. I'm missing the target here. What to enable root account, ssh access, password field?

I added "root" in the middle of the names, eg. disable_root_radio.

poncovka · 2021-07-27T10:18:27Z

Thanks for the reminder, I forgot about that part at some point. I changed the width to be 40 characters, not as wide as possible.

I think you should limit the width of the whole action area to make it more centred.

VladimirSlavik · 2021-07-29T20:05:50Z

Fun fact of the day: GTK3+ does not have a way to set maximum width. Instead, you have to make sure nothing within the widget of interest will expand. In this case, it was simple, the only expanding thing were the labels. As luck has it, labels, unlike other controls, do have a "maximum width" property... set in characters. Then, the "minimal width" can be set by adding a width request to something. I feel like it's 2000 again and I am trying to vertically center a thing on the web...

Here is 60 characters:

VladimirSlavik · 2021-07-29T20:15:55Z

Update: Limited width, rename radio buttons.

VladimirSlavik · 2021-07-29T20:16:45Z

/kickstart-test --testtype smoke

jkonecny12

I don't agree with the current enable_disable but I don't think it's reason for blocking this PR.

This resolves ambiguity of what options are available when root is locked. See also: rhinstaller#3262 (comment) Suggested-by: Mairin Duffy <mairin@redhat.com>

VladimirSlavik · 2021-08-05T17:20:51Z

Update: Changed the name of the handler per @jkonecny12 suggestion - thank you!

VladimirSlavik · 2021-08-05T17:21:04Z

/kickstart-test --testtype smoke

jkonecny12

Looks great to me now. Thanks!

Concerns only visible labels, internal name remains PasswordSpoke.

VladimirSlavik · 2021-08-16T14:31:15Z

/kickstart-test --testtype smoke

VladimirSlavik · 2021-08-16T14:32:23Z

Update: Renamed to Root Account. The longer version with Security seems to still fit in English, but I wonder about all the translations.

poncovka

LGTM!

jkonecny12

Still looks good to me!

VladimirSlavik · 2021-08-17T15:04:04Z

@AdamWill this will break OpenQA, and #3538 too. I'm about to merge these so the breakage is imminent.

(While at it, I'd like to also get into next Fedora also some rehash of #2242. That would address the easier GUI wants. But that's only hopes so far.)

AdamWill · 2021-08-25T22:03:23Z

So, one problem with merging this so late before Beta - it's not translated to any languages in current F35 compose. It's a big slab of untranslated text in every non-English language, which is unfortunate.

It would be great if we could get translations done and included for at least the major languages where we're usually complete or close to it, ahead of Beta. I'd certainly vote +1 on an FE bug to get translations in.

AdamWill · 2021-08-25T22:50:07Z

actually, never mind - I just realized I was looking at Rawhide openQA results when I thought I was looking at F35 ones. In F35 it seems the new screen isn't used, so it's not an issue. It's fine just to get the translations into Rawhide when we can.

M4rtinK · 2021-08-26T14:22:28Z

actually, never mind - I just realized I was looking at Rawhide openQA results when I thought I was looking at F35 ones. In F35 it seems the new screen isn't used, so it's not an issue. It's fine just to get the translations into Rawhide when we can.

The screen is actually part of the F35 build that narrowly missed the Beta Freeze deadline and is currently in Bodhi: https://bodhi.fedoraproject.org/updates/FEDORA-2021-c55ce41cef

If that's a problem we can just do a build just with the brltty change and do a full build with he GUI changes after the Beta freeze.

Still, given that the main deliverable (Fedora Workstation Live) does not have the root configuration screen & we have the option to do another build with updated translations via FE before the Beta RC, I would still prefer to get the new screen to the Beta.

VladimirSlavik added master Please, use the `f39` label instead. manual testing required This issue can't be merged without manual testing notable change Important changes like API change, behavior change... labels Jul 14, 2021

VladimirSlavik mentioned this pull request Jul 14, 2021

repair password entry box conflicts with root lock button when enter root_password spoke #3262

Closed

poncovka reviewed Jul 15, 2021

View reviewed changes

VladimirSlavik force-pushed the master-root-spoke-redesign branch from 2b4b66c to 653f35a Compare July 16, 2021 15:57

VladimirSlavik force-pushed the master-root-spoke-redesign branch from 653f35a to ca9d0ea Compare July 19, 2021 08:13

VladimirSlavik force-pushed the master-root-spoke-redesign branch from ca9d0ea to 35fa6c0 Compare July 19, 2021 12:28

VladimirSlavik force-pushed the master-root-spoke-redesign branch 3 times, most recently from c1b787f to f62b69f Compare July 22, 2021 10:54

VladimirSlavik removed the manual testing required This issue can't be merged without manual testing label Jul 22, 2021

VladimirSlavik force-pushed the master-root-spoke-redesign branch from f62b69f to 0e0ca0c Compare July 22, 2021 13:41

VladimirSlavik marked this pull request as ready for review July 22, 2021 14:29

VladimirSlavik force-pushed the master-root-spoke-redesign branch from 0e0ca0c to ea4608f Compare July 23, 2021 08:53

jkonecny12 requested changes Jul 26, 2021

View reviewed changes

VladimirSlavik force-pushed the master-root-spoke-redesign branch from ea4608f to af756ce Compare July 29, 2021 20:14

jkonecny12 self-requested a review August 5, 2021 09:39

jkonecny12 reviewed Aug 5, 2021

View reviewed changes

jkonecny12 requested review from jkonecny12 and removed request for jkonecny12 August 5, 2021 09:41

Change the root password spoke GUI design

73e514b

This resolves ambiguity of what options are available when root is locked. See also: rhinstaller#3262 (comment) Suggested-by: Mairin Duffy <mairin@redhat.com>

VladimirSlavik force-pushed the master-root-spoke-redesign branch from af756ce to 73e514b Compare August 5, 2021 17:18

jkonecny12 approved these changes Aug 12, 2021

View reviewed changes

poncovka approved these changes Aug 16, 2021

View reviewed changes

Rename spoke to Root Account

1ac8e07

Concerns only visible labels, internal name remains PasswordSpoke.

poncovka approved these changes Aug 16, 2021

View reviewed changes

jkonecny12 approved these changes Aug 17, 2021

View reviewed changes

VladimirSlavik merged commit d5ec5b4 into rhinstaller:master Aug 17, 2021

VladimirSlavik deleted the master-root-spoke-redesign branch August 17, 2021 15:05

poncovka mentioned this pull request Jan 31, 2022

Require admin user #3800

Closed

		self._enable_radio = self.builder.get_object("enable_radio")
		self._disable_radio = self.builder.get_object("disable_radio")

Change the root password spoke GUI design #3511

Change the root password spoke GUI design #3511

Conversation

VladimirSlavik commented Jul 14, 2021 • edited

VladimirSlavik commented Jul 14, 2021

VladimirSlavik commented Jul 14, 2021

mairin commented Jul 14, 2021

poncovka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poncovka commented Jul 15, 2021

jstodola commented Jul 16, 2021

jstodola commented Jul 16, 2021

poncovka commented Jul 19, 2021

VladimirSlavik commented Jul 19, 2021 • edited

VladimirSlavik commented Jul 22, 2021 • edited

VladimirSlavik commented Jul 22, 2021

VladimirSlavik commented Jul 22, 2021

mairin commented Jul 22, 2021

VladimirSlavik commented Jul 22, 2021

mairin commented Jul 22, 2021

jstodola commented Jul 23, 2021

jstodola commented Jul 23, 2021

jkonecny12 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poncovka commented Jul 27, 2021

VladimirSlavik commented Jul 29, 2021

VladimirSlavik commented Jul 29, 2021

VladimirSlavik commented Jul 29, 2021

jkonecny12 left a comment • edited

Choose a reason for hiding this comment

VladimirSlavik commented Aug 5, 2021

VladimirSlavik commented Aug 5, 2021

jkonecny12 left a comment

Choose a reason for hiding this comment

VladimirSlavik commented Aug 16, 2021

VladimirSlavik commented Aug 16, 2021

poncovka left a comment

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

VladimirSlavik commented Aug 17, 2021

AdamWill commented Aug 25, 2021

AdamWill commented Aug 25, 2021

M4rtinK commented Aug 26, 2021

VladimirSlavik commented Jul 14, 2021 •

edited

VladimirSlavik commented Jul 19, 2021 •

edited

VladimirSlavik commented Jul 22, 2021 •

edited

jkonecny12 left a comment •

edited

jkonecny12 left a comment •

edited