D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 38971.
abhijeet2096 edited the summary of this revision.
abhijeet2096 added a comment.


  Modified the Patch for latest Master branch Integration

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=38966&id=38971

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindLIBFWUPD.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 38966.
abhijeet2096 added a comment.


  - Removed Make Default action from the sources backend actions list
  - Removed m_actions vector
  - Renamed the iterateTransaction to updateProgress in transaction Class
  - Now Using Proper connect Syntax
  - Fixed Spaces Between Commas

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=38655&id=38966

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindLIBFWUPD.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Aleix Pol Gonzalez]

apol added a comment.


  Please, do some review yourself, you should be able to see most of these 
things yourself

INLINE COMMENTS

> FwupdSourcesBackend.cpp:151
> +{
> +return  m_actions ;
> +}

`return {}` and remove the unused attribute.

> FwupdSourcesBackend.h:46
> +bool supportsAdding() const override { return false; }
> +void eulaRequired(const QString& remoteName , const QString& 
> licenseAgreement);
> +void populateSources();

There's no space before a coma.

> FwupdSourcesBackend.h:54
> +FwupdSourcesModel* m_sources;
> +QList m_actions;
> +};

Remove.

> FwupdTransaction.cpp:99
> +QNetworkAccessManager *manager = new QNetworkAccessManager(this);
> +connect(manager, SIGNAL(finished(QNetworkReply*)),this, 
> SLOT(fwupdInstall(QNetworkReply*)));
> +manager->get(QNetworkRequest(uri));

Use proper connect syntax.

> FwupdTransaction.cpp:160
> +
> +void FwupdTransaction::iterateTransaction()
> +{

Give it a name that says what it does, this is just copied from the dummy and 
thus it has a meaningless name.

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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 38655.
abhijeet2096 added a comment.


  - Removed The FindGIO.cmake
  - Added GObject Dependency
  - Removed Not Used Gcancellable Object

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=38443&id=38655

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindLIBFWUPD.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> abhijeet2096 wrote in FwupdBackend.h:48
> I need it for g_ptr_array_index, g_cancellable_new and similar functions

No, definitely not, this is part of glib, not gio.

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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 38443.
abhijeet2096 added a comment.


  - Removed the testProceed macro

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=38440&id=38443

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGIO.cmake
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindLIBFWUPD.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 marked 10 inline comments as done.
abhijeet2096 added inline comments.

INLINE COMMENTS

> apol wrote in FwupdBackend.h:48
> gio doesn't seem necessary anymore?

I need it for g_ptr_array_index, g_cancellable_new and similar functions

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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 38440.
abhijeet2096 marked an inline comment as done.
abhijeet2096 added a comment.


  - Removed FindSoup.cmake,FindGObjectIntrospection.cmake
  - Removed FwupdTest
  - using nullptr instead of NULL
  - Now using file path instead of file
  - Now using QVector instead of QList for storing the reference of releases
  - Now using QString::fromUtf8 to convert to QString from gchar*
  - Tried New download for downloading firmware files during installation
  - Refactored SourceBackend code

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=38183&id=38440

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGIO.cmake
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindLIBFWUPD.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

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 doesn't seem necessary anymore?

> FwupdResource.h:114
> +
> +QList 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 actions() const override;

Maybe pass the repostory url as description?

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

> FwupdTransaction.cpp:26
> +#include 
> +#include 
> +

?

> 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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 marked 7 inline comments as done.
abhijeet2096 added inline comments.

INLINE COMMENTS

> apol wrote in FwupdBackend.cpp:395
> Are you sure you need an event-loop there? Let's make this non-blocking.

Can you please guide me, on how can I make this non-blocking?

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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 38183.
abhijeet2096 added a comment.


  - Removed the derived FwupdUpdater
  - Using Standard Backend Updater
  - Now using the standard to name functions
  - Added Status on transactions [Not Tested]
  - Now Apps Cannot be executed/ Launched [Not tested]
  - Fixed Intendation and coding style at some places
  - Removed Ifdef in FwupdBackend.cpp
  - Removed FwupdNotifier
  
  I have still not changed the download function, need help how can I make it 
non-blocking.

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=37778&id=38183

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGIO.cmake
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindGObjectIntrospection.cmake
  cmake/FindLIBFWUPD.cmake
  cmake/FindSoup.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/backends/FwupdBackend/tests/CMakeLists.txt
  libdiscover/backends/FwupdBackend/tests/FwupdTest.cpp
  libdiscover/backends/FwupdBackend/tests/FwupdTest.h
  libdiscover/backends/FwupdBackend/tests/UpdateFwupdTest.cpp
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 added a comment.


  Hey Aleix, I have fixed things, I am reviewing whole code once again, I will 
upload the new diff by tomorrow morning.

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

D14050: Fwupd Backend For Review and Improvement

[Aleix Pol Gonzalez]

apol added a comment.


  Ping?

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

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

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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 marked 18 inline comments as done.
abhijeet2096 added a comment.


  Still, Need to implement Notification Class and some functions in Updater 
Class. I have to see if we could use the standard Updater Class, after 
Confirmation I will use directly FwupdResource instead of abstract resource

INLINE COMMENTS

> apol wrote in SourcesPage.qml:113
> Remove unrelated patch

Actually, I modified this file to show EULA on setting page.

> anthonyfieroni wrote in FwupdBackend.cpp:85
> It's memory leak.

Please see the new modified function. Will Memory be leaking?

> apol wrote in FwupdBackend.cpp:100
> No need to cast. Use the FwupdResource directly.

Still Need To Fix it

> apol wrote in FwupdNotifier.cpp:37
> Don't add one that does nothing.
> Either implement it or just don't add it.

Need to Implement it, Like Updates which require Restart. Will Implement it.

> anthonyfieroni wrote in FwupdSourcesBackend.cpp:75-77
> ?

Here, The UI gets Updates first. The CheckBox fills first then its applied at 
the backend. Need a way to control the checkbox UI

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

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 37778.
abhijeet2096 added a comment.


  Following Updates are made:
  
  - Removed Unnecessary Dependencies
  - Now All strings are constructed by QString
  - Regularized the indentation and coding style
  - Removed the soup headers, Now QT libs are used to Download files
  - QT based Hash Function is used to calculate Hash
  - Removed the Category File
  - Removed The Review Backend Completely
  - Added Dependencies of soup, glib and GIO inside FINDLIBFWUPD.cmake
  - QUrl and QFile is Now Used
  - Added Checks at Various Places

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=37580&id=37778

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGIO.cmake
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindGObjectIntrospection.cmake
  cmake/FindLIBFWUPD.cmake
  cmake/FindSoup.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdNotifier.cpp
  libdiscover/backends/FwupdBackend/FwupdNotifier.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/backends/FwupdBackend/FwupdUpdater.cpp
  libdiscover/backends/FwupdBackend/FwupdUpdater.h
  libdiscover/backends/FwupdBackend/tests/CMakeLists.txt
  libdiscover/backends/FwupdBackend/tests/FwupdTest.cpp
  libdiscover/backends/FwupdBackend/tests/FwupdTest.h
  libdiscover/backends/FwupdBackend/tests/UpdateFwupdTest.cpp
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: zzag, anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 added a comment.


  I have Fixed some, I will fix rest and upload new diff as soon as possible.

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

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

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

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

D14050: Fwupd Backend For Review and Improvement

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> FwupdBackend.cpp:272-289
> +FwupdDevice *device = (FwupdDevice *)g_ptr_array_index (devices, 
> i);
> +FwupdResource* res;
> +
> +   res = FwupdCreateDevice(device); //just to test code should be 
> deleted
> +   m_toUpdate.append(res); //just to test code should be deleted
> +
> +if (!fwupd_device_has_flag (device, FWUPD_DEVICE_FLAG_SUPPORTED))

Coding style nitpick: indentation is not consistent. At all.

> FwupdBackend.cpp:294-308
> +if (rels == NULL) {
> +if (g_error_matches 
> (error2,FWUPD_ERROR,FWUPD_ERROR_NOTHING_TO_DO)){
> +qWarning() << "No Packages Found for "<< 
> fwupd_device_get_id(device);
> +FwupdHandleError(&error2);
> +continue;
> +}
> +}

Coding style nitpick: why does this small piece of code follows several totally 
different coding styles?

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

D14050: Fwupd Backend For Review and Improvement

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> CMakeLists.txt:16
> +
> +#find_package(GIO)
> +find_package(Soup)

Why you comment it when you use it, see below.

> FwupdBackend.cpp:68
> +if (!m_fetching)
> +   m_reviews->initialize();
> +populate(QStringLiteral("Releases"));

Indentations should be 4 spaces, if statement goes with { on same line and } 
even on one line  body.

> FwupdBackend.cpp:85
> +g_strdelimit (tmp, "/", '_');
> +return g_strdup_printf ("org.fwupd.%s.device", tmp);
> +}

It's memory leak.

> FwupdSourcesBackend.cpp:75-77
> +else if(value.toInt() == Qt::Unchecked)
> +{
> +if((value.toInt() == Qt::Checked) )

?

REPOSITORY
  R134 Discover Software Store

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

To: abhijeet2096, apol, davidedmundson
Cc: anthonyfieroni, plasma-devel, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 updated this revision to Diff 37580.
abhijeet2096 added a comment.


  I rebased the patch

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14050?vs=37576&id=37580

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGIO.cmake
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindGObjectIntrospection.cmake
  cmake/FindLIBFWUPD.cmake
  cmake/FindSoup.cmake
  discover/qml/SourcesPage.qml
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdNotifier.cpp
  libdiscover/backends/FwupdBackend/FwupdNotifier.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdReviewsBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdReviewsBackend.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/backends/FwupdBackend/FwupdUpdater.cpp
  libdiscover/backends/FwupdBackend/FwupdUpdater.h
  libdiscover/backends/FwupdBackend/fwupd-backend-categories.xml
  libdiscover/backends/FwupdBackend/tests/CMakeLists.txt
  libdiscover/backends/FwupdBackend/tests/FwupdTest.cpp
  libdiscover/backends/FwupdBackend/tests/FwupdTest.h
  libdiscover/backends/FwupdBackend/tests/UpdateFwupdTest.cpp
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: plasma-devel, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Aleix Pol Gonzalez]

apol requested changes to this revision.
apol added a comment.
This revision now requires changes to proceed.


  This has a ton of unrelated changes. Please rebase your branch before sending 
the patch.

REPOSITORY
  R134 Discover Software Store

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

To: abhijeet2096, apol, davidedmundson
Cc: plasma-devel, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14050: Fwupd Backend For Review and Improvement

[Abhijeet sharma]

abhijeet2096 created this revision.
abhijeet2096 added reviewers: apol, davidedmundson.
abhijeet2096 added a project: Discover Software Store.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
abhijeet2096 requested review of this revision.

REVISION SUMMARY
  This Diff is taken from the current master branch and fwupd-backend for 
review and improvement.

REPOSITORY
  R134 Discover Software Store

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindGIO.cmake
  cmake/FindGLib.cmake
  cmake/FindGObject.cmake
  cmake/FindGObjectIntrospection.cmake
  cmake/FindLIBFWUPD.cmake
  cmake/FindSoup.cmake
  discover/DiscoverObject.cpp
  discover/qml/ApplicationPage.qml
  discover/qml/ApplicationsListPage.qml
  discover/qml/BrowsingPage.qml
  discover/qml/DiscoverPopup.qml
  discover/qml/DiscoverWindow.qml
  discover/qml/SourcesPage.qml
  discover/resources.qrc
  exporter/main.cpp
  libdiscover/backends/CMakeLists.txt
  libdiscover/backends/FlatpakBackend/org.kde.discover.flatpak.appdata.xml
  libdiscover/backends/FwupdBackend/CMakeLists.txt
  libdiscover/backends/FwupdBackend/FwupdBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdBackend.h
  libdiscover/backends/FwupdBackend/FwupdNotifier.cpp
  libdiscover/backends/FwupdBackend/FwupdNotifier.h
  libdiscover/backends/FwupdBackend/FwupdResource.cpp
  libdiscover/backends/FwupdBackend/FwupdResource.h
  libdiscover/backends/FwupdBackend/FwupdReviewsBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdReviewsBackend.h
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
  libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
  libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
  libdiscover/backends/FwupdBackend/FwupdTransaction.h
  libdiscover/backends/FwupdBackend/FwupdUpdater.cpp
  libdiscover/backends/FwupdBackend/FwupdUpdater.h
  libdiscover/backends/FwupdBackend/fwupd-backend-categories.xml
  libdiscover/backends/FwupdBackend/tests/CMakeLists.txt
  libdiscover/backends/FwupdBackend/tests/FwupdTest.cpp
  libdiscover/backends/FwupdBackend/tests/FwupdTest.h
  libdiscover/backends/FwupdBackend/tests/UpdateFwupdTest.cpp
  libdiscover/backends/PackageKitBackend/CMakeLists.txt
  libdiscover/backends/PackageKitBackend/PackageKitResource.cpp
  libdiscover/backends/PackageKitBackend/PackageKitResource.h
  libdiscover/backends/PackageKitBackend/PackageKitUpdater.cpp
  libdiscover/backends/PackageKitBackend/PackageKitUpdater.h
  libdiscover/backends/PackageKitBackend/org.kde.discover.packagekit.appdata.xml
  libdiscover/backends/PackageKitBackend/pkui.qrc
  libdiscover/backends/PackageKitBackend/qml/DependenciesButton.qml
  libdiscover/backends/SnapBackend/SnapResource.cpp
  libdiscover/backends/SnapBackend/SnapResource.h
  libdiscover/backends/SnapBackend/org.kde.discover.snap.appdata.xml
  libdiscover/backends/SnapBackend/qml/PermissionsButton.qml
  libdiscover/backends/SnapBackend/snapui.qrc
  libdiscover/backends/SnapBackend/snapui/PermissionsButton.qml
  libdiscover/resources/AbstractSourcesBackend.h

To: abhijeet2096, apol, davidedmundson
Cc: plasma-devel, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart