D14050: Fwupd Backend For Review and Improvement

[Aleix Pol Gonzalez]

apol requested changes to this revision.
apol added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> FwupdBackend.cpp:93
> +        AbstractResource* res = (AbstractResource*) r;
> +        if(r->m_id.isEmpty())
> +            qDebug() << "Resource ID is Empty" << r->m_name;

Does it still make sense to add it if it doesn't have an id?
In which cases wouldn't it have an id?

> FwupdBackend.cpp:163
> +    {
> +        QString guidStr = QLatin1Literal((char *)g_ptr_array_index (guids, 0));
> +        for (uint i = 1; i < guids->len; i++)

This is not a literal. Use QString::fromLatin1 (or fromUtf8, it should be Utf8).

> FwupdBackend.cpp:223
> +            /* add all Valid Resources */
> +            m_resources.insert(res->name().toLower(), res);
> +          }

Usually name is a user-visible string. Would it make more sense to use packageName?
This way you could also skip the toLower part.

> FwupdBackend.cpp:241
> +        {
> +                #ifdef FWUPD_DEBUG
> +                    qDebug() << "No Devices Found";

Remove ifdef, fix indentation

> FwupdBackend.cpp:395
> +{
> +    QEventLoop loop;
> +    QTimer getTimer;

Are you sure you need an event-loop there? Let's make this non-blocking.

> FwupdBackend.cpp:755
> +{
> +    return m_resources.count() ? true : false;
> +}

return !m_resources.isEmpty()

> FwupdBackend.h:83
> +    
> +    bool FwupdDownloadFile(const QUrl &uri,const QString &filename);
> +    bool FwupdRefreshRemotes(uint cacheAge);

methods should always start with lower-case.

> FwupdNotifier.cpp:2
> +/***************************************************************************
> + *   Copyright © 2013 Lukas Appelhans <l.appelhans at gmx.de>                 *
> + *                                                                         *

Please remove fwupd notifier plugin altogether.

> FwupdResource.cpp:188
> +    QDesktopServices d;
> +    d.openUrl(QUrl(QStringLiteral("https://projects.kde.org/projects/extragear/sysadmin/muon")));
> +}

?

> FwupdTransaction.h:38
> +        ~FwupdTransaction();
> +        bool FwupdCheck();
> +        bool FwupdInstall();

methods should always start lower-case, classes upperc-ase.

REPOSITORY
  R134 Discover Software Store

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

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180716/cf9a89b5/attachment-0001.html>