Make Event::uniqueId sole identifier for events.

[dcaliste]

Remove the recurrenceId from the API, so uniqueId
is able to uniquely identify an event, or an exception.
This is done internally by affecting the
instanceIdentifier to Event::uniqueId.

This would allow later to add a notebook UID
information to this uniqueId and thus being
able to add multi-notebook support without
changing once again the API.

@pvuorela , this one is more to open a discussion on the evolution of the nemo-qml-plugin-calendar API, because with incidences that can share the same UID between notebooks, obviously things like Manager::eventObject(uid, recId) or CalendarApi::remove(uid, recurrenceId, startTime) are broken. I can see three ways to solve this:

1. Add a notebookUid argument to such routines, which means an API break for the public routines or additionnal required properties like for CalendarEventQuery.
2. Hide the notebookUid somehow in the uid by setting CalendarData::Event::uniqueId to notebookUid+uid. This is a bit ugly to hide something in something else but it keeps the API untouched,
3. Assume the change of meaning of CalendarData::Event::uniqueId into something that is truely a unique identifier between events, exceptions and notebooks, and remove the recurrenceId from such API, just keeping this uniqueId that hold everything. Currently, it can simply be the instanceIdentifier() of an incidence, but later, we can extend it easily with the notebookUid.

The PR here investigate the consequences of option 3, namely dropping the recurrenceId argument from all API used to uniquely identify an event. It starts on #53 obviously since this PR is moving in that direction for the internal storage. In term of API changes, it means:

• CalendarEventQuery drops the recurrenceId property and simply uses the uniqueId one to identify an event. The startTime is kept to get a specific occurrence for recurring events.
• CalendarApi::remove() drops the recurrenceId argument.

These API changes introduce changes in three UI applications as far as my grepping capability is concerned:

• jolla-calendar, the largest changes, but quite strait forward (I've a PR ready to show it). It implies though a change in its DBus API for "viewEvent()" function. Once again grepping for it in "/usr/share" on device or in "/usr/lib64/qt5/qml" reveals only one match : the alarms, see later.
• jolla-email, a direct change not to pass the recurrenceId read from the invitation query since the uniqueId is now enough. So very simple.
• jolla-alarm, in /usr/share/jolla-alarm-ui/pages/CalendarAlarmDialog.qml. There, the pull up menu has an option to show the alarm in calendar, thus calling the viewEvent DBus function. The recurrenceId can be drop here.

This alarm stuff raise a transition issue: one can store the uniqueId in mKCal when setting the alarm, so it is available in the ringing dialog and one can pass it to viewEvent(). So far so good. But what happens to previously set alarms that were not defined with the uniqueId in mind ? What is good is that they already contain the three information, the notebookUid, the event uid and the recurrenceId. So it can be possible to reconstruct the uniqueId from these three info. But it's a bit ugly…

[pvuorela]

Overall I think it's nice having a single really unique identifier for the events. Just thinking should it be called something a bit different to reduce confusion with uid of an event, e.g. instanceId or something like that. The API and its usage need to be touched anyway and naming would be hopefully just mechanical search & replace.

No immediate solution for the alarms. Guess there could be some backward compatibility support, detecting how the alarm properties were set up. Or then keep supporting showing events based on uid+recid. In theory multiple notebooks could have the same event but out of sync where it would matter what notebook to use for showing, but then again it's strange even if they are in sync and there are alarms for each notebook separately. Or maybe that's what the user has ordered if the same thing is in multiple places.

[pvuorela]

Needs a rebase.

[pvuorela]

Needs a rebase.

[dcaliste]

Rebased. It seems I had included in this PR the initial commit of the manager fix on model destruction…

[pvuorela]

Getting still a bit confused with the names when there's uid / uniqueId, instanceId and incidenceId.

How about if we keep uid/unique id names reserved for the ICS event uid since it's so much steering the assumptions on the APIs. InstanceId already used in kf5-calendarcore so could be clearer if it meant only that, leaving incidenceId for the nemo-calendar to use. This latter one less much steering thinking as it's not in the spec, but since we can avoid the naming collision could do that.

If using names like this, would then need some additional adjustments to the qml api roles, and more code churn, but then again the usage is often needing adaption to the recurrenceId removal anyways?

[dcaliste]

Ok, I can understand the confusion. So I've rework this PR and the associated one to follow these rules:

• instanceId is the unique identifier that span over notebooks, incidences and exceptions. Currently, it is implemented as incidence->instanceIdentifier() but it will also include the notebookUid in the future.
• incidenceUid is the uid as returned by incidence->uid(). It is a bit useless, but still mandatory because of the dissociation code. Currently the dissociation based on instanceId fetch the parent and create an exception incidence. This incidence has the right uid. But it is converted to Data::Event to be passed back to the manager, and the incidence->uid() is then lost because event.instanceId is forged from exception->instanceIdentifier(). Then in save, the Data::Event is passed and the routine detect that it should be an exception, but don't know the proper parent uid if not saved (and I don't want to deserialise instanceId).
• recurrenceId is the exception tag, if any.
• calendarUid is the notebook uid.

Everywhere in the code, I've followed these naming conventions and changed public API and class members to reflect these.

Do you feel it better ?

[pvuorela]

Looked a lot easier to grasp now, thanks. Couple comments for apis with instanceid + recurrence id but no event uid.

[pvuorela]

Some small comments still.

One thing I'm still a bit thinking is how it goes when events were referred just by uid to e.g. load al the related data and then marked as such. Now there's some code for loading the main event instead of exception if instanceId is such, but then the loaded is marked by instanceId. Though it used to be a bit like that already. Maybe it's all good, but didn't wrap my head around it totally yet.

If you make changes, could rebase.

[pvuorela]

Is this proper? So coming eventually from calendareventmodification. Should be possible to have an exception on recurring exception (i.e. repeats and has recurrence id due to replacing something earlier)? The old code allowed such so should this get the parent event if recurring?

[dcaliste]

There is no support at the moment for recurring exceptions in KCalendarCore, as far as I know.

At the moment in the UI, there is no way to make an exception out of an exception. In the sense that a CalendarEventModification that call ::dissociateSingleOccurrence() can be called only when modifying a recurring parent and selecting "modify this occurrence only".

Previously, the code allowed to add an exception to a recurring parent (only the uid was fetch, without recurrenceId). So here the code is still doing the same, checking that the instanceId was not belonging to an exception.

That being said, to get the exact same behaviour (the caller is an exception, but only pass the uid), I can change for the following:

KCalendarCore::Incidence::Ptr event = mCalendar->instance(instanceId);
if (event && event->hasRecurrenceId()) {
    event = mCalendar->incidence(event->uid());
}
if (!event) {
    qWarning(...

I thought it was a bit verbose for a case that is not possible from UI design neither from internal KCalendarCore implementation.

[pvuorela]

At least we've disabled recurrence on the jolla-calendar side for exceptions, but I'm not sure anymore was that to keep it just simpler or were there deeper problems. But if we do e.g. that "modify this and later" to change time of a meeting it should be done by modifying the exception and needs repetition?

[dcaliste]

The this and later modifications, still didn't make in. But it's a good argument.

The implementation of the this and later does not require to dissociate from an exception. At least not the way it is implemented in KCalendarCore (it may be broken though, don't know). The exception is always dissociated from the parent. Actually, dissociation means internally "provide a recurrenceId that match one of the occurrence of the parent" and not one of the occurrence of the current this and later exception.

So let say you have a recurring parent P, recurring every day at noon.

• Then at one point in time, you decide that it should start recurring every day at 1pm. It means to create a non recurring clone of P and setRecurrenceId(noon on day D1), setThisAndFuture(true) and setDtStart(1pm on day D1).
• Later, you decide to move it every day at 11am. It means to create a non recurring clone of P and setRecurrenceId(noon on day D2), setThisAndFuture(true) and setDtStart(11am on day D2).

So the call to dissociateSingleOccurrence() must always be called with the instanceId of the parent.

[dcaliste]

One thing I'm still a bit thinking is how it goes when events were referred just by uid to e.g. load al the related data and then marked as such. Now there's some code for loading the main event instead of exception if instanceId is such, but then the loaded is marked by instanceId. Though it used to be a bit like that already.

Exactly, this is all the case already. What is currently (before this PR) happening is the following:

• mKCal expose a loadIncidenceByInstance(instanceId) that loads into memory the fully series containing instanceId.
• the worker is calling the former, passing instanceId and send the full series to the manager thanks to a loop on all the events in memory (there is a filtering done with the mSentEvents hash to avoid sending twice the same instance between calls).
• the manager is storing each instance of the series in a hash table indexed by instanceId.
• all current routines in the manager using a (uid, recId) tuple are creating the corresponding instanceId and are thus accessing the right CalendarData::Event.

This PR is only changing the last step, using directly the instanceId in its API instead of (uid, recId) tuples. As far as I know, there is no code that loads a parent incidence (alone) when passed an (exception) instanceId and flagging it with the exception instanceId. In the worker, the fully series is always loaded and then the CalendarData::Events are sent to the manager, indexed using their own instanceId.

[dcaliste]

I've just added a commit on top, following your comment on the dead code for event uid changes.

I'm not reactivating this dead code (yet) since there is no support in mKCal for it, but in this commit, I'm changing it to use instanceId instead of the old eventUid. That's why, I think you're right, and this could go with this PR.

If it's adding a burden to the review process, I can remove it and keep it for later, to go with the mKCal upgrade. As you prefer.

[dcaliste]

Rebased on latest master.

[pvuorela]

I think this is good to go in. Thanks for your efforts!