| Frameworks | |
| Plasma | |
| KDE PIM |
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
Reference implementation in Calindori that both provides and consumes plugins. https://invent.kde.org/kde/calindori/merge_requests/37
| No Linters Available |
| No Unit Test Coverage |
| Buildable 19584 | |
| Build 19602: arc lint + arc unit |
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
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? :)
Thanks for working on this! Some general thoughts:
| src/calendarentry.cpp | ||
|---|---|---|
| 27 | this is leaked it seems, maybe make d a unique_ptr? | |
| src/calendarentry.h | ||
| 28 | add at least very minimal class-level API docs so this shows up on api.kde.org | |
| 35 | "type" is quite generic, maybe rather something like "permission"? | |
| 58 | synchronize() maybe? ie. avoid abbreviations in API | |
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.
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 | ||
|---|---|---|
| 29 | 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...). | |
| 29 | Should this be declared in the KCalendarCore namespace? | |
| 49 | override (overrides QObject's dtor) | |
| 58 | 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). | |
| 64 | 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? | |
| src/calendarentry.h | ||
|---|---|---|
| 58 | 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 | |