<table><tr><td style="">vkrause added subscribers: dvratil, vkrause.<br />vkrause 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><p>Thanks for working on this! Some general thoughts:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">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.</li>
<li class="remarkup-list-item">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 <a href="https://phabricator.kde.org/p/dvratil/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@dvratil</a> was argueing to move away from that approach.</li>
</ul></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-138319">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.cpp:26</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 style="color: #aa2211">::</span><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">d</span><span class="p">(</span><span style="color: #aa4000">new</span> <span class="n">CalendarEntryPrivate</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;">this is leaked it seems, maybe make d a unique_ptr?</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-138317">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:27</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">CalendarEntryPrivate</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><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></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">add at least very minimal class-level API docs so this shows up on api.kde.org</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-138316">View Inline</a><span style="color: #4b4d51; font-weight: bold;">calendarentry.h:34</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">Q_PROPERTY</span><span class="p">(</span><span class="n">QString</span> <span class="n">icon</span> <span class="n">READ</span> <span class="n">icon</span> <span class="n">CONSTANT</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Q_PROPERTY</span><span class="p">(</span><span class="n">CalendarType</span> <span class="n">type</span> <span class="n">READ</span> <span class="n">type</span> <span class="n">CONSTANT</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Q_PROPERTY</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="n">READ</span> <span class="n">calendar</span> <span class="n">CONSTANT</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"type" is quite generic, maybe rather something like "permission"?</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-138273">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">calendarentry.h:57</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I don't understand what this is for.</p>
<p style="padding: 0; margin: 8px;">Is it like Calendar::save ?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">synchronize() maybe? ie. avoid abbreviations in API</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<br /><strong>Cc: </strong>vkrause, dvratil, davidedmundson, dhaumann<br /></div>