D29140: Show POTD in lock screen

[Nathaniel Graham]

ngraham added a comment.


  Done and landed! You can close this now.

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe added a comment.


  Also in KDE Invent. 
https://invent.kde.org/plasma/kdeplasma-addons/-/merge_requests/3
  
  Can someone review it?

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Nathaniel Graham]

ngraham added a comment.


  @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.

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe updated this revision to Diff 82996.
guoyunhe added a comment.


  No delete engine and watcher

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29140?vs=82994=82996

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29140

AFFECTED FILES
  CMakeLists.txt
  dataengines/potd/PoTD-list.txt
  dataengines/potd/cachedprovider.cpp
  kdeds/CMakeLists.txt
  kdeds/potd/CMakeLists.txt
  kdeds/potd/README.md
  kdeds/potd/kded_potd.cpp
  kdeds/potd/kded_potd.desktop
  kdeds/potd/kded_potd.h

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kded_potd.cpp:25
> +delete engine;
> +delete watcher;
> +}

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

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe updated this revision to Diff 82994.
guoyunhe added a comment.


  Remove unused slots

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29140?vs=82993=82994

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29140

AFFECTED FILES
  CMakeLists.txt
  dataengines/potd/PoTD-list.txt
  dataengines/potd/cachedprovider.cpp
  kdeds/CMakeLists.txt
  kdeds/potd/CMakeLists.txt
  kdeds/potd/README.md
  kdeds/potd/kded_potd.cpp
  kdeds/potd/kded_potd.desktop
  kdeds/potd/kded_potd.h

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe updated this revision to Diff 82993.
guoyunhe added a comment.


  Fix leaks

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29140?vs=82821=82993

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29140

AFFECTED FILES
  CMakeLists.txt
  dataengines/potd/PoTD-list.txt
  dataengines/potd/cachedprovider.cpp
  kdeds/CMakeLists.txt
  kdeds/potd/CMakeLists.txt
  kdeds/potd/README.md
  kdeds/potd/kded_potd.cpp
  kdeds/potd/kded_potd.desktop
  kdeds/potd/kded_potd.h

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[David Redondo]

davidre added a comment.


  Flickr works here F8325269: Screenshot_20200516_095930.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...
  
  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.

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe added a comment.


  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...

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[David Redondo]

davidre added a comment.


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

INLINE COMMENTS

> kded_potd.cpp:9
> +{
> +consumer = new Plasma::DataEngineConsumer();
> +engine = consumer->dataEngine(QStringLiteral("potd"));

still leaks

> kded_potd.cpp:28
> + */
> +void PotdModule::dataUpdated(const QString& sourceName, const 
> Plasma::DataEngine::Data& data)
> +{

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

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe updated this revision to Diff 82821.
guoyunhe added a comment.


  Code improvement

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29140?vs=82819=82821

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29140

AFFECTED FILES
  CMakeLists.txt
  dataengines/potd/PoTD-list.txt
  dataengines/potd/cachedprovider.cpp
  kdeds/CMakeLists.txt
  kdeds/potd/CMakeLists.txt
  kdeds/potd/README.md
  kdeds/potd/kded_potd.cpp
  kdeds/potd/kded_potd.desktop
  kdeds/potd/kded_potd.h

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe updated this revision to Diff 82819.
guoyunhe marked 9 inline comments as done.
guoyunhe added a comment.


  Fix small issues

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29140?vs=82635=82819

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29140

AFFECTED FILES
  CMakeLists.txt
  dataengines/potd/PoTD-list.txt
  dataengines/potd/cachedprovider.cpp
  kdeds/CMakeLists.txt
  kdeds/potd/CMakeLists.txt
  kdeds/potd/README.md
  kdeds/potd/kded_potd.cpp
  kdeds/potd/kded_potd.desktop
  kdeds/potd/kded_potd.h

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[Yunhe Guo]

guoyunhe marked 9 inline comments as done.
guoyunhe added inline comments.

INLINE COMMENTS

> davidre wrote in CMakeLists.txt:44
> Seems unused?

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

> davidre wrote in CMakeLists.txt:68
> unrelated

Will revert.

> davidre wrote in PoTD-list.txt:8
> Unrelated

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

> davidre wrote in cachedprovider.cpp:54
> Can't we save the image in its original format?

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.

> davidre wrote in kded_potd.cpp:3
> QDebug

Will remove.

> davidre wrote in kded_potd.cpp:10
> unused

Will remove.

> davidre wrote in kded_potd.cpp:12
> You can use K_PLUGIN_CLASS_WITH_JSON

Changed

> davidre wrote in kded_potd.cpp:18
> this leaks

Fixed.

> davidre wrote in kded_potd.cpp:34
> Why don't we care if the data was updated?

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

> davidre wrote in kded_potd.cpp:46
> Why?

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

> davidre wrote in kded_potd.cpp:51
> Couldn't we get the config directly with "kscreenlockerrc" if we use 
> cascading either way? No need to seach for the actual path

I tried but it doesn't work.

> davidre wrote in kded_potd.h:21
> remove

Will do.

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29140: Show POTD in lock screen

[David Redondo]

davidre added a comment.


  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.

INLINE COMMENTS

> CMakeLists.txt:44
>  CoreAddons
> +DBusAddons
>  Declarative

Seems unused?

> CMakeLists.txt:68
>  )
> -
>  add_definitions(

unrelated

> PoTD-list.txt:8
>  where YY is the 2 digits year, MM is the 2 digits month and DD is the 2 
> digits day.
> +Note: sometimes, the webpage shows a YouTube video and picture cannot be 
> fetched.
>  

Unrelated

> cachedprovider.cpp:54
>  const QString path = CachedProvider::identifierToPath( m_identifier );
> -m_image.save(path, "PNG");
> +m_image.save(path, "JPEG");
>  emit done( m_identifier, path, m_image );

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

> kded_potd.cpp:3
> +
> +#include 
> +

QDebug

> kded_potd.cpp:10
> +
> +#define COMPONENT_NAME "potd"
> +

unused

> kded_potd.cpp:12
> +
> +K_PLUGIN_FACTORY_WITH_JSON(PotdModuleFactory,
> +   "kded_potd.json",

You can use K_PLUGIN_CLASS_WITH_JSON

> kded_potd.cpp:18
> +{
> +Plasma::DataEngineConsumer *consumer = new Plasma::DataEngineConsumer();
> +engine = consumer->dataEngine(QStringLiteral("potd"));

this leaks

> kded_potd.cpp:34
> +
> +void PotdModule::dataUpdated(const QString& sourceName, const 
> Plasma::DataEngine::Data& data)
> +{

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

> kded_potd.cpp:46
> +engine->connectSource(previousSource, this);
> +watcher->addPath(configPath); // when recreated, it needs to be added to 
> watcher again
> +}

Why?

> kded_potd.cpp:49
> +
> +QString PotdModule::getSource()
> +{

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

> kded_potd.cpp:51
> +{
> +KConfig config(configPath);
> +KConfigGroup group = config.group(QStringLiteral("Greeter"))

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

> kded_potd.h:21
> +
> +public Q_SLOTS:
> +

remove

REPOSITORY
  R114 Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D29140

To: guoyunhe, #plasma, davidre, broulik
Cc: davidre, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart