[sddm-theme] Add slot to configure a logo to be shown on the SDDM login screen
AcceptedPublic

Authored by cblack on Jul 14 2019, 8:14 PM.

Details

Reviewers
filipf
ngraham
Group Reviewers
VDG
Plasma
Summary

The Breeze SDDM theme now shows a logo that can be configured by distro vendors or curious users or anyone else.

Test Plan

Test configuration and defaults.

Default logo:

Example with a distro logo:

Diff Detail

Repository
R120 Plasma Workspace
Branch
distro-logo-slot (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14047
Build 14065: arc lint + arc unit
cblack created this revision.Jul 14 2019, 8:14 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 14 2019, 8:14 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Jul 14 2019, 8:14 PM
cblack updated this revision to Diff 61756.Jul 14 2019, 8:21 PM

This is why you shouldn't forget to test - default logo is now no longer pitifully small

cblack edited the test plan for this revision. (Show Details)Jul 14 2019, 8:24 PM
ngraham added a subscriber: ngraham.

An interesting idea. Are you in contact with any distros that have requested this or would make use of it?

I wouldn't mind adding some text to the default logo so for any distros that opt to turn it on and not override the defaults, people learn what it actually is (Plasma's logo recognition is low right now). Then again that means unlocalized text, unless you do something clever to allow the logo component to pull text from a localizable file.

An interesting idea. Are you in contact with any distros that have requested this or would make use of it?

This was created directly because an LCP (artwork dude) at openSUSE requested it (see VDG chat).

I wouldn't mind adding some text to the default logo so for any distros that opt to turn it on and not override the defaults, people learn what it actually is (Plasma's logo recognition is low right now). Then again that means unlocalized text, unless you do something clever to allow the logo component to pull text from a localizable file.

Names in logos tend not to be localized from roman characters -> other character sets, so this really isn't a large concern for me.

cblack updated this revision to Diff 61761.Jul 14 2019, 10:09 PM

Logo has text now

cblack edited the test plan for this revision. (Show Details)Jul 14 2019, 10:09 PM
ngraham added inline comments.Jul 15 2019, 1:38 AM
sddm-theme/Main.qml
424

Match the duration for other opacity animation effects (i.e. use units.longDuration)

444

ditto

cblack updated this revision to Diff 61774.Jul 15 2019, 1:46 AM

Use units.longDuration

broulik added inline comments.
sddm-theme/Main.qml
414

Why not just a bool?

415

Also set a sourceSize and probably asynchronous: true

421

This is not what spacing is supposed to be used for. Try units.gridUnit

Idea makes sense.

lets drop the word "distro" and then we can use it for corporation/city/whatever logos

sddm-theme/Main.qml
423

OpacityAnimator is better where possible

cblack added inline comments.Jul 15 2019, 4:54 PM
sddm-theme/Main.qml
414

the config object exposes true as "true", not true, so ¯\_(ツ)_/¯

cblack updated this revision to Diff 61809.Jul 15 2019, 5:20 PM

Rename config file properties to not say "distro"
Use gridunit
Use opacityanimator

cblack added inline comments.Jul 15 2019, 5:21 PM
sddm-theme/Main.qml
415

I haven't a clue what I should set sourceSize to ¯\_(ツ)_/¯

filipf added a subscriber: filipf.Jul 15 2019, 5:29 PM
filipf added inline comments.
sddm-theme/Main.qml
415

to the height and width of the item I believe, at least that's what I did here: https://github.com/KDE/sddm-kcm/blob/master/src/qml/main.qml#L54

cblack added inline comments.Jul 15 2019, 5:33 PM
sddm-theme/Main.qml
415

I don't really have a defined width, but I guess I could define the height

cblack updated this revision to Diff 61813.Jul 15 2019, 5:35 PM

Define sourceSize.height

filipf added inline comments.Jul 15 2019, 5:40 PM
sddm-theme/Main.qml
415

You'll notice that in my example I didn't have height defined, but the sourceSize code should still work because the fillMode magic does set some definitive width.

cblack updated this revision to Diff 61814.Jul 15 2019, 5:42 PM

Add sourceSize.width to the party

I'm testing this out now and there seems to be only one obstacle left, see inline comment

sddm-theme/Main.qml
424

units.gridUnit is larger than units.largeSpacing, you can't keep 8 as the multiplier:

cblack updated this revision to Diff 61819.Jul 15 2019, 6:12 PM

Sizing tweaks

please don't make this
it look not good
we can make it more smaller and put it on any corner

cblack edited the test plan for this revision. (Show Details)Jul 15 2019, 6:40 PM

Logo size still looks too big to me; IMO these things shouldn't be too much in the users' face and the logo shouldn't overpower the clock. Also I think the second screenshot doesn't accurately represent new code.

You had units.largeSpacing * 8 before => 8 * 8 = 64

Now it's units.gridUnits * 6 => 18 * 6 = 108

Ideally we'd multiply units.gridUnits with 3 or 4 to get to 64 again (or do Math.round when multiplying with 3.5).

For me units.gridUnits * 3 looks perfect:

And it seems I didn't give the best of advice for sourceSize.width as there's now a binding loop. If width is changed with implicitWidth it goes away though.


On a more general note, I'd keep the logo turned off by default but let's see what others think. Looking into the future it would also be good to add a UI option and chooser too, but not important now.

Yeah, I woudn't actually mind showing the Plasma logo and text there by default at some point as long as it's small and tasteful. Our branding is pretty weak right now and any improvement there is welcome IMO.

But yeah, that's a discussion for later.

cblack updated this revision to Diff 61825.Jul 15 2019, 7:25 PM

More sizing tweaks

cblack edited the test plan for this revision. (Show Details)Jul 15 2019, 7:26 PM
filipf accepted this revision as: filipf.Jul 16 2019, 9:58 AM

As far as I'm concerned the patch is landable; I really like that it's developed into a more generic placeholder rather than one just for distro logos. Nice work @cblack, make sure to edit the commit title and message to reflect this though.

Some things to still consider in the near or more distant future:

  • present to distros (namely openSUSE) and see if this is what they wanted
  • see if we want this on by default
  • add a UI option for having a logo and then an image chooser
  • see if the drop shadow will be interfering with existing drop shadows in logos (but logo authors can just remove it themselves I guess)
  • move the default svg to the artwork subfolder since it feels like it belongs there
  • fix the binding loop related to sourceSize.width
sddm-theme/Main.qml
421

try implicitWidth to avoid binding loop

This revision is now accepted and ready to land.Jul 16 2019, 9:58 AM
sddm-theme/Main.qml
421

Just remove sourceSize.width

We dont define a width.

cblack updated this revision to Diff 61859.Jul 16 2019, 1:57 PM

Remove sourceSize.width

cblack retitled this revision from [sddm-theme] Add slot for distro vendors to configure a logo to be shown on the SDDM login screen to [sddm-theme] Add slot to configure a logo to be shown on the SDDM login screen.Jul 16 2019, 1:58 PM
cblack edited the summary of this revision. (Show Details)
ngraham added inline comments.Jul 16 2019, 4:46 PM
sddm-theme/theme.conf.cmake
2

I thought this was going to be off by default for now?

cblack added a comment.EditedJul 16 2019, 4:52 PM

Doesn't really matter to me ¯\_(ツ)_/¯
If other people want it off by default, I'll update the default theme.conf

edit: brainfart - forgot how to reply to inline comments. whoops.

ngraham accepted this revision.Jul 16 2019, 4:53 PM

All right, let's try having it on by default for now and see if we hate it during the time before 5.17 branches. If so, we can always turn it off before then.

Plasma folks?

Off.

A login manager does not know what session you will be using.

cblack updated this revision to Diff 61916.Jul 17 2019, 2:05 PM

Hide logo by default