[mkcal] Simplify rawExpandedEvents() by using upstrean OccurrenceIterator. #2
Conversation
dcaliste
changed the title
|
The one outstanding discussion from the previous MR was that now, if the client attempts to dissociate the same occurrence twice (i.e. create two exception occurrences at the same recurrenceId) it will fail at storage save() time instead of at dissociate() time. |
|
David pointed out that this has the potential to transform a single-event error into a whole-sync-cycle error. In practice, it should not be a problem, but worth considering. |
|
To address this, I'm going to add a call to |
|
I've added the Once accepted, to avoid the warning, you should consider sailfishos/nemo-qml-plugin-calendar#3. In that MR, I'm also bringing up a discussion on the necessity to actually keep this @chriadam and @pvuorela, what do you think ? @flypig you may have also some advices ? |
src/extendedcalendar.h
Outdated
| */ | ||
| ExpandedIncidenceList rawExpandedIncidences(const QDateTime &start, const QDateTime &end) const; | ||
| /** | ||
| Depreacted routine, kept for backward compatibility. |
There was a problem hiding this comment.
Corrected, thanks.
Could make sense to indeed then just drop the method. I think it was added here when the calendar app was getting hacked together. More we get rid of our extra api the better. |
I cannot agree more ;) So I've reworked the commits to simply remove the function. This requires to include sailfishos/nemo-qml-plugin-calendar#3 first. |
This method is deprecated in favor of KCalendarCore::OccurrenceIterator. By removing it, the convention to always add an occurrence recurrenceId as an EXDATE is not necessary anymore since the upstream iterator is dealing with exceptions internally.
|
So, are we all happy with this one now? In combination with sailfishos/nemo-qml-plugin-calendar#3 |
|
I'm happy so let's merge. |
This has the consequence that it's not mandatory anymore to declare any exception recurrenceId as an exdatetime of the parent. The API of rawExpandedEvents() is modified to simplify it. The inclusion parameters were not used. The expansion time zone has also been removed, so returned start time and end time are always given in the system time zone. This allow to transparently handle the events declared in local time.
@chriadam and @pvuorela, the purpose of this MR is to simplify a lot the rawExpandedEvents() by relying on the upstream OccurrenceIterator. It requires to change the method API though. When looking where it is used, I find only mer-core/nemo-qml-plugin-calendar in calendarworker.cpp. There it is used without the inclusive parameters, so I removed them. It is also used with the system time zone, which is in my opinion the only value that makes sense. Assuming only system time zone is used to expand the events simplify the routine a lot and avoid all changes in date time that were previously done.
I've slightly updated the tests to use the new API and all of them are passing (except the all day deserialisation onethat is broken, but not related), particularly, they are also passing when specifying another time zone like in TZ=Asia/Ho_Chi_Minh ./tst_storage.
If you agree with the API change, I'll submit a MR in the QML plugin to update it also.
The last advantage of this MR (besides relying more on upstream methods, and removing lines from mKCal) is also very important : it removes the necessity to add occurrence exception recurrenceId as exDateTime in the parent. This necessity was contrary to the RFC, but used as a mean to get timesInInterval() of the parent not to return an event on the exception date time (OccurrenceIterator is filtering out the returned list of timeInInterval for us and makes this all transparent). This RFC violation created complications in sync code that needs to add the exDateTime when receiving a recurring event with exceptions and remove them when upsyncing.
That's the purpose of the second commit : it is removing the addition of the exDateTime in dissociateSingleOccurrence(). But because of legacy events stored with additional exDateTime, not all code from the sync plugins handling with removing the additionnal exDateTime can be removed at this time.
Last comment maybe : one may decide to rename the rawExpandedEvents() to rawExpandedIncidences() since upstream OccurrenceIterator is dealing on events, todos and journals.
There is a long comment history, see : https://git.sailfishos.org/mer-core/mkcal/merge_requests/66