<table><tr><td style="">dvratil added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24443">View Revision</a></tr></table><br /><div><div><ul class="remarkup-list">
<li class="remarkup-list-item">I would add <tt style="background: #ebebeb; font-size: 13px;">name</tt> property to the <tt style="background: #ebebeb; font-size: 13px;">CalendarEntry</tt></li>
<li class="remarkup-list-item">I would add <tt style="background: #ebebeb; font-size: 13px;">color</tt> property to the <tt style="background: #ebebeb; font-size: 13px;">CalendarEntry</tt>, so that calendar that is e.g. green in KOrganizer would show up green in the Plasma applet, too.</li>
</ul>

<p>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.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138428">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:28</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">class</span> <span class="n">Q_DECL_EXPORT</span> <span style="color: #a0a000">CalendarEntry</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QObject</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I'm confused, does this represent a single calendar or  event? To me a calendar entry is an event. I realize <tt style="background: #ebebeb; font-size: 13px;">Calendar</tt> cannot be used, but I think it should be a better name, or maybe be a nested class in <tt style="background: #ebebeb; font-size: 13px;">CalendarPlugin</tt> (if you can nest QObjects, I actually don't know...).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138433">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:28</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">class</span> <span class="n">Q_DECL_EXPORT</span> <span style="color: #a0a000">CalendarEntry</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QObject</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Should this be declared in the <tt style="background: #ebebeb; font-size: 13px;">KCalendarCore</tt> namespace?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138431">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:48</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">CalendarEntry</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">id</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">description</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">icon</span><span class="p">,</span> <span class="n">CalendarType</span> <span class="n">type</span><span class="p">,</span> <span class="n">KCalendarCore</span><span style="color: #aa2211">::</span><span class="n">Calendar</span><span style="color: #aa2211">::</span><span class="n">Ptr</span> <span class="n">calendar</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa2211">~</span><span class="n">CalendarEntry</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">override</tt> (overrides QObject's dtor)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138439">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:57</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">public</span> <span style="color: #a0a000">Q_SLOTS</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">void</span> <span class="n">sync</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would move <tt style="background: #ebebeb; font-size: 13px;">sync()</tt> from here to be a virtual method on the plugin - <tt style="background: #ebebeb; font-size: 13px;">sync(const CalendarEntry::Ptr &)</tt>. The implementations would reimplement it to handle sync, which feels cleaner than having to connect to <tt style="background: #ebebeb; font-size: 13px;">syncRequested()</tt> signal on each calendar that the plugin owns, and it decouples data (calendar) from logic (plugin).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138432">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:63</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #a0a000">private</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">CalendarEntryPrivate</span> <span style="color: #aa2211">*</span><span class="n">d</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">};</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Use <tt style="background: #ebebeb; font-size: 13px;">std::unique_ptr<CalendarEntryPrivate> const d</tt> for automatic memory management.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138437">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarplugin.h:31</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">CalendarPlugin</span><span class="p">(</span><span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span class="n">QVariantList</span> <span class="n">args</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">const QVariantList &args</tt></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138434">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarplugin.h:33</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">virtual</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">vector</span><span style="color: #aa2211"><</span><span class="n">CalendarEntry</span><span style="color: #aa2211">::</span><span class="n">Ptr</span><span style="color: #aa2211">></span> <span class="n">calendars</span><span class="p">()</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I'd go for QVector, here.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138435">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarplugin.h:33</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">virtual</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">vector</span><span style="color: #aa2211"><</span><span class="n">CalendarEntry</span><span style="color: #aa2211">::</span><span class="n">Ptr</span><span style="color: #aa2211">></span> <span class="n">calendars</span><span class="p">()</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Should it be a pure virtual function? There's no point in implementing a <tt style="background: #ebebeb; font-size: 13px;">CalendarPlugin</tt> if you don't reimplement this method...</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24443#inline-138436">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarplugin.h:39</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #a0a000">private</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">void</span> <span style="color: #aa2211">*</span><span class="n">d</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unused?</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24443">https://phabricator.kde.org/D24443</a></div></div><br /><div><strong>To: </strong>nicolasfella, Frameworks, Plasma, KDE PIM<br /><strong>Cc: </strong>vkrause, dvratil, davidedmundson, dhaumann<br /></div>