[nemo-qml-plugin-calendar] Allow multi-notebook when importing. Contributes to JB#60800 #59
Conversation
dcaliste
force-pushed
the
import
branch
3 times, most recently
from
2f0037c
to
2bf0ac6
Compare
dcaliste
changed the title
|
I've quickly tested it and it seems that nothing is broken wrt. importation in the UI (after some slight adjustments of course). It's still not possible to actually test that the duplication warning disappear when we choose a different notebook because of the ExtendedCalendar backend, but at least the code should be ready for the capability when available in the backend. |
src/calendarutils.cpp
Outdated
| @@ -53,7 +53,6 @@ CalendarData::Event::Event(const KCalendarCore::Event &event) | |||
| , startTime(event.dtStart()) | |||
| , endTime(event.dtEnd()) | |||
| , allDay(event.allDay()) | |||
| , instanceId(event.instanceIdentifier()) | |||
There was a problem hiding this comment.
I'm not sure about the first commit doing for example this. There's nothing specified in the calendar data that the instanceId would define ownerships, only about identifying an event across calendars. Also there's occurrence getId() which would give strange output if there's no instance identifier, so in a way there's some implicit requirement for it to be set, right?
There was a problem hiding this comment.
First of all, this first commit is not mandatory. I can spin it off so we can discuss its relevance on its own.
There's nothing specified in the calendar data that the instanceId would define ownerships, only about identifying an event across calendars.
That's indeed the issue : it should identify event across notebooks, but when in the import model, we're creating CalendarData::Event structures that does not belong to any notebook. We're creating CalendarImportEvent from such data structures, which is fine, because such objects does not inherit from CalendarStoredEvent, only CalendarEvent. But CalendarImportEvent can create CalendarEventOccurrence objects when calling nextOccurrence(). And these are plain occurrences, which gives access to methods like eventObject(). Which should give access to the initial CalendarImportEvent, but which will not because it is made to ping the CalendarManager, using the instanceId.
Luckily, at UI level, all code has been duplicated in ImportPage.qml not to use the standard delegate to avoid the eventObject() call.
So this first commit is an attempt to be cleaner about what this instanceId is and what it is used for: it is an incidence identifier for stored incidences in the manager (sorting out exception and notebooks).
As far as I know, there are two places where a CalendarData::Event structure is built from a KCalendarCore::Incidence:
- the calendar worker, and there, I'm properly setting the instanceId since it is pushing these data to the manager only.
- the import case to transiently show the ICS data to the user. In that case, previously the instanceId should not match anything in the manager and
eventObject()from occurrences will report a null pointer. In the new proposed way, the instanceId is empty which will make alsoeventOject()to return a null pointer from the manager (but this code path is not used in the UI).
I put this commit in the PR with the change in importation workflow because it is (slightly) related in my opinion. Going further, and make eventObject() being able to return the CalendarImportEvent the occurrence was created from and not interfere with the manager, is something that remains to be done. It may allow to reduce the code duplication in the UI in the Import*.qml pages.
There was a problem hiding this comment.
If it's not required, could make sense to split out or move into other PR to get the import changes in.
I'm not totally against having instanceId empty on these detached events, but at least the member variable should then document this detail. Maybe getId() could do a warning too if called on instance which doesn't have identifier?
Though not sure should the change be in the qml model side to have special handling on properties on import case or would this be more topical when the identifier is changed to include notebooks.
There was a problem hiding this comment.
Ok, I've removed the first commit and will keep it in #60. I totally makes sense there also, since on cannot anymore set the instanceId only from a KCalendarCore::Incidence in the CalendarData::Event constructor since we will need the notebook uid there.
…ibutes to JB#60800 When two events in different notebooks can share the same UID, the dedection of duplicates during the importation must be done per notebook and not globally when parsing the ICS data.
and also:
@pvuorela, this sadly implies to change the API of calendarimportmodel. Indeed, the duplication must be detected when selecting a destination notebook, and changing this destination should rerun the duplication check. That's why I created a new property setting the notebookuid. It's still WIP since I need to create the corresponding PR for the UI.