D19403: Add Team Drive model and basic fetch.

[Daniel Vrátil]

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


  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.

INLINE COMMENTS

> teamdrive.cpp:79
> +  public:
> +    Private();
> +    Private(const Private &other);

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)

> teamdrive.cpp:80
> +    Private();
> +    Private(const Private &other);
> +

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.

> teamdrive.cpp:82
> +
> +    bool adminManagedRestrictions;
> +    bool copyRequiresWriterPermission;

Initialize all the members to `false`, please.

> teamdrive.cpp:110
> +
> +Teamdrive::Restrictions::~Restrictions()
> +{

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.

> teamdrive.cpp:149
> +  public:
> +    Private();
> +    Private(const Private &other);

`Private() = default;`

> teamdrive.cpp:150
> +    Private();
> +    Private(const Private &other);
> +

`Private(const Private &) = default;`

> teamdrive.cpp:152
> +
> +    bool canAddChildren;
> +    bool canChangeCopyRequiresWriterPermissionRestriction;

Default-initialize all the members to `false`.

> teamdrive.cpp:208
> +
> +Teamdrive::Capabilities::~Capabilities()
> +{

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

> teamdrive.cpp:331
> +  public:
> +    Private();
> +    Private(const Private &other);

` = default`

> teamdrive.cpp:332
> +    Private();
> +    Private(const Private &other);
> +

` = default`

> teamdrive.cpp:335
> +    QString id;
> +    qreal xCoordinate;
> +    qreal yCoordinate;

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

> teamdrive.cpp:362
> +
> +Teamdrive::BackgroundImageFile::~BackgroundImageFile()
> +{

` = default`

> teamdrive.cpp:401
> +  public:
> +    Private();
> +    Private(const Private& other);

` = default`

> teamdrive.cpp:402
> +    Private();
> +    Private(const Private& other);
> +

` = default`

> teamdrive.cpp:442
> +
> +    TeamdrivePtr teamdrive(new Teamdrive());
> +    teamdrive->d->id = map[IdAttr].toString();

`auto teamdrive = TeamdrivePtr::create()`

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

> teamdrive.cpp:451
> +    const QVariantMap backgroundImageFileMap = map[BackgroundImageFileAttr].toMap();
> +    BackgroundImageFilePtr backgroundImageFile(new BackgroundImageFile());
> +    backgroundImageFile->d->id = backgroundImageFileMap[IdAttr].toString();

`BackgroundImageFilePtr::create()` instead of `new`

> teamdrive.cpp:459
> +    const QVariantMap capabilitiesMap = map[CapabilitiesAttr].toMap();
> +    CapabilitiesPtr capabilities(new Capabilities());
> +    capabilities->d->canAddChildren = capabilitiesMap[CanAddChildrenAttr].toBool();

`CapabilitiesPtr::create()` instead of `new`

> teamdrive.cpp:481
> +    const QVariantMap restrictionsMap = map[RestrictionsAttr].toMap();
> +    RestrictionsPtr restrictions(new Restrictions());
> +    restrictions->d->adminManagedRestrictions = restrictionsMap[AdminManagedRestrictionsAttr].toBool();

`RestrictionsPtr::create()` instead of `new`

> teamdrive.cpp:503
> +
> +Teamdrive::~Teamdrive()
> +{

` = default`

> teamdrive.h:60
> +      public:
> +        explicit Restrictions(const Restrictions &other);
> +        virtual ~Restrictions();

Copy constructors are usually not `explicit`.

> teamdrive.h:61
> +        explicit Restrictions(const Restrictions &other);
> +        virtual ~Restrictions();
> +        bool operator==(const Restrictions &other) const;

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

> teamdrive.h:97
> +        class Private;
> +        Private *const d;
> +        friend class Private;

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

> teamdrive.h:111
> +      public:
> +        explicit Capabilities(const Capabilities &other);
> +        virtual ~Capabilities();

Remove `explicit`

> teamdrive.h:112
> +        explicit Capabilities(const Capabilities &other);
> +        virtual ~Capabilities();
> +        bool operator==(const Capabilities &other) const;

Remove `virtual`

> teamdrive.h:228
> +        class Private;
> +        Private *const d;
> +        friend class Private;

`QScopedPointer<Private>`

> teamdrive.h:243
> +      public:
> +        explicit BackgroundImageFile(const BackgroundImageFile &other);
> +        virtual ~BackgroundImageFile();

Remove `explicit`

> teamdrive.h:244
> +        explicit BackgroundImageFile(const BackgroundImageFile &other);
> +        virtual ~BackgroundImageFile();
> +        bool operator==(const BackgroundImageFile &other) const;

Remove `virtual`

> teamdrive.h:272
> +        class Private;
> +        Private *const d;
> +        friend class Private;

`QScopedPointer<Private>`

> teamdrive.h:280
> +    explicit Teamdrive();
> +    explicit Teamdrive(const Teamdrive &other);
> +    ~Teamdrive() override;

Remove `explicit`.

> teamdrive.h:337
> +    class Private;
> +    Private *const d;
> +    friend class Private;

`QScopedPointer<Private>`

> teamdrivefetchjob.cpp:51
> +
> +    int maxResults;
> +    bool useDomainAdminAccess;

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

> teamdrivefetchjob.cpp:52
> +    int maxResults;
> +    bool useDomainAdminAccess;
> +

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

> teamdrivefetchjob.cpp:55
> +  private:
> +    TeamdriveFetchJob *q;
> +};

`TeamdriveFetchJob * const q`

> teamdrivefetchjob.cpp:90
> +
> +TeamdriveFetchJob::~TeamdriveFetchJob()
> +{

` = default`

> teamdrivefetchjob.h:84
> +    class Private;
> +    Private *const d;
> +    friend class Private;

`QScopedPointer`

REPOSITORY
  R477 KGAPI Library

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

To: barchiesi, dvratil
Cc: kde-pim, #libkgapi, barchiesi, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20190303/2a51e7f8/attachment.html>