Report when events already exist in db on importation #7
Conversation
src/calendarimportmodel.cpp
Outdated
| const KCalendarCore::Incidence::Ptr old = | ||
| mStorage->calendar()->incidence(incidence->uid(), incidence->recurrenceId()); | ||
| if (old) { | ||
| // Unconditionnaly overwrite existing incidence with the same UID/RecID. |
There was a problem hiding this comment.
Sorry, thanks for the notice.
src/calendarimportmodel.h
Outdated
| @@ -54,6 +55,7 @@ class CalendarImportModel : public QAbstractListModel | |||
| EndTimeRole, | |||
| AllDayRole, | |||
| LocationRole, | |||
| DuplicateRole, | |||
There was a problem hiding this comment.
maybe put this one after the UidRole
| if (!mEventList.isEmpty()) | ||
| mEventList.clear(); | ||
| mEventList = cal->events(KCalendarCore::EventSortStartDate); | ||
| for (const KCalendarCore::Event::Ptr &event : mEventList) { |
There was a problem hiding this comment.
Not really important, but I think this will detach due to ranged-for semantics, because mEventList isn't a const ref.
There was a problem hiding this comment.
Yes, we need a qAsConst here, but it deosn't exist yet in Qt5.6… I left it like that for simplicity, the size of the list is the number of imported events, so it is small. And the list content are QSharedPointers, so the detach should be cheap : no copy will be done.
I've added a comment for future upgrade though.
|
Can you please add "Contributes to JB#55786" to the commit message also? |
|
I've added the JB id to the commit message. We also wondered on IRC what happened before the PR if the added event had a revision strictly higher than the already existing event. After checking in mKCal, I'm sure there is no treatment of the revision. You can add an event sharing the same UID and RecID to a calendar that has not been populated with the DB content, and then when saving, whatever the revision of your event, the operation will fail because of UNIQUE constraint not being valid. |
|
Didn't test now but seems good to me. |
| const KCalendarCore::Event::Ptr old = | ||
| mStorage->calendar()->event(event->uid(), event->recurrenceId()); | ||
| if (old) { | ||
| mDuplicates.insert(old->instanceIdentifier()); |
There was a problem hiding this comment.
mDuplicates should be emptied somewhere in this method?
There was a problem hiding this comment.
Good catch, I'll update the MR. Thanks.
…butes to JB#55786
|
LGTM |
Add a "duplicate" role to the import model to be able to flag events that already exist in the database, based on their UID.
Also push two simplification commits:
KCalendarCore::Calendar::events()routine.@pvuorela and @chriadam, the root of this MR comes from various discussions in this thread on the forum : https://forum.sailfishos.org/t/bug-4-2-0-21-cannot-import-calendar-entries-via-ics-file/7988/13 The overall goal would be to give a better feedback to the user of what is going on during importation. At the moment, importing twice an event with a UID will result in an importation failed, without explaining to the user what happened. What I'm proposing instead of just a detailed error is to warn the user of possible existing event in the database and let the user overwrite them if they choose to.
This PR requires sailfishos/mkcal#7 since, I'm using the same DEL+ADD pattern as in KCalendarCore.