Fwupd-Backend Integration
ClosedPublic

Authored by abhijeet2096 on Jul 11 2018, 3:40 PM.

Details

Summary

This patch brings the initial integration of Fwupd Backend.

Diff Detail

Repository
R134 Discover Software Store
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
zzag added inline comments.Jul 11 2018, 7:51 PM
libdiscover/backends/FwupdBackend/FwupdBackend.cpp
273–290

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

295–309

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

apol requested changes to this revision.Jul 11 2018, 8:43 PM
apol added inline comments.
discover/qml/SourcesPage.qml
113

Remove unrelated patch

libdiscover/backends/CMakeLists.txt
47

See warning

libdiscover/backends/FwupdBackend/CMakeLists.txt
30

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

libdiscover/backends/FwupdBackend/FwupdBackend.cpp
101

No need to cast. Use the FwupdResource directly.

168

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

194

Use QString, not g_* to construct strings.

210

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

220

Same

234

if (releases)

236

Use uint rather than casting

262

Don't initialize twice.

263

if (devices)

270

} else {

465

Use QFile

483

QUrl

libdiscover/backends/FwupdBackend/FwupdBackend.h
36

We use the i18n coming from KLocalizedString

libdiscover/backends/FwupdBackend/FwupdNotifier.cpp
37 ↗(On Diff #37580)

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

libdiscover/backends/FwupdBackend/FwupdResource.cpp
154

Check that it's different before setting/emitting.

libdiscover/backends/FwupdBackend/FwupdReviewsBackend.cpp
30 ↗(On Diff #37580)

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

libdiscover/backends/FwupdBackend/FwupdUpdater.cpp
76 ↗(On Diff #37580)

Would it make sense to use the StandardUpdater?

libdiscover/backends/FwupdBackend/fwupd-backend-categories.xml
9 ↗(On Diff #37580)

I wouldn't include the category file for now.

This revision now requires changes to proceed.Jul 11 2018, 8:43 PM

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

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
abhijeet2096 marked 18 inline comments as done.Jul 15 2018, 3:16 AM

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

discover/qml/SourcesPage.qml
113

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

libdiscover/backends/FwupdBackend/FwupdBackend.cpp
86

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

101

Still Need To Fix it

libdiscover/backends/FwupdBackend/FwupdNotifier.cpp
37 ↗(On Diff #37580)

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

libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
76–78

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

apol requested changes to this revision.Jul 16 2018, 12:26 PM
apol added inline comments.
libdiscover/backends/FwupdBackend/FwupdBackend.cpp
94

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

164

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

224

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

242

Remove ifdef, fix indentation

396

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

756

return !m_resources.isEmpty()

libdiscover/backends/FwupdBackend/FwupdBackend.h
84

methods should always start with lower-case.

libdiscover/backends/FwupdBackend/FwupdNotifier.cpp
2 ↗(On Diff #37778)

Please remove fwupd notifier plugin altogether.

libdiscover/backends/FwupdBackend/FwupdResource.cpp
189

?

libdiscover/backends/FwupdBackend/FwupdTransaction.h
39

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

This revision now requires changes to proceed.Jul 16 2018, 12:26 PM

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

  • 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.

abhijeet2096 marked 7 inline comments as done.Sat, Jul 21, 5:20 PM
abhijeet2096 added inline comments.
libdiscover/backends/FwupdBackend/FwupdBackend.cpp
396

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

apol added a comment.Mon, Jul 23, 2:01 AM

Looking much much better, please see new comments.

cmake/FindSoup.cmake
1 ↗(On Diff #38183)

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

libdiscover/backends/CMakeLists.txt
47

Please add

libdiscover/backends/FwupdBackend/FwupdBackend.cpp
86

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

329

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

397

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

Maybe just keep the path?

412

else { qWarning() ...

658

Use nullptr everywhere you use NULL.

libdiscover/backends/FwupdBackend/FwupdBackend.h
49

gio doesn't seem necessary anymore?

libdiscover/backends/FwupdBackend/FwupdResource.h
115

Use QVector

libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
153

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

libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
44

Maybe pass the repostory url as description?

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

libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
27

?

libdiscover/backends/FwupdBackend/tests/FwupdTest.cpp
148 ↗(On Diff #38183)

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

abhijeet2096 marked an inline comment as done.
  • 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
abhijeet2096 marked 10 inline comments as done.Wed, Jul 25, 4:11 PM
abhijeet2096 added inline comments.
libdiscover/backends/FwupdBackend/FwupdBackend.h
49

I need it for g_ptr_array_index, g_cancellable_new and similar functions

  • Removed the testProceed macro
abhijeet2096 marked an inline comment as done.Wed, Jul 25, 4:20 PM
apol added inline comments.Fri, Jul 27, 1:17 PM
libdiscover/backends/FwupdBackend/FwupdBackend.h
49

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

  • Removed The FindGIO.cmake
  • Added GObject Dependency
  • Removed Not Used Gcancellable Object
apol added a comment.Sun, Jul 29, 11:54 PM

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

libdiscover/backends/FwupdBackend/FwupdSourcesBackend.cpp
152

return {} and remove the unused attribute.

libdiscover/backends/FwupdBackend/FwupdSourcesBackend.h
47

There's no space before a coma.

55

Remove.

libdiscover/backends/FwupdBackend/FwupdTransaction.cpp
100

Use proper connect syntax.

161

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

  • 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
abhijeet2096 marked 10 inline comments as done.Thu, Aug 2, 4:49 PM
apol accepted this revision.Thu, Aug 2, 5:13 PM
This revision is now accepted and ready to land.Thu, Aug 2, 5:13 PM
abhijeet2096 edited the summary of this revision. (Show Details)

Modified the Patch for latest Master branch Integration

abhijeet2096 retitled this revision from Fwupd Backend For Review and Improvement to Fwupd-Backend Integration.Thu, Aug 2, 7:17 PM
abhijeet2096 edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.