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

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200514/fdefb5f3/attachment-0001.htm>