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
