Add Team Drive model and basic fetch.
ClosedPublic

Authored by barchiesi on Feb 27 2019, 10:58 PM.

Details

Summary

Add Team Drive model, basic fetch (no File search query string) and fetch test.
Add Team Drive usage example that allows listing Team Drives and their contents (very crude). Uses teamdrivefetchjob and filefetchjob.

Test Plan

See provided example in examples/teamdrive.

Diff Detail

Repository
R477 KGAPI Library
Lint
Lint Skipped
Unit
Unit Tests Skipped
barchiesi created this revision.Feb 27 2019, 10:58 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptFeb 27 2019, 10:58 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
barchiesi requested review of this revision.Feb 27 2019, 10:58 PM
barchiesi added a reviewer: dvratil.
dvratil requested changes to this revision.Feb 28 2019, 10:49 AM

Nice, thanks for the patch!

I haven't checked the code in-depth (I don't know if I'll get to it before the weekend, sorry), but one thing that I noticed: please update year and your name in the copyright headers :)

Second thing: I realized when I was fixing a bug in kgapi recently that having all the properties as strings everywhere (e.g. QStringLiteral("fooPropertyName") is annoying) - it's hard to read and easy to make a typo in. Could you please define all the strings that are used as JSON property names and URL queries as named constants in an anonymous namespace at the top of the file, e.g. for TeamDrive:

namespace {
static const QString tdId = QStringLiteral("id");
static const QString tdName = QStringLiteral("name");
...
}

Same for URL query parameters in jobs, e.g. static const QString queryMaxResults = QStringLiteral("maxResults");

I will eventually get around to change it everywhere to make the code safer. It will also reduce the size of the binaries since the strings won't be repeated twice there.

Thanks!

This revision now requires changes to proceed.Feb 28 2019, 10:49 AM
barchiesi updated this revision to Diff 52869.Feb 28 2019, 11:39 PM
barchiesi edited the test plan for this revision. (Show Details)

Added constants for attributes. What is the correct naming?
Fixed copyright names.

dvratil requested changes to this revision.Mar 3 2019, 3:15 PM

Nice job!

There's a bunch of places where you can simplify the code by using C++11 default initialization and defaulted constructors and destructors, as well as QScopedPointer to own the d-pointers everywhere. I realize we don't use them elsewhere, but this is probably the first big contribution since we switched to C++11, so let's set an example for any future contributions :) I'll eventually get around and clean up the rest of the code as well.

Once you fix those, it's good to go.

src/drive/teamdrive.cpp
80

Since the ctor here does not do anything special, you can just use Private() = default; to let the compiler to generate default implementation of the constructor (and you don't have to define it yourself later on)

81

Same for the copy ctor, you can just use Private(const Private &other) = default and the compiler will generate the copy ctor for you, so you can remove the custom implementation.

83

Initialize all the members to false, please.

111

While we don't need to delete the d pointer ourselves when using QScopedPointer, the dtor still has to be declared here, but you can use Teamdrive::Restrictions::~Restrictions() = default; to let the compiler generate a default dtor.

150

Private() = default;

151

Private(const Private &) = default;

153

Default-initialize all the members to false.

209

Teamdrive::Capabilities::~Capabilities() = default;

332

= default

333

= default

336

qreal is defined as a double by default and according to docs, those a float numbers, so let's just make those floats, and default-initialize them to 0.0f

363

= default

402

= default

403

= default

443

auto teamdrive = TeamdrivePtr::create()

The idea when using smart pointers is to not have any new in your code :)

452

BackgroundImageFilePtr::create() instead of new

460

CapabilitiesPtr::create() instead of new

482

RestrictionsPtr::create() instead of new

504

= default

src/drive/teamdrive.h
61

Copy constructors are usually not explicit.

62

Doesn't need to be virtual, we have no virtual methods and this is likely a final class.

98

use QScopedPointer<Private> const d to have the d-pointer deleted automatically when the parent object is destroyed.

Note that because Private is only forward-declared here, you still need to have the destructor for this class defined in the .cpp file, because that's where the compiler will generate destructor code for the QScopedPointer as well (and where Private is already defined).

112

Remove explicit

113

Remove virtual

229

QScopedPointer<Private>

244

Remove explicit

245

Remove virtual

273

QScopedPointer<Private>

281

Remove explicit.

338

QScopedPointer<Private>

src/drive/teamdrivefetchjob.cpp
52

Default-Initialize to 0 here instead of the ctor please.

53

Default-initialize to false here instead of the ctor please.

56

TeamdriveFetchJob * const q

91

= default

src/drive/teamdrivefetchjob.h
85

QScopedPointer

This revision now requires changes to proceed.Mar 3 2019, 3:15 PM
barchiesi updated this revision to Diff 53125.Mar 4 2019, 1:58 PM

Use C++11 default constructors and QScopedPointer. I should have fixed all the points, let me know if I missed something.

dvratil requested changes to this revision.Mar 4 2019, 5:26 PM

Awesome, thanks a lot for fixing it! You missed few comments in the TeamdriveFetchJob, please fix those too.

src/drive/teamdrivefetchjob.cpp
52

Initialize to 0 here instead of in ctor, same for useDomainAdminAccess below.

91

= default

src/drive/teamdrivefetchjob.h
85

QScopedPointer

This revision now requires changes to proceed.Mar 4 2019, 5:26 PM
barchiesi updated this revision to Diff 53148.Mar 4 2019, 6:44 PM

Embarrassing

dvratil accepted this revision.Mar 5 2019, 8:56 AM

Thanks!

This revision is now accepted and ready to land.Mar 5 2019, 8:56 AM
This revision was automatically updated to reflect the committed changes.