-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change the root password spoke GUI design #3511
Change the root password spoke GUI design #3511
Conversation
@mairin, please comment on the preferred form of attribution, if you want it different. |
Wow that looks wonderful, @VladimirSlavik !!! The padding / margins / spacing etc. are right on and the animation looks slick. 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great!
@@ -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(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be tested with the initial setup.
Please, test it with |
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. |
2b4b66c
to
653f35a
Compare
@AdamWill please be aware of this UI change, since it might affect your Fedora installation testing. |
653f35a
to
ca9d0ea
Compare
Also, add a release note for this change, please (https://anaconda-installer.readthedocs.io/en/latest/contributing.html#release-notes). |
ca9d0ea
to
35fa6c0
Compare
@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? |
c1b787f
to
f62b69f
Compare
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. |
f62b69f
to
0e0ca0c
Compare
/kickstart-test --testtype smoke |
/kickstart-test --testtype users |
@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! |
Thanks - and here you go! new-root-spoke2.mp4 |
@VladimirSlavik looks great to me, I see no issue! |
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? |
0e0ca0c
to
ea4608f
Compare
It looks great to me, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I really like the new solution, however, I would like to make the variable names a bit more clear.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case is disable_enable
a good control name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems much better to understand to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I will do that, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
self._enable_radio = self.builder.get_object("enable_radio") | ||
self._disable_radio = self.builder.get_object("disable_radio") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to change these variable names. I'm missing the target here. What to enable root account, ssh access, password field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added "root" in the middle of the names, eg. disable_root_radio.
I think you should limit the width of the whole action area to make it more centred. |
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: |
ea4608f
to
af756ce
Compare
Update: Limited width, rename radio buttons. |
/kickstart-test --testtype smoke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
af756ce
to
73e514b
Compare
Update: Changed the name of the handler per @jkonecny12 suggestion - thank you! |
/kickstart-test --testtype smoke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me now. Thanks!
Concerns only visible labels, internal name remains PasswordSpoke.
/kickstart-test --testtype smoke |
Update: Renamed to Root Account. The longer version with Security seems to still fit in English, but I wonder about all the translations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me!
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. |
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. |
This resolves ambiguity of what options are available when root is locked.
This is a draft because there are at least these TODOs:add an "empty password" bannerautomatically switches to no password + locked instead.See also: #3262 (comment)
Suggested-by: Mairin Duffy mairin@redhat.com @mairin