D24443: Add a plugin system

[Nicolas Fella]

nicolasfella abandoned this revision.
nicolasfella added a comment.


  Moving to invent: 
https://invent.kde.org/frameworks/kcalendarcore/-/merge_requests/20

REPOSITORY
  R172 KCalendar Core

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: z3ntu, ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, 
dhaumann, fbampaloukas, dvasin, rodsevich, winterz, cgiboudeaux, mlaurent, 
knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella planned changes to this revision.

REPOSITORY
  R172 KCalendar Core

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: z3ntu, ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, 
dhaumann, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 80142.
nicolasfella added a comment.


  - Convert license headers to SPDX

REPOSITORY
  R172 KCalendar Core

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=80141=80142

BRANCH
  arcpatch-D24443_2

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: z3ntu, ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, 
dhaumann, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 80141.
nicolasfella added a comment.


  - Remove calendarentry

REPOSITORY
  R172 KCalendar Core

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=71067=80141

BRANCH
  arcpatch-D24443_2

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: z3ntu, ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, 
dhaumann, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella edited the summary of this revision.
nicolasfella added a dependency: D28834: Add metadata properties to calendar.

REPOSITORY
  R172 KCalendar Core

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: z3ntu, ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, 
dhaumann, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella planned changes to this revision.
nicolasfella added a comment.


  - single level hierarchy with a calendargroup object
  - calendar entry folded into calendar
  - calendar::open

REPOSITORY
  R172 KCalendar Core

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, dhaumann, 
fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Daniel Vrátil]

dvratil added inline comments.

INLINE COMMENTS

> calendarentry.cpp:36
> +
> +CalendarEntry::~CalendarEntry()
> +{

` = default` (instead of empty body)

> calendarentry.cpp:57
> +{
> +return ReadWrite;
> +}

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

REPOSITORY
  R172 KCalendar Core

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, dhaumann, 
fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 71067.
nicolasfella added a comment.


  - Use uniqe_ptr again

REPOSITORY
  R172 KCalendar Core

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=71066=71067

BRANCH
  arcpatch-D24443_1

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarentry.cpp
  src/calendarentry.h
  src/calendarentry_p.h
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, dhaumann, 
fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Carl Schwan]

ognarb added inline comments.

INLINE COMMENTS

> dvratil wrote in calendarentry.h:63
> Use `std::unique_ptr const d` for automatic memory 
> management.

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

> nicolasfella wrote in calendarplugin.h:39
> My idea was to be able to add a d-ptr later if needed without breaking ABI

Will this also work if we are using unique_ptr?

REPOSITORY
  R172 KCalendar Core

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: ognarb, kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, dhaumann, 
fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 71066.
nicolasfella added a comment.
Herald added a project: KDE PIM.
Herald added a subscriber: kde-pim.


  - Fix build
  - Rename description to name

REPOSITORY
  R172 KCalendar Core

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=69253=71066

BRANCH
  arcpatch-D24443_1

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarentry.cpp
  src/calendarentry.h
  src/calendarentry_p.h
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: kde-pim, dkardarakos, vkrause, dvratil, davidedmundson, dhaumann, 
fbampaloukas, dcaliste, dvasin, rodsevich, winterz, mlaurent, knauss

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 69253.
nicolasfella added a comment.


  - Docs

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=69241=69253

BRANCH
  arcpatch-D24443

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarentry.cpp
  src/calendarentry.h
  src/calendarentry_p.h
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: dkardarakos, vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella marked an inline comment as done.

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: dkardarakos, vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 69241.
nicolasfella added a comment.


  - Put into namespace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=69239=69241

BRANCH
  arcpatch-D24443

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarentry.cpp
  src/calendarentry.h
  src/calendarentry_p.h
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: dkardarakos, vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> dvratil wrote in calendarentry.h: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).

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

> dvratil wrote in calendarplugin.h:39
> Unused?

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

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: dkardarakos, vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella updated this revision to Diff 69239.
nicolasfella marked 9 inline comments as done.
nicolasfella added a comment.


  - Drop property
  - Use QVector
  - Use unique_ptr
  - Make unique_ptr const
  - Comments
  - Make calendars pure virtual
  - ref

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24443?vs=67385=69239

BRANCH
  arcpatch-D24443

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarentry.cpp
  src/calendarentry.h
  src/calendarentry_p.h
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: dkardarakos, vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Daniel Vrátil]

dvratil added a comment.


  - 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 plugins 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.

INLINE COMMENTS

> calendarentry.h:28
> +
> +class Q_DECL_EXPORT CalendarEntry : public QObject
> +{

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...).

> calendarentry.h:28
> +
> +class Q_DECL_EXPORT CalendarEntry : public QObject
> +{

Should this be declared in the `KCalendarCore` namespace?

> calendarentry.h:48
> +CalendarEntry(const QString , const QString , const 
> QString , CalendarType type, KCalendarCore::Calendar::Ptr calendar);
> +~CalendarEntry();
> +

`override` (overrides QObject's dtor)

> calendarentry.h:57
> +public Q_SLOTS:
> +void sync();
> +

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).

> calendarentry.h:63
> +private:
> +CalendarEntryPrivate *d;
> +};

Use `std::unique_ptr const d` for automatic memory 
management.

> calendarplugin.h:31
> +public:
> +CalendarPlugin(QObject *parent, const QVariantList args);
> +

`const QVariantList `

> calendarplugin.h:33
> +
> +virtual std::vector calendars() const;
> +

I'd go for QVector, here.

> calendarplugin.h:33
> +
> +virtual std::vector calendars() const;
> +

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

> calendarplugin.h:39
> +private:
> +void *d;
> +};

Unused?

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Volker Krause]

vkrause added a reviewer: KDE PIM.

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

To: nicolasfella, #frameworks, #plasma, #kde_pim
Cc: vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Volker Krause]

vkrause added a comment.


  In D24443#543140 , @vkrause wrote:
  
  > - 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.

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

To: nicolasfella, #frameworks, #plasma
Cc: vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[Volker Krause]

vkrause added subscribers: dvratil, vkrause.
vkrause added a comment.


  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.

INLINE COMMENTS

> calendarentry.cpp:26
> +CalendarEntry::CalendarEntry(const QString , const QString , 
> const QString , CalendarType type, KCalendarCore::Calendar::Ptr calendar)
> +: d(new CalendarEntryPrivate)
> +{

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

> calendarentry.h:27
> +class CalendarEntryPrivate;
> +
> +class Q_DECL_EXPORT CalendarEntry : public QObject

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

> calendarentry.h:34
> +Q_PROPERTY(QString icon READ icon CONSTANT)
> +Q_PROPERTY(CalendarType type READ type CONSTANT)
> +Q_PROPERTY(KCalendarCore::Calendar::Ptr calendar READ calendar CONSTANT)

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

> davidedmundson wrote in calendarentry.h:57
> I don't understand what this is for.
> 
> Is it like Calendar::save ?

synchronize() maybe? ie. avoid abbreviations in API

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

To: nicolasfella, #frameworks, #plasma
Cc: vkrause, dvratil, davidedmundson, dhaumann

D24443: Add a plugin system

[David Edmundson]

davidedmundson added a comment.


  Plan makes sense.

INLINE COMMENTS

> calendarentry.h:35
> +Q_PROPERTY(CalendarType type READ type CONSTANT)
> +Q_PROPERTY(KCalendarCore::Calendar::Ptr calendar READ calendar CONSTANT)
> +public:

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.

> calendarentry.h:57
> +public Q_SLOTS:
> +void sync();
> +

I don't understand what this is for.

Is it like Calendar::save ?

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

To: nicolasfella, #frameworks, #plasma
Cc: davidedmundson, dhaumann

D24443: Add a plugin system

[Dominik Haumann]

dhaumann added a comment.


  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? :)

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

To: nicolasfella, #frameworks, #plasma
Cc: dhaumann

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella edited the test plan for this revision.

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

To: nicolasfella, #frameworks, #plasma

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella added a comment.


  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

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

To: nicolasfella, #frameworks, #plasma

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella edited the test plan for this revision.

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

To: nicolasfella, #frameworks, #plasma

D24443: Add a plugin system

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added reviewers: Frameworks, Plasma.
nicolasfella requested review of this revision.

REVISION 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

BRANCH
  plugins

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

AFFECTED FILES
  src/CMakeLists.txt
  src/calendarentry.cpp
  src/calendarentry.h
  src/calendarentry_p.h
  src/calendarplugin.cpp
  src/calendarplugin.h

To: nicolasfella, #frameworks, #plasma