D28560: [resources] Add a unified Google Groupware Resource (WIP)

[Daniel Vrátil]

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


  Great job!
  
  I haven't checked the detailed implementation of each handler, since I assume it's mostly just copy-pasted code from the old resources, so I looked more at the new code and the overall architecture.
  
  Regarding the Handlers being friends of the Resource: I would propose here to use a design similar to the one in the IMAP resource: create an abstract layer with Resource-like API, call it e.g. `ResourceStateInterface`. Then create an implementation of that interface, called `ResourceState`, which is a friend of `GoogleResource` and forwards all method calls to the real resource. In constructor of each handler, you pass them this `ResourceState`, instead of the `GoogleResource` itself. Thanks to this the Handlers will not have to have access to `GoogleResource`'s private stuff and it will be possible to create a `MockResourceState` for testing purposes and write proper uinit-tests for each handler.

INLINE COMMENTS

> googleresource.cpp:327
> +
> +    m_jobs = m_handlers.count();
> +    for (auto &handler : m_handlers) {

This is making me feel uneasy - we keep some global state here for a local task, but I do not have a better solution at the moment.

> googleresource.cpp:329
> +    for (auto &handler : m_handlers) {
> +        handler->retrieveCollections();
> +    }

I would propose passing the `m_rootCollection` into the `GenericHandler::retrieveCollections()` as an argument rather than allowing the Handlers to touch the Resource's private `m_rootCollection`.

> googleresource.cpp:351-354
> +    auto it = std::find_if(m_handlers.begin(), m_handlers.end(),
> +            [&collection](const GenericHandler::Ptr &handler){
> +                return collection.contentMimeTypes().contains(handler->mimetype());
> +            });

This variant of code keeps repeating - I propose a method for this, e.g. `GenericHandler *GoogleResource::handlerForCollectionTask(const QString &mimeType)`

> googleresource.cpp:389-392
> +    auto it = std::find_if(m_handlers.begin(), m_handlers.end(),
> +            [&item](const GenericHandler::Ptr &handler){
> +                return handler->canPerformTask(item);
> +            });

Same thing with the handler here and in all the other tasks - should be a method of its own

> googleresource.cpp:406-411
> +    // TODO: what if items have different mimetypes?
> +    const QString mimeType = items.first().mimeType();
> +    auto it = std::find_if(m_handlers.begin(), m_handlers.end(),
> +            [&mimeType](const GenericHandler::Ptr &handler){
> +                return handler->mimetype() == mimeType;
> +            });

Akonadi guarantees that all `items` here belong to the same Collection, thus they are all managed by the same `Handler`.

> googleresource.h:117
> +
> +    QList<GenericHandler::Ptr> m_handlers;
> +    CalendarHandler::Ptr m_freeBusyHandler;

IMO this should be a `std::vector<std::unique_ptr<GenericHandler>>` - in other words, it should be an "owning" list. When the list is destroyed, all handlers are destroyed.

> googleresource.h:118
> +    QList<GenericHandler::Ptr> m_handlers;
> +    CalendarHandler::Ptr m_freeBusyHandler;
> +    int m_jobs;

I'd propose to make a dedicated `FreeBusyHandler` that is /not/ subclasses from `GenericHandler` and implement the corresponding functionality there., rather than having two `CalendarHandler`s. Or just drop the `m_freeBusyHandler` completely and use the `CalendarHandler` from `m_handlers` directly (look it up by the mimetype).

REPOSITORY
  R44 KDE PIM Runtime

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

To: poboiko, dvratil, mlaurent
Cc: mlaurent, kde-pim, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, vkrause, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20200409/b3db5a48/attachment-0001.html>