[resources] Add a unified Google Groupware Resource
ClosedPublic

Authored by poboiko on Apr 4 2020, 2:16 PM.

Details

Reviewers
dvratil
mlaurent
Commits
R44:b47c248321cd: Fix "My Contacts" <-> "Other Contacts" handling
R44:f4f50d1c5ed9: Fix subtasks detaching
R44:737096e0a406: Change logic for contacts subresource
R44:c5b4be60d15d: Address raised issues
R44:38822e551ad5: Fixed copy-paste typos for collection management code
R44:710dcd347427: [resources] Add a unified Google Groupware Resource (WIP)
R44:006d888337c3: Use std::find_if, small changes
R44:2a02937ed455: Allow calendars removing
R44:6d7648bdc38a: Fix creation of new calendars
R44:e01a5ee3da29: Initial implementation of a Google Groupware resource
R44:1df2c557a170: Fixed batch objects handling: call changesCommitted
R44:f60707f6973f: Some cosmetic code changes
R44:210710395cb9: Get rid of unnecessary Settings class (now there's only GoogleSettings)
R44:c516c89b2b11: Use lambdas in some places
R44:5aece8da38cb: Added collection management code
R44:bfd3e478ed67: Reverted to old behavior of contacts
R44:65ac85e8816b: Settings dialog merging
R44:ed82806dce9d: Populate remoteId of newly created collections
R44:695ab4777f14: Got rid of GoogleAccountManager, merged Wallet functionality to GoogleSettings
R44:24761e194492: Rebase
R44:07bd2f785a1d: Hide "All Contacts" collection from UI
R44:dc5439c0fceb: Do not run unnecessary ItemModifyJobs as changeCommited does that for us
R44:ae6f484a5bc8: Use std::find_if / std::transform where appropriate
R44:809ee9664b28: Make compiler & clazy happy
R44:e3f9a949e45f: Use newly introduced syncToken for incremental updates of calendar
R44:b1c6ed3e8675: Move to google-groupware folder
R44:57cf67672c5a: Merge branch 'dev/google-groupware' of git.kde.org:kdepim-runtime into…
R44:0ee5cf2d3009: Implement item linking/unlinking for contacts subresource
R44:def6de004985: Added support for batch operations
R44:5209f761b268: [resources] Add a unified Google Groupware Resource (WIP)
R44:38bf66cff415: Account is now stored only inside settings
R44:52113e2f3c18: [resources] Add a unified Google Groupware Resource
R44:3e62568e2fb8: Fix parent detaching: ask Google for that too
R44:b12a0ec2ec5c: Added FreeBusy handling
R44:3aa6678f377f: Fixed photos fetching
R44:5188c4add230: Some more cosmetic changes to the code
Summary

This is an attempt to unify existing Calendar&Tasks and Contacts resources into
a single Groupware resource. At some point, hopefully, GMail support could be also
added here (see task T646: [Google] Native Gmail resource / Google Groupware and T9422: Automatic setup of Gmail + Google Calendar/Contacts).

Various "subresources" (Calendar, Tasks and Contacts) are implemented as subclasses of GenericHandler,
which is a basic Akonadi::ResourceBase interface. The resource decides which Handler it should call
by looking at mimetypes. Handlers are friends of GoogleResource, so they can call its callbacks
(like itemsRetrieved()) as needed. This was done primarily to separate logic of different subresources;
this might be not the best solution, I'm open to suggestions.

This patch also reworks the settings dialog & relevant code.
The dialog is now using .ui file. The "account picker" is gone, as it's no longer needed;
instead, a single "Configure..." button is added which invokes the auth process.

It also implements "last sync token" API (T647: [KGAPI] Investigate the new "last sync token" in Google API) for calendar incremental updates. Without this API,
event moving between calendars were not handled properly (i.e. event was not removed from the "source" calendar).

Work to be done:

  1. KAccounts integration. Need to be able to disable various subresources on demand, and determine auth scopes based on that.
  2. GMail integration. Need to somehow adopt ImapResourceBase / ResourceState scheme, and merge it with current Handlers scheme.
  3. Add Akonadi::Tag support for Contacts. Tags seem to be more appropriate than having bunch of virtual collections, but this might require some changes inside KAddressBook.
Test Plan

Here's a comprehensive list of what was tested and what issues were discovered.

  1. Adding event locally
  2. Changing event locally
  3. Removing event locally
  4. Moving events between calendars locally
  5. Adding calendar locally
    • new calendar is added as a virtual collection, cannot add events there afterwards; probably a KOrganizer issue
    • color of newly added calendar is not known, google just don't return it to us
  6. Removing calendar locally
  7. Changing calendar locally
  8. Adding event remotely
  9. Changing event remotely
  10. Removing event remotely
  11. Moving events between calendars remotely
  12. Adding calendar remotely
  13. Removing calendar remotely
  14. Adding task&subtask remotely
  15. Changing task locally
  16. Removing task locally
  17. Removing a task with subtasks locally (subtasks go to the upper level)
  18. Adding/removing tasklist locally
    • wasn't tested, but should work. KOrganizer just don't know how to add a tasklist, it adds a calendar by default (probably it sees no difference between them...)
  19. Changing tasklist locally
  20. Adding task&subtask remotely
  21. Changing task remotely
  22. Removing task remotely
  23. Adding tasklist remotely (it is not subscribed automatically, however, so user have to go to account settings and enable it)
  24. Changing tasklist remotely
  25. Removing tasklist remotely
  26. Adding contact (including photo) locally, both inside "My Contacts" and "Other Contacts" groups
  27. Moving contact between "My Contacts" and "Other Contacts" groups
  28. Chaging contact (including photo) locally
  29. Removing contat locally
  30. Adding contact to contact group locally
  31. Removing contact from contact group locally
    • wasn't tested, since I've found to UI to "Unlink Item" :(
  32. Adding contact (including photo) remotely
  33. Changing contact (including photo) remotely
  34. Removing contact remotely
  35. Adding contact to contact group remotely
  36. Removing contact from contact group remotely
    • doesn't work: need an easy way to fetch all collections we're linked to (so we know which UnlinkJobs we should start)

Diff Detail

Repository
R44 KDE PIM Runtime
Branch
google-groupware (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25584
Build 25602: arc lint + arc unit
poboiko created this revision.Apr 4 2020, 2:16 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 4 2020, 2:16 PM
poboiko requested review of this revision.Apr 4 2020, 2:16 PM
mlaurent requested changes to this revision.Apr 4 2020, 3:20 PM
mlaurent added a subscriber: mlaurent.
mlaurent added inline comments.
kdepim-runtime.categories
31 ↗(On Diff #79297)

Better to rebase it.
now it's autogenerated

resources/google-groupware/calendarhandler.cpp
110

coding style Job *job

resources/google-groupware/defaultreminderattribute.cpp
51

const

resources/google-groupware/generichandler.h
63

initialize it to nullptr.

coding style GoogleResource *m_resource

resources/google-groupware/googlesettings.h
70

better to initialize value here => = false;

resources/google-groupware/googlesettingsdialog.cpp
101

missing delete m_ui;

resources/google-groupware/googlesettingsdialog.ui
14

remove it. We don"'t want to translate "Form"

resources/google-groupware/taskhandler.cpp
145

const Object &objects

This revision now requires changes to proceed.Apr 4 2020, 3:20 PM
poboiko updated this revision to Diff 79326.Apr 4 2020, 5:30 PM
poboiko marked 8 inline comments as done.

Address raised issues; rebase

This revision was not accepted when it landed; it landed in state Needs Review.Apr 4 2020, 5:38 PM
This revision was automatically updated to reflect the committed changes.
poboiko reopened this revision.Apr 4 2020, 5:42 PM

OK, I've messed up a rebase, and apparently it was apparently landed to the branch "dev/google-groupware".
Sorry about that! Will try to figure out how to fix it :S

poboiko updated this revision to Diff 79330.Apr 4 2020, 5:54 PM

Second attempt to update diff, sorry for the noise

poboiko updated this revision to Diff 79331.Apr 4 2020, 6:02 PM

Yet another attempt to fix a mess

poboiko updated this revision to Diff 79332.Apr 4 2020, 6:08 PM

Remove unused "Form" header

poboiko removed commits: R44:57cf67672c5a: Merge branch 'dev/google-groupware' of git.kde.org:kdepim-runtime into…, R44:710dcd347427: [resources] Add a unified Google Groupware Resource (WIP), R44:c5b4be60d15d: Address raised issues, R44:b1c6ed3e8675: Move to google-groupware folder, R44:b47c248321cd: Fix "My Contacts" <-> "Other Contacts" handling, R44:ae6f484a5bc8: Use std::find_if / std::transform where appropriate, R44:5188c4add230: Some more cosmetic changes to the code, R44:3e62568e2fb8: Fix parent detaching: ask Google for that too, R44:f4f50d1c5ed9: Fix subtasks detaching, R44:ed82806dce9d: Populate remoteId of newly created collections, R44:f60707f6973f: Some cosmetic code changes, R44:6d7648bdc38a: Fix creation of new calendars, R44:809ee9664b28: Make compiler & clazy happy, R44:1df2c557a170: Fixed batch objects handling: call changesCommitted, R44:bfd3e478ed67: Reverted to old behavior of contacts, R44:def6de004985: Added support for batch operations, R44:c516c89b2b11: Use lambdas in some places, R44:2a02937ed455: Allow calendars removing, R44:006d888337c3: Use std::find_if, small changes, R44:dc5439c0fceb: Do not run unnecessary ItemModifyJobs as changeCommited does that for us, R44:24761e194492: Rebase, R44:38bf66cff415: Account is now stored only inside settings, R44:e3f9a949e45f: Use newly introduced syncToken for incremental updates of calendar, R44:737096e0a406: Change logic for contacts subresource, R44:38822e551ad5: Fixed copy-paste typos for collection management code, R44:3aa6678f377f: Fixed photos fetching, R44:695ab4777f14: Got rid of GoogleAccountManager, merged Wallet functionality to GoogleSettings, R44:65ac85e8816b: Settings dialog merging, R44:5aece8da38cb: Added collection management code, R44:210710395cb9: Get rid of unnecessary Settings class (now there's only GoogleSettings), R44:0ee5cf2d3009: Implement item linking/unlinking for contacts subresource, R44:b12a0ec2ec5c: Added FreeBusy handling, R44:07bd2f785a1d: Hide "All Contacts" collection from UI, R44:e01a5ee3da29: Initial implementation of a Google Groupware resource.Apr 4 2020, 6:09 PM
poboiko updated this revision to Diff 79334.Apr 4 2020, 6:14 PM

Initialize m_isReady in header

mlaurent requested changes to this revision.Apr 7 2020, 12:01 PM
mlaurent added inline comments.
resources/google-groupware/CMakeLists.txt
16

sync with kdepim-runtime master. It's not necessary

This revision now requires changes to proceed.Apr 7 2020, 12:01 PM
poboiko updated this revision to Diff 79580.Apr 7 2020, 3:15 PM

Remove check ECM_VERSION < 5.68 from CMakeLists for logging categories

poboiko marked an inline comment as done.Apr 7 2020, 3:15 PM
dvratil requested changes to this revision.Apr 9 2020, 11:41 AM

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.

resources/google-groupware/googleresource.cpp
328

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.

330

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.

352–355

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

390–393

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

407–412

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

resources/google-groupware/googleresource.h
118

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.

119

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

This revision now requires changes to proceed.Apr 9 2020, 11:41 AM
poboiko updated this revision to Diff 79760.Apr 10 2020, 11:11 AM

Implemented interface system similar to Imap:

  • added ResourceStateInterface, which is a generic interface for ResourceBase methods (could probably be shared with imap resource in the future)
  • added GoogleResourceStateInterface, which provides interface for some GoogleResource-specific methods (like handleError(), or FreeBusy)
  • added GoogleResourceState (friend of GoogleResource), which simply implements this interface and propagates all the calls to the resource.

Handlers are now std::unique_ptr.
Added generic methods to fetch a handler by mimeType or to fetch a handler for collection.

Added dedicated FreeBusyHandler, which implements just FreeBusy interface
(implemented in calendarhandler.cpp, as it is calendar-related)

Moved slotGenericJobFinished from Resource to GenericHandler, so it works through the same ResourceStateInterface.

Added canPerformTask(Item::List items) method, which checks if all of items have the payload and correct mimetype, not just the items.first().
Moved canPerformTask() methods to GenericHandler.

poboiko marked 6 inline comments as done.Apr 10 2020, 11:12 AM
dvratil accepted this revision.Apr 15 2020, 1:04 PM

Thanks a lot for you work! I think the patch is good to land, but do not commit it yet, please.

However, before we can ship this, we need to implement a migration path from the old resources to this new one. Let me try to implement something, then we can merge both changes at the same time, and remove the old resources as well.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 18 2020, 7:03 PM
This revision was automatically updated to reflect the committed changes.
dvratil reopened this revision.Apr 18 2020, 7:06 PM

I accidentally pushed the commit to my dev branch, it's still open :)

dvratil accepted this revision.Apr 18 2020, 7:06 PM
poboiko updated this revision to Diff 80527.Apr 19 2020, 10:27 AM

Some cosmetic changes: removed trailing whitespaces, fixed typo in comment, etc.

Also, we should respect the default value for interval check time in the dialog
(even when interval checking is disabled and spinbox is grayed out;
the default value is 60 minutes, not 30)

dvratil accepted this revision.Apr 21 2020, 10:13 AM

Thanks a lot for your work! You can land the patch now.

Once it's landed I'll land the migrator as well.

I'll leave you the honors of removing the old Google resources :-)

poboiko updated this revision to Diff 80742.Apr 21 2020, 10:28 AM

Finalize porting: remove the old Goolge resources

poboiko retitled this revision from [resources] Add a unified Google Groupware Resource (WIP) to [resources] Add a unified Google Groupware Resource.Apr 21 2020, 10:28 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 21 2020, 10:35 AM
This revision was automatically updated to reflect the committed changes.