Add a plugin system
Changes PlannedPublic

Authored by nicolasfella on Oct 6 2019, 2:42 PM.

Details

Reviewers
None
Group Reviewers
Frameworks
Plasma
KDE PIM
Summary

This allows applications/services (e.g. Akonadi, Sink, KHolidays etc) to provide calendar content to other applications/services (e.g. KOrganizer, Plasma, Calindori). The plugins expose a set of KCalendarCore::Calendars

This is intended to replace the calendar plugin system found in https://cgit.kde.org/kdeclarative.git/tree/src/calendarevents

Depends on D28834

Test Plan

Reference implementation in Calindori that both provides and consumes plugins. https://invent.kde.org/kde/calindori/merge_requests/37

Diff Detail

Repository
R172 KCalendar Core
Branch
arcpatch-D24443_2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25241
Build 25259: arc lint + arc unit
nicolasfella requested review of this revision.Oct 6 2019, 2:42 PM
nicolasfella created this revision.

Something I'm unsure about is the usage of std::vector in public API. We never really did that, but given Qt6 is ahead we might want to consider doing it from now on

nicolasfella edited the test plan for this revision. (Show Details)Oct 6 2019, 2:44 PM
nicolasfella edited the test plan for this revision. (Show Details)Oct 6 2019, 3:02 PM

I'd go for a QVector for now. Arguing with Qt6 doesn't sound convincing to me, since Qt6 will take another>=1 year(s). So why try this experiment in public API now? :)

Plan makes sense.

src/calendarentry.h
35 ↗(On Diff #67385)

If the intention is for QML to be able to get and access the CalendarCore::Calendar object this won't work. You'll need to return the raw pointer.

57 ↗(On Diff #67385)

I don't understand what this is for.

Is it like Calendar::save ?

Thanks for working on this! Some general thoughts:

  • Is KDeclarative is the right place for this? It's a module on the way out in KF6, it is hard to build for Android due to its dependency chain (which isn't even needed for this), while at the same time forcing ABI stability on this basically immediately without much chance for battle testing this.
  • This currently represents a flat list of calendars, which matches Android IIRC, but not Akonadi (which treats calendars as folders and thus hierarchical). That might not be a problem as long as we treat Akonadi as a single calendar and don't expose the different backends on this level at all, but IIUC @dvratil was argueing to move away from that approach.
src/calendarentry.cpp
26 ↗(On Diff #67385)

this is leaked it seems, maybe make d a unique_ptr?

src/calendarentry.h
27 ↗(On Diff #67385)

add at least very minimal class-level API docs so this shows up on api.kde.org

34 ↗(On Diff #67385)

"type" is quite generic, maybe rather something like "permission"?

57 ↗(On Diff #67385)

synchronize() maybe? ie. avoid abbreviations in API

  • Is KDeclarative is the right place for this? It's a module on the way out in KF6, it is hard to build for Android due to its dependency chain (which isn't even needed for this), while at the same time forcing ABI stability on this basically immediately without much chance for battle testing this.

Uh, where did I get the idea from that this is in KDeclarative? So that part of the comment is obviously nonsense. The concern about committing to ABI too quickly remains though.

dvratil added a comment.EditedOct 8 2019, 9:30 AM
  • I would add name property to the CalendarEntry
  • I would add color property to the CalendarEntry, so that calendar that is e.g. green in KOrganizer would show up green in the Plasma applet, too.

The Akonadi plugin for this will need to expose a list of multiple calendars, otherwise the user would only be able to toggle all-or-nothing, but not to toggle individual calendars. In Akonadi, we can have a tree of calendar folders, but realistically what you usually see is only the account folder with calendar subfolders, without any deeper nesting. I wonder if maybe we could have some system of "groups", so e.g. all different plugins that provide some holidays calendars would make the calendars members of "holidays" group, the Akonadi calendars could be groupped by the account name they belong to...this could be then shown nicely in the (QML) UI.

src/calendarentry.h
28 ↗(On Diff #67385)

I'm confused, does this represent a single calendar or event? To me a calendar entry is an event. I realize Calendar cannot be used, but I think it should be a better name, or maybe be a nested class in CalendarPlugin (if you can nest QObjects, I actually don't know...).

28 ↗(On Diff #67385)

Should this be declared in the KCalendarCore namespace?

48 ↗(On Diff #67385)

override (overrides QObject's dtor)

57 ↗(On Diff #67385)

I would move sync() from here to be a virtual method on the plugin - sync(const CalendarEntry::Ptr &). The implementations would reimplement it to handle sync, which feels cleaner than having to connect to syncRequested() signal on each calendar that the plugin owns, and it decouples data (calendar) from logic (plugin).

63 ↗(On Diff #67385)

Use std::unique_ptr<CalendarEntryPrivate> const d for automatic memory management.

src/calendarplugin.h
32

const QVariantList &args

34

I'd go for QVector, here.

34

Should it be a pure virtual function? There's no point in implementing a CalendarPlugin if you don't reimplement this method...

40

Unused?

nicolasfella marked 9 inline comments as done.
  • Drop property
  • Use QVector
  • Use unique_ptr
  • Make unique_ptr const
  • Comments
  • Make calendars pure virtual
  • ref
nicolasfella added inline comments.Nov 3 2019, 10:32 PM
src/calendarentry.h
57 ↗(On Diff #67385)

I decided to make it a virtual member of CalendarEntry because that simplifies the implementation in calindori. This way it's enough to keep track of the entries, otherwise I'd need to keep track of the plugins too. Also this allows for more fine-grained sync, interesting for large numbers of calendars that are expensive to sync

src/calendarplugin.h
40

My idea was to be able to add a d-ptr later if needed without breaking ABI

  • Put into namespace
nicolasfella marked an inline comment as done.Nov 3 2019, 10:44 PM
  • Fix build
  • Rename description to name
Restricted Application added a project: KDE PIM. · View Herald TranscriptDec 7 2019, 9:46 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
ognarb added a subscriber: ognarb.Dec 7 2019, 10:17 PM
ognarb added inline comments.
src/calendarentry.h
63 ↗(On Diff #67385)

This wasn't done, I still see CalendarEntryPrivate *d;

src/calendarplugin.h
40

Will this also work if we are using unique_ptr?

  • Use uniqe_ptr again
dvratil added inline comments.Apr 4 2020, 12:10 PM
src/calendarentry.cpp
36 ↗(On Diff #71067)

= default (instead of empty body)

57 ↗(On Diff #71067)

Should that be d->type? How can implementations change it? Will implementation have to access calendarentry_p.h?

nicolasfella planned changes to this revision.Apr 4 2020, 12:57 PM
  • single level hierarchy with a calendargroup object
  • calendar entry folded into calendar
  • calendar::open
z3ntu added a subscriber: z3ntu.Apr 10 2020, 2:54 PM
  • Remove calendarentry
  • Convert license headers to SPDX
nicolasfella planned changes to this revision.Apr 14 2020, 5:08 PM