Review Request 125817: Add plugin system for Calendar events

Mon Oct 26 22:05:55 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125817/#review87470
-----------------------------------------------------------


src/declarativeimports/calendar/daysmodel.cpp (line 102)
<https://git.reviewboard.kde.org/r/125817/#comment60049>

    const


src/declarativeimports/calendar/daysmodel.cpp (line 131)
<https://git.reviewboard.kde.org/r/125817/#comment60050>

    const


src/declarativeimports/calendar/daysmodel.cpp (line 140)
<https://git.reviewboard.kde.org/r/125817/#comment60042>

    Is this iteration necessary just to print debug output?
    
    Once we have a PIM plugin with many events, this will get very very noisy.


src/declarativeimports/calendar/daysmodel.cpp (line 146)
<https://git.reviewboard.kde.org/r/125817/#comment60043>

    Call `m_eventsData.reserve(m_eventsData.size() + data.size())` to reduce allocations (due to how QMultiHash::operator+= is implementated).


src/declarativeimports/calendar/daysmodel.cpp (line 161)
<https://git.reviewboard.kde.org/r/125817/#comment60052>

    const


src/declarativeimports/calendar/daysmodel.cpp (line 162)
<https://git.reviewboard.kde.org/r/125817/#comment60051>

    `m_qmlData.reserve(events.size())` to reduce allocations


src/declarativeimports/calendar/daysmodel.cpp (line 167)
<https://git.reviewboard.kde.org/r/125817/#comment60044>

    This return statement probably shouldn't be there?


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h (line 31)
<https://git.reviewboard.kde.org/r/125817/#comment60041>

    IMO the members here should be hidden in PIMPL.
    
    1) it would prevent ABI breakage (you can reoder members easilly when adding new ones)
    2) it reduced copying and memory use when inserting/retrieving the objects from the QHash as well as passing a copy to EventDataDecorator.


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h (line 40)
<https://git.reviewboard.kde.org/r/125817/#comment60047>

    This means that all-day events that span over multiple days will be ignored, which breaks many common PIM events.
    
    Otherwise the documentation should explictly say that the implementation has to resolve multi-day events into multiple EventData objects.


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h (line 42)
<https://git.reviewboard.kde.org/r/125817/#comment60046>

    You can move the bools to the end of the member list to save some bytes on padding.


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h (line 44)
<https://git.reviewboard.kde.org/r/125817/#comment60048>

    Should be called "description" IMO. "Comment" in the context of events/todos means something different (see RFC2445, part 4.8.1.4)


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h (line 53)
<https://git.reviewboard.kde.org/r/125817/#comment60038>

    explicit?


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h (line 54)
<https://git.reviewboard.kde.org/r/125817/#comment60037>

    Dtor should be virtual


src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.cpp (line 25)
<https://git.reviewboard.kde.org/r/125817/#comment60045>

    : QObject(parent)


- Daniel Vrátil


On Oct. 26, 2015, 9:22 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125817/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2015, 9:22 p.m.)
> 
> 
> Review request for Plasma and Daniel Vrátil.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> This adds a simple plugin interface that can be subclassed
> and provide events integration with Plasma Calendar applet.
> 
> It's asynchronous and I've kept it deliberately simple.
> For now the Calendar tells the plugins which date range
> is being displayed, the plugins load the data and then
> emit the dataReady() signal containing the events.
> 
> The events are stored in a multihash for quick access
> by the Calendar's agenda part but also for overall
> easy-to-use (eg. in teh model data()).
> 
> The event data is stored in EventData class, which has
> a pretty self-explanatory members, except perhaps the
> "isMinor" one. The intention with this is to support
> namedays, where in some countries the calendars have
> different name every day. This is just a minor holiday
> and as such should not mark the calendar grid, otherwise
> the whole grid would be in a different color.
> 
> Putting the interface here might raise the question of
> depending on plasma-framework, but plugins provided by
> KDE can go to plasma-workspace and other 3rd party ones
> would just have to live with it. I don't think it will
> be a problem but if it turns out it is, we can rethink
> the placement.
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/calendar/CMakeLists.txt 40ead91 
>   src/declarativeimports/calendar/calendarplugin.cpp bafe80c 
>   src/declarativeimports/calendar/daysmodel.h a5bdac9 
>   src/declarativeimports/calendar/daysmodel.cpp 2d059a8 
>   src/declarativeimports/calendar/eventdatadecorator.h PRE-CREATION 
>   src/declarativeimports/calendar/eventdatadecorator.cpp PRE-CREATION 
>   src/declarativeimports/calendar/plasmacalendarintegration/CMakeLists.txt PRE-CREATION 
>   src/declarativeimports/calendar/plasmacalendarintegration/PlasmaCalendarIntegrationConfig.cmake.in PRE-CREATION 
>   src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.h PRE-CREATION 
>   src/declarativeimports/calendar/plasmacalendarintegration/calendareventsplugin.cpp PRE-CREATION 
>   src/declarativeimports/calendar/plasmacalendarintegration/plasmacalendarintegration_export.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125817/diff/
> 
> 
> Testing
> -------
> 
> I have a simple KHolidays based plugin written (patch should be up later today)
> and patches in the Calendar applet.
> 
> Everything works as expected:
> * the days are marked as containing an event
> * the agenda part displays details of that event (name)
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20151026/baea9a86/attachment-0001.html>