Show POTD in lock screen
Needs ReviewPublic

Authored by guoyunhe on Apr 23 2020, 7:39 PM.

Details

Reviewers
davidre
broulik
ngraham
Group Reviewers
Plasma
Summary
  1. POTD was cached in ~/.cache/. But lock screen seems not a process of current user, so it won't find the cache in user's home dir. So I changed cache location to /tmp and it starts to work.
  2. Flickr and APOD(NASA) providers cannot cache pictures in PNG but JPEG works. So I simply changed the cache format to JPEG. Most POTD are photos. JPEG saves more spaces.
  3. Added a KDED module to cache lockscreen POTD, because lockscreen program doesn't have internet access.

BUG: 389962
FIXED-IN: 5.20

Test Plan
  1. Apply patch
  2. Kill and restart kded5
  3. Set a POTD slideshow as your lock screen background
  4. Lock the screen

Diff Detail

Repository
R114 Plasma Addons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26991
Build 27009: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Plasma. · View Herald TranscriptApr 23 2020, 7:39 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
guoyunhe requested review of this revision.Apr 23 2020, 7:39 PM
guoyunhe edited the summary of this revision. (Show Details)Apr 23 2020, 9:38 PM
guoyunhe updated this revision to Diff 81064.Apr 24 2020, 6:18 AM

Fix APOD and Flicker provider cache

Thanks for working on this!

It seems like to make this cache work on the lock screen, the file needs to be added to the cache *before* the screen locks, even if it hasn't been added there by the desktop wallpaper.

To totally solve the issue, there are several solutions I can find:

  1. POTD need to have a daemon process that can always access network. The lock screen send request to the daemon through DBus or other protocol.
  2. Create an invisible lock screen widget in the desktop, so it fetches and updates the POTD even before screen locks.

The second option is much easier to do. Both will cause some performance impact.

To totally solve the issue, there are several solutions I can find:

  1. POTD need to have a daemon process that can always access network. The lock screen send request to the daemon through DBus or other protocol.
  2. Create an invisible lock screen widget in the desktop, so it fetches and updates the POTD even before screen locks.

    The second option is much easier to do. Both will cause some performance impact.
  1. doesn't sound something like we want do

Anyway, this patch just focus on fixing the cache issue.

ngraham requested changes to this revision.Wed, May 6, 6:26 PM

So this only fixes the problem when the POTD image has already been cached by the plugin for the desktop wallpaper? While the patch would fix that particular use case, I don't think that's a general purpose solution, and if we do implement a general-purpose solution, we won't have to do this. So I think we should focus on the general solution, sorry. Maybe we can have kded or another already-running daemon perform the caching?

dataengines/potd/cachedprovider.cpp
54

seems unrelated

This revision now requires changes to proceed.Wed, May 6, 6:26 PM

I was looking for kded documentation but cannot find a good example. Do you know any?

guoyunhe updated this revision to Diff 82610.Mon, May 11, 9:30 PM

Draft for PoTD KDED. Not tested.

guoyunhe updated this revision to Diff 82619.Tue, May 12, 6:46 AM

Add file watcher

guoyunhe updated this revision to Diff 82620.Tue, May 12, 6:50 AM

Clean up libraries

I created the kded service and it seems running. But it doesn't cache the image as I expected. Don't know how to debug it.

guoyunhe updated this revision to Diff 82632.Tue, May 12, 7:15 AM

Fix config file path

guoyunhe retitled this revision from Fix POTD in lock screen (partly) to Fix POTD in lock screen (complete).Tue, May 12, 7:21 AM
guoyunhe edited the summary of this revision. (Show Details)

@ngraham the kded module is working now. please have a look!

guoyunhe updated this revision to Diff 82635.Tue, May 12, 7:52 AM

Fix watcher when file is recreated

ngraham resigned from this revision.Wed, May 13, 6:55 PM
ngraham added reviewers: davidre, broulik.

It works! Great job.

I'd like Plasma people to review this as well, and I think we should wait until after 5.19 branches before landing it, as it's generally unwise to land big patches or features right before the beta period begins.

ngraham retitled this revision from Fix POTD in lock screen (complete) to Show POTD in lock screen.Wed, May 13, 6:56 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

I fail to see what the kded module actually does. Or is it the case that simply requesting the data from the engine causes it to cache the image on disk? Maybe that should be documented.

CMakeLists.txt
44

Seems unused?

68

unrelated

dataengines/potd/PoTD-list.txt
8

Unrelated

dataengines/potd/cachedprovider.cpp
54

Can't we save the image in its original format?

kdeds/potd/kded_potd.cpp
4

QDebug

11

unused

13

You can use K_PLUGIN_CLASS_WITH_JSON

19

this leaks

35

Why don't we care if the data was updated?

47

Why?

50

Maybe getProvider or getProviderName no to confuse it with the dataSource?

52

Couldn't we get the config directly with "kscreenlockerrc" if we use cascading either way? No need to seach for the actual path

kdeds/potd/kded_potd.h
22

remove

guoyunhe marked 9 inline comments as done.Thu, May 14, 8:07 AM
guoyunhe added inline comments.
CMakeLists.txt
44

It is necessary for the KDED module. Without it, the compilation fails.

68

Will revert.

dataengines/potd/PoTD-list.txt
8

It is related. It explains why sometimes APOD still fails to show in lock screen even after this fix.

dataengines/potd/cachedprovider.cpp
54

JPEG is the original format for all POTD providers. For the save() function, you must specify format or extension. It cannot guess the original format.

kdeds/potd/kded_potd.cpp
4

Will remove.

11

Will remove.

13

Changed

19

Fixed.

35

We only want to trigger the caching behavior of POTD data engine. No need to do anything with the data itself.

47

It won't work without this line. Each time you change and save lockscreen configuration, the QFileSystemWatcher stop watching the configuration file.

52

I tried but it doesn't work.

kdeds/potd/kded_potd.h
22

Will do.

guoyunhe updated this revision to Diff 82819.Thu, May 14, 8:08 AM
guoyunhe marked 9 inline comments as done.

Fix small issues

guoyunhe updated this revision to Diff 82821.Thu, May 14, 8:59 AM

Code improvement

I don't get how saving as a png fails if we have the image as QImage.

kdeds/potd/kded_potd.cpp
10

still leaks

29

This method can be removed it's not referenced anywhere.

Maybe it is some unknown Qt bug. You can try set wallpaper as Flickr POTD and check if you have ~/.cache/plasmashell/plasma_engine_potd/flickr saved. Anyway, for photos, JPEG makes more sense than PNG.

For the leaks, I really have no idea. (also asked some friends but no help) C++ isn't my primary programming language. If you can share some document/tutorials/examples...

Flickr works here

For the leaks, I really have no idea. (also asked some friends but no help) C++ isn't my primary programming language. If you can share some document/tutorials/examples...

In C++ there is automatic and dynamic storage duration (and some others but you don't need to care about them for now). Automatic is the normal when you write

 void f() {
    int i = 0;
    [complicated code]
}

i is automatically allocated at the start of the function and deallocated at the end of the function. Objects with automatic storage duration are deallocated at the end of the scope they were declared in. You can't use a variable that you declare in a for loop or inside an if block outside of them because the scope they were declared in (the foor loop or the if block) has ended. You can also manually create a scope by wrapping code with braces.

Dynamic storage duration happens when you manually allocate memory by writing new Object or more C-like by calling malloc or calloc. Here the memory is not automatically freed again. It's on the programmer to deallocate it when it's no longer needed. You need to call delete (or free if you used malloc) to free the memory again.

Here it's not a huge problem because I guess the module will be only instantiated once but it's something one should always keep an eye on it. Imagine if the class was instantiated multilple times. After some (or longer) time your memory would be full because each object allocates a Plasma::DataEngineConsumer but never frees the corresponding memory even if the object itself is destroyed. That's what's called a leak. You have to call delete consumer in the destructor of PotdModule.

There are some tools that help with memory managment std::unique_ptr and QScopePointer wrap a pointer created with new and will delete it if they are destroyed themelves, so you don't forget the delete. std::shared_ptr and QSharedPointer count how many places hold a refrence to it, if noone holds a reference anymore the memory is freed. Finally in Qt there is the QObject(QObject *parent) pattern. A QObject will delete it this children. So if you write new QWidget(this) you don't need to care about freeing the memory occupied of the new widget.

guoyunhe updated this revision to Diff 82993.Sat, May 16, 9:39 AM

Fix leaks

guoyunhe updated this revision to Diff 82994.Sat, May 16, 9:47 AM

Remove unused slots

davidre added inline comments.Sat, May 16, 9:56 AM
kdeds/potd/kded_potd.cpp
26

Don't need to delete watcher because you constructed it with this as parent. https://doc.qt.io/qt-5/objecttrees.html
For the engine don't delete it because you don't have the ownership pof the object. The function just returned a pointer to it.

When the DataEngineConsumer class is deleted, all engines accessed using it are de-referenced and possibly deleted (in the case that there are no other users of the engine in question).

https://api.kde.org/frameworks/plasma-framework/html/classPlasma_1_1DataEngineConsumer.html

guoyunhe updated this revision to Diff 82996.Sat, May 16, 10:26 AM

No delete engine and watcher

@davidre is this better now?

Also @broulik and other Plasma people may want to review.

Finally, you might consider moving this to GitLab for greater visibility.