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 &id, const QString &description, const QString &icon, 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<CalendarEntryPrivate> const d` for automatic memory management.

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

`const QVariantList &args`

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

I'd go for QVector, here.

> calendarplugin.h:33
> +
> +    virtual std::vector<CalendarEntry::Ptr> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20191008/bb9b5ea2/attachment-0001.html>