Align lock icon with bold message text; reduce overall size of dialog
ClosedPublic

Authored by sharvey on Apr 18 2018, 2:36 PM.

Details

Summary

Adjusted spacers in UI file to properly align icon with message text. Also attempt to minimize white space in dialog box by hiding UI elements until they are needed. (Set height to 1, then restore to original layout size.)

Test Plan
  • Compile polkit-kde-agent-1 with patch
  • Trigger a root password request window (launch Synaptic, try to change SDDM in System Settings)
  • Ensure that lock icon is aligned with bold headline text

Diff Detail

Repository
R121 Policykit (Polkit) KDE Agent
Branch
align-lock-icon (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
sharvey created this revision.Apr 18 2018, 2:36 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 18 2018, 2:36 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sharvey requested review of this revision.Apr 18 2018, 2:36 PM

Probably fine, but can you attach a screenshot

sharvey edited the test plan for this revision. (Show Details)Apr 18 2018, 2:37 PM
sharvey added reviewers: davidedmundson, ngraham.

Probably fine, but can you attach a screenshot

You're too quick on the draw. Was still editing the summary!

sharvey added a comment.EditedApr 18 2018, 2:39 PM


Before



After

I just realized I inadvertently shrank the spacing between the bold headline and the non-bold explanation text. There's a lot of white space in this dialog. Let me know if you want something adjusted...

+1 for much much less whitespace.

abetts accepted this revision.Apr 18 2018, 3:00 PM
This revision is now accepted and ready to land.Apr 18 2018, 3:00 PM


How's this? It's a little tighter.

Some of the white space is reserved for otherwise-invisible error messages (like an invalid password).

If it gets more +1's, I'll update.

Better already! Do we even need to reserve that space? Many other such dialogs dynamically allocate it when necessary by increasing the height of the box. Is that not possible here?

Better already! Do we even need to reserve that space? Many other such dialogs dynamically allocate it when necessary by increasing the height of the box. Is that not possible here?

It's got a Big Scary Red Box that appears for an error. It's already in the UI design, just not visible. So when it's set to visible, its space is already allocated.

There's another hidden UI element above it, which is a combo box for selecting a different user. I've only got one user on my system, but i presume it activates when superuser privileges are requested, allowing you to enter the root password as a different user.

It's got a Big Scary Red Box that appears for an error. It's already in the UI design, just not visible. So when it's set to visible, its space is already allocated.

There's another hidden UI element above it, which is a combo box for selecting a different user. I've only got one user on my system, but i presume it activates when superuser privileges are requested, allowing you to enter the root password as a different user.

Right. What I'm saying is: can we make the box compact by default, and become taller to accommodate the above-mentioned UI elements only when they're actually being displayed?

BTW, creating another user and then removing it later is trivially easy in System SettingsAccount DetailsUser Manager.

This comment was removed by sharvey.
sharvey updated this revision to Diff 32496.Apr 18 2018, 5:57 PM
  • Reduce height of dialog by shrinking UI elements when not needed

Password error alert and user-choice combo box are reduced to 1px high when not needed, then restored to their original geometry when activated.

Here's the latest from the Pixel Conservation Society. I can't manage wholesale space-savings without breaking up the existing gridLayout and basically starting from scratch. It does resize a bit and does no harm.

sharvey updated this revision to Diff 32498.Apr 18 2018, 6:14 PM
  • Correct initial Y placement for errorMessageWidget
sharvey retitled this revision from Align lock icon with bold message text to Align lock icon with bold message text; reduce overall size of dialog.Apr 18 2018, 6:17 PM
sharvey edited the summary of this revision. (Show Details)
davidedmundson accepted this revision.Apr 18 2018, 6:18 PM
ngraham accepted this revision.Apr 18 2018, 6:42 PM
ngraham added a reviewer: Frameworks.

Much better! I'd still prefer less whitespace, but this is already an improvement. But now that I'm staring at this dialog over and over again, I'm wondering if we even need the non-bold caption text at all. It just repeats the bold text in a slightly more awkward and more wordy fashion. What do you think?

I think there's something severly wrong with the dialog's size hints if we need to add arbitrary numbers all over the place. Can you make sure this doesn't break with

  • different font
  • larger font
  • screen scaling
  • different widget style
  • ...


The dialog seems immune to QT_SCALE_FACTOR=2.0 (because it's launched from a separate process?). I don't have a HiDPI display to test on.

I tried different fonts up to 15pt and all is okay. I even tried the nasty Windows-style widgets and it's still okay.

I agree the layout is kind of a mess. I can take a shot at reworking it if the reviewers think it's worth the effort.

I think it would be worth it! It's definitely not a crown jewel of polished UI design, that's for sure.

To test scaling, try setting a systemwide scale factor in System SettingsDisplay and MonitorDisplaysScale Display, then logging out and back in, then doing it again,


The good news is that it still behaves properly with a scaled display.

The bad news is that, due to the minimumSize we configured for System Settings, I can't get back to the bottom of the Display KCM to reset my screen scale. Had to set QT_SCALE_FACTOR=0.5 and launch systemsettings5 directly. Never a dull moment!


The good news is that it still behaves properly with a scaled display.

The bad news is that, due to the minimumSize we configured for System Settings, I can't get back to the bottom of the Display KCM to reset my screen scale. Had to set QT_SCALE_FACTOR=0.5 and launch systemsettings5 directly. Never a dull moment!

Ouch, that's pretty bad. Perhaps we should move the button up higher in the window so you'll always be able to reach it no matter what weird state you find yourself in.

Reviewers: as part of my task to redesign and tidy up this dialog box, I'm considering removing the Details button in the bottom left corner, along with the small pop-open panel that shows additional information. My argument is that the info in the Details panel is quite technical (PID's of calling process and polkit process) and - in some circumstances - lucid information about the program needing authorization. Most of the time, what's coming through is in raw form, such as com.canonical.ubuntu.synaptic (more or less) instead of Synaptic Package Manager.

I know carving out UI elements can be controversial, so please give a few +/- 1's so I know what others think. Maybe some people use it. I don't. But that's not a good enough reason to remove it.

ngraham added a comment.EditedApr 20 2018, 5:55 PM

In general it's okay to display nerdy technical information hidden away like this--as long as it's actually useful information! That's the real question. If it's of no real value to anyone for any use case that we can imagine, we can probably safely remove it. Otherwise, it should probably stay in.

Personally I have no use for it, but let's collect more perspectives.

Do you really want to remove the proper source of information in security dialog that asks you some additional credentials?
Please keep it there.

bruns added a subscriber: bruns.Apr 20 2018, 8:32 PM

If you really want to save some space (and probably make it easier for users to understand the dialog without getting lost), I think the whole

An application is attempting to perform an action that requires privileges.
Authentication is required to perform this action.

boilerplate can be removed. It is some lenghty, generic text, which does not add any useful information.

If the Action in the Details tab is showing the raw id of the action, its the fault of the action definition, lacking a <description> tag.

If you really want to save some space (and probably make it easier for users to understand the dialog without getting lost), I think the whole

An application is attempting to perform an action that requires privileges.
Authentication is required to perform this action.

boilerplate can be removed. It is some lenghty, generic text, which does not add any useful information.

If the Action in the Details tab is showing the raw id of the action, its the fault of the action definition, lacking a <description> tag.

What about something like:

"Authentication is required to perform this action. Please enter your administrator password"

And nothing else. No 3 lines but just one.

??

bruns added a comment.Apr 20 2018, 8:41 PM

The "Authentication is required to ..." header line is directly sourced from the action definition below /usr/share/polkit-1/actions/*

"Administrator password" is insufficient/wrong, as you can also (dependent on system configuration) authorize the actions as another priviledged user, see the screenshots.

Password is redundant (label on the textfield), and wrong, as you can potentially use something like a fingerprint to authenticate.

bruns added a comment.Apr 20 2018, 8:43 PM

Btw, an easy way to trigger the dialog is e.g.

$> pkcheck -u -p $$ -a org.freedesktop.udisks2.eject-media-system

$$ is the PID of the shell.

The small-text boilerplate is definitely on the chopping block. @ltoscano makes a fine case for keeping the Details section.

@bruns - Thanks for helping me find the source of the incoming messages. I hadn't gotten around to searching for them yet, but you saved me some work. Thanks!

Semi-related bug from @stikonas, while I'm on dialog duty: https://bugs.kde.org/show_bug.cgi?id=393355

ngraham added a comment.EditedApr 20 2018, 9:17 PM

So right now, we have two strings of text:

  • The top bold string comes from the app and differs on a per-app basis
  • The bottom long boilerplate string is from us, and shown all the time

I like @abetts' general idea: we should remove the app-specific text and make the always-shown boilerplate text short and to the point. We can massage the wording to apply to all situations.

Hmm, I was actually leaning the other way. Ditch the generic boilerplate and keep the app-specific text. I think it's helpful when the dialog tells you why it appeared and what app/function is requesting your password.

bruns added a comment.Apr 20 2018, 9:24 PM

No, thats completely off, as thats the action you are authorizing.

Authorization is required to format disk WDC WD10EZEX-08M2NA0

I want it to show what it is asking permission for, and to be specific - does it want to format the USB stick I just inserted, or my home partition.

No, thats completely off, as thats the action you are authorizing.

Authorization is required to format disk WDC WD10EZEX-08M2NA0

I want it to show what it is asking permission for, and to be specific - does it want to format the USB stick I just inserted, or my home partition.

As long as you keep the detailed information available, that's fine. You can always end up having a non useful name for your USB stick...

bruns added a comment.Apr 20 2018, 9:27 PM

Its not available otherwise ...

No, thats completely off, as thats the action you are authorizing.

Authorization is required to format disk WDC WD10EZEX-08M2NA0

I want it to show what it is asking permission for, and to be specific - does it want to format the USB stick I just inserted, or my home partition.

Partition Manager wouldn't be able to show such specific text anyway (what is being formatted). It just starts a helper that can execute any command it asks. You can't make it work otherwise.

Gnome-disks does, and so do other programs.

stikonas added a comment.EditedApr 21 2018, 6:27 AM

Gnome-disks does, and so do other programs.

Gnome-disks uses UDisks to do everything and can't do anything on its own. So available features are very limited (not even resizing partitions).
I suspect it is significantly less portable (e.g. to FreeBSD).
That's why gnome-disks was written from scratch (with fewer features than gparted that it was supposedly replacing) and it wasn't the case of somebody porting gparted to udisks .

sharvey updated this revision to Diff 32758.Apr 22 2018, 12:01 AM
  • Merge branch 'master' into align-lock-icon
  • Undo manual resizing of ser combo box & password error box

Okay, this patch is back to where we started, with nothing changed except centering the icon. Hopefully we can commit it now. I'll move the discussion over how to redesign the dialog itself to a separate task.

FYI: I undid the wonky manual UI sizing that @broulik objected to. In hindsight, not the best approach.

bruns added a comment.Apr 23 2018, 2:09 AM

Gnome-disks does, and so do other programs.

Gnome-disks uses UDisks to do everything and can't do anything on its own. So available features are very limited (not even resizing partitions).
I suspect it is significantly less portable (e.g. to FreeBSD).
That's why gnome-disks was written from scratch (with fewer features than gparted that it was supposedly replacing) and it wasn't the case of somebody porting gparted to udisks .

Resizing: http://storaged.org/doc/udisks2-api/latest/gdbus-org.freedesktop.UDisks2.Partition.html#gdbus-method-org-freedesktop-UDisks2-Partition.Resize

Does not work well yet, just a few errors where KPM succeeds:

  • Cannot resize btrfs filesystem on /dev/sdb1: (null) filesystem 'btrfs' is not supported.
  • LUKS encrypted ext4: looks like I can resize internal file system but no way to resize outer LUKS container (i.e. what cryptsetup resize does)
  • Does not recognize LVM PVs, no way to resize them.

On the other hand, it might be useful in certain cases. E.g. to resize FAT and maybe HPFS (not all distros ship fatresize).

In any case, resizing is not the only thing. No way to copy/move file systems.

I'm not opposed to UDisks in principle, I am just saying it's not there yet.

Does not work well yet, just a few errors where KPM succeeds:

  • Cannot resize btrfs filesystem on /dev/sdb1: (null) filesystem 'btrfs' is not supported.

Wrong, tested, works. You have to use http://storaged.org/doc/udisks2-api/latest/gdbus-org.freedesktop.UDisks2.Filesystem.BTRFS.html

Can we commit this?

I believe it's done.

ngraham accepted this revision.Jul 23 2018, 2:27 PM

Go for it then!

Are you going to land this?

This revision was automatically updated to reflect the committed changes.