Add a plugin system
Needs ReviewPublic

Authored by nicolasfella on Sun, Oct 6, 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 CalendarEntries, which are a thin wrapper around a KCalendarCore::Calendar and some metadata.

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

Test Plan

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

Diff Detail

Branch
plugins
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17362
Build 17380: arc lint + arc unit
nicolasfella requested review of this revision.Sun, Oct 6, 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)Sun, Oct 6, 2:44 PM
nicolasfella edited the test plan for this revision. (Show Details)Sun, Oct 6, 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

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

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

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

src/calendarentry.h
27

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

34

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

57

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.EditedTue, Oct 8, 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

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

Should this be declared in the KCalendarCore namespace?

48

override (overrides QObject's dtor)

57

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

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

src/calendarplugin.h
31

const QVariantList &args

33

I'd go for QVector, here.

33

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

39

Unused?