Redesign Advanced tab
ClosedPublic

Authored by filipf on Jun 5 2019, 5:55 PM.

Details

Summary

This patch redesigns and modernizes the Advanced tab of SDDM's KCM.

Test Plan

Before:

After:

Entries were still being written to /etc/sddm.conf

Diff Detail

Repository
R123 SDDM Configuration Panel (KCM)
Branch
redesign-sddm-advanced-tab (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12458
Build 12476: arc lint + arc unit
filipf created this revision.Jun 5 2019, 5:55 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 5 2019, 5:55 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Jun 5 2019, 5:55 PM
filipf edited the test plan for this revision. (Show Details)Jun 5 2019, 5:57 PM
filipf added reviewers: ngraham, vdavda.
filipf edited the test plan for this revision. (Show Details)
filipf edited reviewers, added: VDG, davidedmundson; removed: vdavda.
filipf planned changes to this revision.Jun 5 2019, 6:48 PM

The spacing between sections seems too big, I need to reduce it so that it matches the one in Kirigami's FormLayout.

The spacing between sections seems too big, I need to reduce it so that it matches the one in Kirigami's FormLayout.

QSpacerItem works well for this, and you can customize the height so that it matches perfectly.

abetts added a subscriber: abetts.Jun 5 2019, 8:05 PM

+1 on visuals

filipf updated this revision to Diff 59231.Jun 5 2019, 9:59 PM

reduce height of vertical spacers

filipf edited the test plan for this revision. (Show Details)Jun 5 2019, 10:00 PM
filipf edited the test plan for this revision. (Show Details)
filipf updated this revision to Diff 59232.Jun 5 2019, 10:02 PM

update comment

ngraham requested changes to this revision.Jun 6 2019, 12:50 AM

Nice. Looks like this layout includes some empty whitespace on the bottom though. Open it in kcmshell and reduce the window height:

This revision now requires changes to proceed.Jun 6 2019, 12:50 AM
filipf added a comment.Jun 6 2019, 8:59 AM

I believe that's due to the other tab. If you try to resize the Themes tab height-wise, you'll notice the scrollbar appears when the bottom buttons start disappearing. I'll investigate some more though.

Ah you're right, that's a pre-existing issue. However it does get worse with this redesign since the Advanced page is now much more compact. Oh well.

I've found one new issue: when auto-login is enabled and you go to the Advanced page, its checkbox is checked but none of the related controls are un-disabled:

When you toggle it off and on again, they do get enabled as expected. An inline comment explains why:

src/ui/advanceconfig.ui
247

This and other subsequent connections are probably the problem. The signal is only getting passed along when the checkbox is actually clicked. But nothing is connected to its checked() signal, so it doesn't know to enable the controls when the checkbox starts out checked but is not explicitly clicked by the user.

You can probably change clicked to checked.

filipf updated this revision to Diff 59280.Jun 6 2019, 6:02 PM

use toggled instead of clicked

Much better, thanks! Works now.

One final thing, maybe: can we re-word "Relogin after quit"? I admit I have no idea what it even does! It's also got an English error ("relogin" isn't a word) and it has no tooltip to explain what it does. Needs a bit of loving.

filipf added a comment.Jun 6 2019, 6:16 PM

Much better, thanks! Works now.

One final thing, maybe: can we re-word "Relogin after quit"? I admit I have no idea what it even does! It's also got an English error ("relogin" isn't a word) and it has no tooltip to explain what it does. Needs a bit of loving.

I had no idea what it meant either lol. Then I thought about it a bit and assumed it means that when you have autologin on and you log off it just logs you back in. Which tests seem to confirm.

My suggestion is to rename it to "Log in again after logging off"

My suggestion is to rename it to "Log in again after logging off"

Much better! How about "Log in again after immediately after logging off" to emphasize how instant it is? Or even "After logging off, immediately log in again".

filipf added a comment.Jun 6 2019, 6:22 PM

My suggestion is to rename it to "Log in again after logging off"

Much better! How about "Log in again after immediately after logging off" to emphasize how instant it is? Or even "After logging off, immediately log in again".

Nice, I'll do option no.1 because the HIG tells us to start off checkbox labels with an action verb.

filipf updated this revision to Diff 59282.Jun 6 2019, 6:24 PM

improve string "Relogin after quit"

ngraham accepted this revision.Jun 6 2019, 6:31 PM

Shipit! Master only, obviously

This revision is now accepted and ready to land.Jun 6 2019, 6:31 PM
This revision was automatically updated to reflect the committed changes.