D14050: Fwupd Backend For Review and Improvement

[Aleix Pol Gonzalez]

apol added a comment.


  Looking much much better, please see new comments.

INLINE COMMENTS

> FindSoup.cmake:1
> +# FindSoup.cmake
> +# <https://github.com/nemequ/gnome-cmake>

Are you sure this file is till necessary? Where is libsoup used?

> CMakeLists.txt:47
> +endif()
> \ No newline at end of file


Please add

> FwupdBackend.cpp:85
> +{
> +    const QString name = QLatin1String(fwupd_device_get_name(device));
> +    FwupdResource* res = new FwupdResource(name, true, this);

When getting a string from glib, use QString::fromUtf8, which is what I guess they use.

> FwupdBackend.cpp:328
> +    GPtrArray *checksums;
> +    FwupdResource* app = NULL;
> +

Don't set to null only to initialize at the next line.

> FwupdBackend.cpp:396
> +    QFile file(filename_cache.toString());
> +    app->m_file = &file;
> +    // To Do Download for a file?

&file is in this scope, it will crash as soon as you call m_file outside this scope.

Maybe just keep the path?

> FwupdBackend.cpp:411
> +            file.write(Data);
> +        }
> +        file.close();

else { qWarning() ...

> FwupdBackend.cpp:657
> +{
> +    return NULL;
> +}

Use nullptr everywhere you use NULL.

> FwupdBackend.h:48
> +}
> +#include <gio/gio.h>
> +

gio doesn't seem necessary anymore?

> FwupdResource.h:114
> +
> +    QList<FwupdResource*> m_releases; // A list of all refrences to releases of a device.
> +};

Use QVector

> FwupdSourcesBackend.cpp:152
> +{
> +    qWarning() << "Removal of Sources Not Allowed" << "Remote-ID" << id;
> +    return false;

How come it's possible to add but not to remove? Is it just a TODO?

> FwupdSourcesBackend.h:43
> +    bool removeSource(const QString& id) override;
> +    QString idDescription() override { return QStringLiteral(""); }
> +    QList<QAction*> actions() const override;

Maybe pass the repostory url as description?

Also to return qn empty string use either {} or QString().

> FwupdTransaction.cpp:26
> +#include <QDebug>
> +#include <KRandom>
> +

?

> FwupdTest.cpp:148
> +    const auto resources = fetchResources(m_appBackend->search({}));
> +    QCOMPARE(m_appBackend->property("startElements").toInt()*2, resources.count());
> +

This is just copied from the dummy test, it doesn't make sense here...

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/20180723/ac8354e5/attachment-0001.html>