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

> SourcesPage.qml:113
>                                  }
> +
>                                  onObjectAdded: {

Remove unrelated patch

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


See warning

> CMakeLists.txt:29
> +add_library(fwupd-backend MODULE ${fwupd-backend_SRCS})
> +target_link_libraries(fwupd-backend Qt5::Core Qt5::Widgets KF5::CoreAddons KF5::ConfigCore Discover::Common LIBFWUPD  ${Soup} ${GIO} ${GLib} )
> +

Soup, GIO and GLib are dependencies in LIBFWUPD, no? if so do the search and link in FindFWUPD.cmake.

> FwupdBackend.cpp:100
> +    {
> +        AbstractResource* res = (AbstractResource*) r;
> +        if(r->m_id.isEmpty())

No need to cast. Use the FwupdResource directly.

> FwupdBackend.cpp:167
> +    if (fwupd_device_has_flag (dev, FWUPD_DEVICE_FLAG_UPDATABLE))
> +        res->isLiveUpdatable = true;
> +    if (fwupd_device_has_flag (dev, FWUPD_DEVICE_FLAG_ONLY_OFFLINE))

` res->isLiveUpdatable = fwupd_device_has_flag (dev, FWUPD_DEVICE_FLAG_UPDATABLE)`
And same with the rest.

> FwupdBackend.cpp:193
> +        {
> +            vendor_name = g_strdup_printf ("%s %s",fwupd_device_get_vendor (dev), fwupd_device_get_name (dev));
> +        }

Use QString, not g_* to construct strings.

> FwupdBackend.cpp:209
> +{
> +    g_autoptr(GPtrArray) devices = NULL;
> +

Just initialize at once. No need to set NULL first.

> FwupdBackend.cpp:219
> +            FwupdDevice *device = (FwupdDevice *)g_ptr_array_index (devices, i);
> +            FwupdResource * res = NULL;
> +            g_autoptr(GPtrArray) releases = NULL;

Same

> FwupdBackend.cpp:233
> +
> +            if (releases != NULL)
> +            {

`if (releases)`

> FwupdBackend.cpp:235
> +            {
> +                for (int j = 0; j < (int)releases->len; j++)
> +                {

Use uint rather than casting

> FwupdBackend.cpp:261
> +    /* get All devices, Will filter latter */
> +    devices = fwupd_client_get_devices (client, cancellable, &error);
> +    if(devices == NULL){

Don't initialize twice.

> FwupdBackend.cpp:262
> +    devices = fwupd_client_get_devices (client, cancellable, &error);
> +    if(devices == NULL){
> +        if (g_error_matches (error,FWUPD_ERROR,FWUPD_ERROR_NOTHING_TO_DO)){

if (devices)

> FwupdBackend.cpp:269
> +        }
> +    }
> +    else{

} else {

> FwupdBackend.cpp:464
> +
> +    if (!g_file_set_contents (filename,msg->response_body->data,msg->response_body->length,&error_local)) 
> +    {

Use QFile

> FwupdBackend.cpp:482
> +    /* local */
> +    if (g_str_has_prefix (uri, "file://")) {
> +        gsize length = 0;

QUrl

> FwupdBackend.h:35
> +#include <gio-unix-2.0/gio/gunixfdlist.h>
> +#include <glib/gi18n.h>
> +#include <glib/gstdio.h>

We use the i18n coming from KLocalizedString

> FwupdNotifier.cpp:37
> +{
> +    emit foundUpdates();
> +}

Don't add one that does nothing.
Either implement it or just don't add it.

> FwupdResource.cpp:153
> +{
> +    m_state = state;
> +    emit stateChanged();

Check that it's different before setting/emitting.

> FwupdReviewsBackend.cpp:30
> +
> +FwupdReviewsBackend::FwupdReviewsBackend(FwupdBackend* parent)
> +    : AbstractReviewsBackend(parent)

This is all copied from the dummy backend and doesn't apply. Remove.

> FwupdUpdater.cpp:76
> +
> +quint64 FwupdUpdater::downloadSpeed() const
> +{

Would it make sense to use the StandardUpdater?

> fwupd-backend-categories.xml:9
> +    <Include>
> +        <And>
> +            <Category>Firmware Updates</Category>

I wouldn't include the category file for now.

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/20180711/cd81d133/attachment-0001.html>