Reshape Kickoff avatar to a circle with Opacity Mask
ClosedPublic

Authored by ngraham on Jun 7 2018, 5:53 PM.

Details

Summary

Crop the square user avatar into a circle with an outline, like in SDDM and
the lock screen.

BUG: 386656
FIXED-IN: 5.18.0

Test Plan
  • Use Kickoff
  • Change/set avatar in User Manager
  • See pretty circle-cropped avatar

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sharvey created this revision.Jun 7 2018, 5:53 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 7 2018, 5:53 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sharvey requested review of this revision.Jun 7 2018, 5:53 PM

I encounter the very same issue as in D13202: Changing the Avatar in System Settings results in a blank circle in Kickoff. Here's a clue though: it only happens on the first change for each image. For example, if I change the image to something else in the gallery, hit apply, and open Kickoff, I see a blank circle. If I go to System Settings and change the avatar again--but choose the very same gallery image--then it shows up in Kickoff.

Here's the good news though: I see a variant of same thing without the patch: instead of being blank, it reverts to a previous image I used some time ago. So it looks like these patches may not be introducing the regression; instead they're simply changing the manifestation of a pre-existing regression. Perhaps touching this code may shed some light on that issue too.

As for this patch itself, I rather prefer it to the shader approach, which seems more complicated and fragile, and also a lot easier to understand for me (and probably future contributors who are as bad at programming as I am).

applets/kickoff/package/contents/ui/Header.qml
134

I think I might prefer a lighter color than this. Perhaps the window alternate background color?

sharvey added inline comments.Jun 7 2018, 9:30 PM
applets/kickoff/package/contents/ui/Header.qml
134

As soon as I get it working reliably and not showing blank circles...!

I encounter the very same issue as in D13202: Changing the Avatar in System Settings results in a blank circle in Kickoff. Here's a clue though: it only happens on the first change for each image. For example, if I change the image to something else in the gallery, hit apply, and open Kickoff, I see a blank circle. If I go to System Settings and change the avatar again--but choose the very same gallery image--then it shows up in Kickoff.

Hmm. Thanks for the clue. Not sure what to do with it yet.

I'm convinced there's some kind of caching or trick of timing involved. The correct local files get updated when the avatar is changed. Kickoff somehow notices the change - it goes blank - but it doesn't refresh with the new contents.

The Plasma team got their tarballs out today, so maybe I can nab someone's time in the next few days.

hein added a subscriber: hein.Jun 20 2018, 7:58 PM

I had a look at this today, and I wasn't able to reproduce the problem so far.

What I've done is test by using cp at a terminal to copy various PNG/SVG/JPG files into the $HOME/.face.icon location, which is what Kickoff (via a KDirWatch in KUserProxy in kdeclarative) watches for creation or changes. Kickoff picked up any change on the fly correctly.

That means both the kdeclarative and the Kickoff side are probably fine, and this shouldn't hold up this patch.

It means the User Manager KCM perhaps writes out corrupted images we cannot load occasionally, though, or that it's KIO copy-over of selected files is broken in some way. This needs to be debugged. Unfortunately I can't get the KCM to list any users on my system at all though, so I need to understand that first (likely a setup issue with my self-built Plasma). If anyone wants to debug this, please go ahead.

@hein : Thanks for the time spent investigating. The User Manager KCM appears to write out proper (non-corrupt) files, at least in my testing. I know there's a KDirWatch in one of the included Frameworks (KCoreAddons, I think), but that seems to work properly. It does indeed copy the file to the other places where it belongs. I'm still at a loss as to why it goes blank, but works when plasmashell is restarted. So Kickoff is picking up on the fact that there's been a change, but the operation doesn't complete.

Is there a way to force a refresh of Kickoff, an equivalent to kpackage5's "update" functiion?

hein added a comment.Jun 20 2018, 8:09 PM

You can run touch $HOME/.face.icon to make the system believe the file changed, then it goes through KDirWatch etc.

sharvey added a comment.EditedJun 20 2018, 9:41 PM

Perhaps I can go into the UserManager KCM, let it change the avatar like always, then run a brief timer (250ms?), and then ‘touch’ the face file so the KDirWatch runs a second time.

Trying to use touch made no difference. I called it manually between changes, and actually added it (temporarily) to the actual Kickoff code, so it "touched" every time Kickoff was opened. No change.

Anybody have another guess?

hein added a comment.Jun 25 2018, 7:08 PM

Can you elaborate what you tried and what you wanted to happen and what didn't happen? I'm a bit confused now @ touch.

In D13415#282860, @hein wrote:

Can you elaborate what you tried and what you wanted to happen and what didn't happen? I'm a bit confused now @ touch.

A few days ago, you suggested

You can run touch $HOME/.face.icon to make the system believe the file changed, then it goes through KDirWatch etc.

When picking a new avatar from user-manager, all the files all change properly: ~/.face, ~/.face.icon, and the IconItem source user-identity. But the avatar in Kickoff goes blank and will only refresh after a fresh login.

Trying to use touch as you suggested did not help: the Kickoff avatar stayed blank. I issued it from a terminal, no change. I added a call in the QML to trigger an external command (don't have that nasty little hack on hand any longer), trying to trigger a touch each time the Kickoff QML was run. Still no change. Killing and restarting plasmashell also didn't work.

In short, the avatar files are all correctly changed, but Kickoff refuses to update. It "updates" once to blank, but won't update again to the new image. Sidenote: there's some code in user-manager that explicitly deletes the old avatar, before installing the newly-chosen one. But even then, all the files get properly updated.

My machine crashed over the weekend, so I'm reciting some of this from memory. If I've got slight inaccuracies, I apologize.

I can put together a screen capture video if you want to see the steps I'm using.

hein added a comment.Jun 25 2018, 7:44 PM

Can you have a look at the code in kdeclarative, specifically src/qmlcontrols/kcoreaddons/kuserproxy.cpp - that's where the KDirWatch monitoring the face.icon file is. I think you need to place some qDebug() there to see if the file change gets picked up on your system and the signal emitted. If this is working, then we know the problem has to be in the QML side.

In D13415#282884, @hein wrote:

Can you have a look at the code in kdeclarative, specifically src/qmlcontrols/kcoreaddons/kuserproxy.cpp - that's where the KDirWatch monitoring the face.icon file is. I think you need to place some qDebug() there to see if the file change gets picked up on your system and the signal emitted. If this is working, then we know the problem has to be in the QML side.

Okay, I'll give that a shot. The worst that'll happen is that it won't work. ;-)

ngraham added a comment.EditedAug 28 2018, 11:29 PM

Perhaps this is a clue. With or without your patch, every single time I change the icon in user-manager, plasmashell immediately prints the following to the console:

file:///usr/share/plasma/plasmoids/org.kde.plasma.kickoff/contents/ui/Header.qml:55:5: QML Image: Error decoding: file:///var/lib/AccountsService/icons/dev: Unsupported image format

Notably, it's looking for the icon in /var/lib/AccountsService/icons/[username], not ~/.face.icon

The file exists and appears to be a valid image:

file /var/lib/AccountsService/icons/dev
/var/lib/AccountsService/icons/dev: PNG image data, 64 x 64, 8-bit/color RGBA, non-interlaced
xdg-open /var/lib/AccountsService/icons/dev
[opens in Gwenview]

So I think we can confirm that the icon is being changed on disk and that plasmashell sees the changed image and tries to load it.

However without your patch the avatar changes anyway (most of the time), while with your patch, it does almost never does until plasmashell is restarted. It seems like there is some latent bug with QML Image loading/parsing/MIMEtype detecting/something else that is only being triggered when the OpacityMask is applied to the Image...

BTW, you can do the OpacityMask inline, like so:

Image {
    id: faceIcon
    source: kuser.faceIconUrl

    [...]

    layer.enabled: true
    layer.effect: OpacityMask {
        // this Rectangle is a circle due to radius size
        maskSource: Rectangle {
            width: faceIcon.width
            height: faceIcon.height
            radius: height / 2
            visible: false
       }
    }

Thanks for the investigation. I've been hoping someone else would lend a pair of eyes to this problem for ages.

When you change the icon in user-manager repeatedly, are you prompted for a root password every time? On my system (Tumbleweed), there's a "grace period" where I don't get re-prompted and the icon doesn't change reliably. It may be different on different system. @hein has said this works for him, but his system ( Fedora?) doesn't use user-manager.

I think there's either a bug in QML or some weirdness in user-manager that I haven't been able to track down.

Thanks for the tip on cutting the OpacityMask circle, although I doubt it's related to the bigger issue.

Thanks for the investigation. I've been hoping someone else would lend a pair of eyes to this problem for ages.

When you change the icon in user-manager repeatedly, are you prompted for a root password every time? On my system (Tumbleweed), there's a "grace period" where I don't get re-prompted and the icon doesn't change reliably.

Right, there is a "grace period" of five minutes after every time you enter your password. This is built into PolicyKit, I believe. As far as I can tell from keeping a Dolphin window open showing the contents of /var/lib/AccountsService/icons/, the icon immediately gets updated correctly on disk every single time I change it in user-manager. I'm unable to fully confirm your assertion that the icon only reliably gets changed in Kickoff after being promped for a password. For me, it's a bit more subtle than that: the icon only reliably gets changed in Kickoff the first time I change it in user-manager that triggers a PolKit prompt after plasmashell has been started or restarted. I get a blank circle for all other circumstances:

  • Change icon in user-manager after restarting plasmashell less than 5 minutes after the last change, such that there is no PolKit prompt
  • Change icon in user-manager twice after restarting plasmashell, waiting more than 5 minutes each time so a PolKit prompt will be triggered; second update does not change Kickoff

I can also confirm @hein's report that this patch is 100% reliable when not using user-manager. I patched KUser::faceIconPath() in KCoreAddons to only ever return the ~/.face.icon path, instead of /var/lib/AccountsService/icons/$username, and it becomes 100% reliable. I don't get the "Unsupported image format" error anymore, which is a little odd because the two images are identical:

$ (master) md5sum /var/lib/AccountsService/icons/dev ~/.face.icon
28badcce926e3e4b1696e8bcac7b04d9 /var/lib/AccountsService/icons/dev
28badcce926e3e4b1696e8bcac7b04d9 /home/dev/.face.icon

What a weird bug.

ngraham commandeered this revision.Nov 21 2019, 8:08 PM
ngraham edited reviewers, added: sharvey; removed: ngraham.

I'm not 100% sure why, but the outstanding issues with this patch appear to be totally fixed with D25439. I'll finish it up.

ngraham updated this revision to Diff 70123.Nov 21 2019, 8:29 PM

Rebase and simplify

ngraham marked 2 inline comments as done.Nov 21 2019, 8:30 PM
ngraham edited the summary of this revision. (Show Details)Nov 21 2019, 8:33 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham edited reviewers, added: VDG, Plasma; removed: davidedmundson, mart, apol, abetts.
ngraham updated this revision to Diff 70124.Nov 21 2019, 8:41 PM

Handle the software rendering case

filipf added a subscriber: filipf.Nov 21 2019, 9:12 PM

I'm thinking users might use PNGs with transparency for their avatars, which means they might not want to have a border around it.

I haven't found a way to detect transparency in a QML Image the way you can with QImage. :/

mart accepted this revision.Nov 29 2019, 2:45 PM
This revision is now accepted and ready to land.Nov 29 2019, 2:45 PM
This revision was automatically updated to reflect the committed changes.