GNOME Bugzilla – Bug 657808
Copy/move of a single instance should grab whole series
Last modified: 2013-11-04 20:24:01 UTC
Sequence: 1. Create a recurring meeting on an EWS account. 2. Move a single occurrence of the meeting to the local calendar. 3. Refresh. Expected: The meeting should appear only in the local calendar. Actual: The occurrence appears both on the local calendar. All other occurrences appear on both EWS calendar and on the local calendar.
I see below warning on e-calendar-factory terminal while following the steps (e-calendar-factory:11052): libecal-CRITICAL **: e_cal_util_remove_instances: assertion `mod != CALOBJ_MOD_ALL' failed
I have a similiar problem here, using ews 3.9.90. Firstly, looks like we have no option to move a single instance of a meeting to another calendar, and it is one problem. Another one is when we move the instance(s), once all occurrences will be move too, only the selected occurrence disappears.
Created attachment 252844 [details] [review] Bug #657808 - Moving a single occurrence to a different calendar causes duplication of other occurrences. We are only supporting to move/copy *all* *instances* of a recurring event to another calendar.
Review of attachment 252844 [details] [review]: No no no, this is not correct. a) It's not evolution's fault that evolution-ews cannot remove single instances (by the way, it can [1][2]) b) When you copy only one instance, but then remove all occurrences, then you cause data loss The [1][2] is how you store recurrence exceptions (it's called that way in libical) [1] http://msdn.microsoft.com/en-us/library/aa580978%28v=exchg.140%29.aspx [2] http://msdn.microsoft.com/en-us/library/aa563731%28v=exchg.140%29.aspx
Review of attachment 252844 [details] [review]: After a chat with Fidencio on IRC, whom noted me with couple things and clarified some other things, I see the patch is correct. The code copied/moved whole meeting, but tried to remove only one instance. thinking of it the copy/move operation (either through popup menu or by drag&drop) should always move all instances with its master object. It doesn't make much sense to copy/move only a single instance. Thus please commit. I'm sorry for misread on my side. By the way, it would be good if you could test before the commit whether both operations (popup menu and drag&drop) behaves the same. Just in case.
Milan, Is not so easy to handle with the drag&drop part. According to my understand of the code, we don't have access to the source E{,Cal}Client from the calendar_selector_data_dropped() function, as you can check in: https://git.gnome.org/browse/evolution/tree/calendar/gui/e-calendar-selector.c#n167 Without this, is a bit hard to handle recurring events properly. The current code is not handling recurring events neither the "Move" action. IMHO, the best thing to do for now would be disable the drag&drop from one calendar to another and force the user to use the popup menu. Please, let me know your thoughts.
You might consider using GDK_ACTION_ASK for this situation, since defaulting to one way or the other is going to be wrong in the eyes of someone. I'm a little rusty on the whole drag-and-drop protocol at the moment, so I can't recite the entire chain of events from memory, but I do know that GDK_ACTION_ASK is purposed for just this kind of ambiguous situation.
(In reply to comment #7) > You might consider using GDK_ACTION_ASK for this situation, since defaulting to > one way or the other is going to be wrong in the eyes of someone. > > I'm a little rusty on the whole drag-and-drop protocol at the moment, so I > can't recite the entire chain of events from memory, but I do know that > GDK_ACTION_ASK is purposed for just this kind of ambiguous situation. Matthew, from my point of view, the problem is not let the decision to the user, what could be solved using GDK_ACTION_ASK. The problem is how to get the proper information of the recurring event from the ECalClient source and then do what we need to do with the icalcomponent.
OK, so, only Day View and Work Week View (which is internally implemented as a Day View) supports drag & drop currently (to be changed one day, but that's another question). The day view, in e_day_view_on_drag_data_get(), encodes dragged component as a string with notation: <source-uid>\n<icalstring> thus yours calendar_selector_data_dropped() can decode from it the source UID, and decide all what is necessary based on it. The best if you avoid code duplication as much as possible and pass the actual copy-or-move into the same functions as the context menu options, the same with a dealing of recurrent events.
Created attachment 253332 [details] [review] EDS: Bug #657808 - Copy/move of a single instance should grab whole series
Created attachment 253333 [details] [review] Evo: Bug #657808 - Copy/move of a single instance should grab whole series We are only supporting to move/copy *all* *instances* of a recurring event to another calendar.
Review of attachment 253332 [details] [review]: Let's do the change in evolution only. I agree it might be useful in general, but for now, in this time, let's touch evo only. ::: calendar/libecal/e-cal-client.c @@ +6730,3 @@ + + if (e_cal_client_get_timezone_sync (ftd->source_client, tzid, &tz, NULL, NULL) && tz) + e_cal_client_add_timezone_sync (ftd->destination_client, tz, NULL, NULL); pass GError and GCancellable in these too, or at least the GCancellable. I'm wondering whether it makes sense to abort the operation (not application), if any of the two fails. @@ +6750,3 @@ + ECalClient *dest_client, + icalcomponent *icalcomp_vcal, + gboolean remove) add also GCancellable and GError parameters and use them below as necessary; I would name "rename" as "do_copy". Seems to me better. @@ +6767,3 @@ + uid = icalcomponent_get_uid (icalcomp_event); + + success = e_cal_client_get_object_sync (dest_client, uid, NULL, &icalcomp, NULL, NULL); does this check make sense if you copy the event? you generate a new UID anyway. @@ +6792,3 @@ + } else { + /* ... or remove the recurrence ID ... */ + icalcomp_clone = icalcomponent_new_clone (icalcomp_event); the icalcomp_event is supposed to come from src_client. if you cannot find it there then something failed horribly. @@ +6819,3 @@ + if (!success) { + icalcomponent_free (icalcomp_clone); + g_warning ("%s: Failed to create object: %s", G_STRFUNC, error->message); once you have the parameters, there will be no g_warning @@ +6836,3 @@ + mod_type = CALOBJ_MOD_ALL; + + e_cal_client_remove_object_sync (src_client, uid, NULL, mod_type, NULL, NULL); this can also fail @@ +6839,3 @@ + } + + return TRUE; return success; though I see you get here only if the success is true.
Review of attachment 253333 [details] [review]: ::: calendar/gui/e-calendar-selector.c @@ +96,1 @@ + success = e_cal_client_transfer_item_to (E_CAL_CLIENT (src_client), E_CAL_CLIENT (dest_client), icalcomp, remove); because the operation can take some time, and because this is done in the main thread, and because you have the data already, let's define an async version for this operation as well and use it here. Let's add the functions into calendar/gui/comp-util.c/.h. Also remember, this can be used with all vEvent, vJournal and vTodo components, and the resulting vCalendar can contain both data components and timezones, thus when you get the inner component, then make sure you are working with data component, not with a timezone (this is more for the e_cal_client_transfer_item_to() implementation).
(In reply to comment #12) > Review of attachment 253332 [details] [review]: I see the most things I write in there are in the current code and you only copy&pasted the code to an outer function. I hope you do not mind to change the things. And/or feel free to correct me, if I'm wrong with any of them.
Created attachment 253610 [details] [review] Bug #657808 - Copy/move of a single instance should grab whole series *TODO: - check for a cancelled GCancellable/previous error on add_timezone_to_cal_cb() and cancel the operation of it occurs - use _transfer_item_to() instead of _transfer_item_to_sync() - crete and EActivity, set a GCancellable on it, add this into EShellSomething and use it forward
Review of attachment 253610 [details] [review]: (due to TODO, I'm setting this to needs-work.) For an inspiration on the EActivity thing feel free to check usage of https://git.gnome.org/browse/evolution/tree/modules/calendar/e-cal-shell-view-private.c#n1576 ::: calendar/gui/comp-util.c @@ +862,3 @@ + + if (e_cal_client_get_timezone_sync (ftd->source_client, tzid, &tz, ftd->cancellable, ftd->error) && tz) + e_cal_client_add_timezone_sync (ftd->destination_client, tz, ftd->cancellable, NULL); it's the opposite, we care of the error of the e_cal_client_add_timezone_sync, not of the e_cal_client_get_timezone_sync call. Do not forget to store the last error state (whether it occurred) and do not do anything from this if it did (remember of GError overwrite runtime warnings). I imagine this as some boolean in the 'ftd' struct. @@ +965,3 @@ + icalcomp_event = icalcomponent_get_inner (icalcomp_vcal); + g_return_val_if_fail (icalcomp_event != NULL, FALSE); + g_return_val_if_fail (icalcomponent_isa (icalcomp_event) != ICAL_VEVENT_COMPONENT, FALSE); why is there this limit? I would like to use the same code in memos and tasks as well. @@ +974,3 @@ + return FALSE; + + icalcomp_clone = icalcomponent_new_clone (icalcomp); there is no need for this clone, just replace all 'icalcomp_clone' with 'icalcomp'. @@ +994,3 @@ + + icalcomponent_foreach_tzid (icalcomp_clone, add_timezone_to_cal_cb, &ftd); + if (error && *error) once you have the error indicator in the ftd struct you'll use it here too (if the caller of the function passes NULL for GError, then you will pretend that nothing wrong happened, which is not correct). ::: calendar/gui/e-calendar-selector.c @@ +26,3 @@ #include <libecal/libecal.h> + added empty line? @@ +81,3 @@ + segments = g_strsplit ((const gchar *) data, "\n", 2); + source_uid = g_strdup (segments[0]); + icalcomp = icalparser_parse_string (segments[1]); check here, whether you really received two items, otherwise the segments[1] can lead to a segv crash. @@ +104,2 @@ + if (source) + g_object_unref (source); free also the icalcomp, if not NULL.
Created attachment 253918 [details] [review] Add EShellView to E{Calendar,MemoList,TaskList}Selector
Created attachment 253919 [details] [review] Bug #657808 - Copy/move of a single instance should grab whole series
Milan, I'm getting the following error during the delete operation: "Cannot retrieve calendar object path: Object not found" The most curious thing is that the operation works properly, and I can see that it worked properly also checking some webcalendar. Do you have some tip?
Ah, we also could use the async version of e_client_selector_get_client_sync(). I really don't know how is the real gain, but let me know if you prefer.
Created attachment 253957 [details] [review] Bug #657808 - Copy/move of a single instance should grab whole series
Review of attachment 253918 [details] [review]: Looks good, feel free to commit. Also note of g_clear_object(), it can be used in those dispose functions.
Review of attachment 253957 [details] [review]: Let me know, whether the comments make sense. ::: calendar/gui/comp-util.c @@ +858,3 @@ + return; + + if (ftd->cancellable && g_cancellable_is_cancelled (ftd->cancellable)) { do this before tzid check, and add a test for (!ftd->success) @@ +970,3 @@ + g_return_val_if_fail (icalcomp_event != NULL, FALSE); + + uid = icalcomponent_get_uid (icalcomp_event); 'uid' should be read inside the 'for' @@ +988,3 @@ + + for (; + icalcomp_event != NULL; the first inner component can be of a different type than icalcomp_kind @@ +990,3 @@ + icalcomp_event != NULL; + icalcomp_event = icalcomponent_get_next_component (icalcomp_event, icalcomp_kind)) { + you should check if the respective uid was already processed or not (which can happen when multiple instances of one recurring event are selected; use GHashTable). @@ +991,3 @@ + icalcomp_event = icalcomponent_get_next_component (icalcomp_event, icalcomp_kind)) { + + if (e_cal_client_get_object_sync (dest_client, uid, NULL, &icalcomp, cancellable, error)) { I believe this call is causing the error in UI; you should: a) use local_error for it b) check if the returned error is E_CAL_CLIENT_ERROR_OBJECT_NOT_FOUND, and if not, then propagate the local_error into error and stop processing further (equivalent of other failures). c) still, this particular check (get_object_sync call) should be done only if you do_copy, because otherwise the UID is regenerated later anyway) ::: calendar/gui/e-calendar-selector.c @@ +95,3 @@ + priv->transfer_alert = alert; + g_object_add_weak_pointer (G_OBJECT (alert), &priv->transfer_alert); + e_alert_start_timer (priv->transfer_alert, 5); the alert is hidden quite quickly here, I almost didn't have time to read it. I guess that these error alerts can be shown longer, say about 5 minutes (if I recall correctly, mailer has some such timer for errors, and other one for warnings). @@ +297,3 @@ + message = do_copy ? + g_strdup_printf (("Copying an event into the calendar %s"), display_name) : + g_strdup_printf (("Moving an event into the calendar %s"), display_name); hmm, new strings to be localized, right? ::: modules/calendar/e-cal-shell-view-private.c @@ +1024,3 @@ + priv->transfer_alert = alert; + g_object_add_weak_pointer (G_OBJECT (alert), &priv->transfer_alert); + e_alert_start_timer (priv->transfer_alert, 5); enlarge the timer, as earlier
Created attachment 253987 [details] [review] Bug #657808 - Copy/move of a single instance should grab whole series
Review of attachment 253987 [details] [review]: The rest of the patch looks OK to me (I commented on one file only). With respect of detached instances (mentioned below), try this: a) create a recurring event in a local calendar, which will have 5 occurrences b) edit the third instance and change its Summary + start time (can be only one of them, but do both) c) delete the fourth instance And now, copy the event into another local calendar. See the copied events are at the same dates, only the detached instance from b) is not shown properly, it shows the same summary and start time as the master object. ::: calendar/gui/comp-util.c @@ +853,3 @@ + g_return_if_fail (ftd->source_client != NULL); + g_return_if_fail (ftd->destination_client != NULL); + g_return_if_fail (*ftd->success == TRUE); not as a runtime warning, if you have multiple events, with multiple timezones, and it fails with the first timezone, the second+ timezone will have set success to FALSE, which is "expected", because you cannot cancel the loop in any other way. @@ +866,3 @@ + *ftd->success = e_cal_client_get_timezone_sync (ftd->source_client, tzid, &tz, ftd->cancellable, NULL); + if (*ftd->success && tz) + *ftd->success = e_cal_client_add_timezone_sync ( I do not think it's necessary to do *ftd->success, it's easier to not use 'gboolean *success' in the structure. Also, it's not a problem (basically) if you do not get the timezone form the source client, but it's a problem when it failed to add the timezone to the destination client. @@ +988,3 @@ + } + + processed_uid = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); just a little tiny thing, I would call it 'processed_uids' (plural form of 'uid') @@ +1005,3 @@ + uid = icalcomponent_get_uid (icalcomp_event); + + if (g_hash_table_lookup (processed_uid, uid)) there is one thing I did not think of, detached instances (see the main description). @@ +1011,3 @@ + if (success) { + success = e_cal_client_modify_object_sync ( + dest_client, icalcomp, CALOBJ_MOD_ALL, cancellable, &local_error); this doesn't make much sense to me: a) if it fails, then the local_error is leaked b) you are writing the same component you've just got from the calendar, which is equivalent to a no-op Why is here this branch at all? As we spoke on IRC, my initial idea of not calling e_cal_client_get_object_sync() always was wrong, because when one considers also multiselect, and that events from multiple calendars can be selected, then if the source uid and the destination uid of the calendar (not component) matches, then the change should be a no-op, because the event is already there, either copied, or moved. Removing it would lead to a data loss. Thinking of it, the current implementation expects only one calendar (as you mentioned on IRC), thus let's deal with multiselect when it is fully supported by views. @@ +1041,3 @@ + + if (e_cal_util_component_is_instance (icalcomp_event)) { + success = e_cal_client_get_object_sync (src_client, uid, NULL, &icalcomp, cancellable, error); do you get a vCalendar here, when the event has detached instances? If yes, then the below new_uid set will not work properly. @@ +1064,3 @@ + + icalcomponent_foreach_tzid (icalcomp, add_timezone_to_cal_cb, &ftd); + if (!success) use ftd.success here (as you did in the previous version of the patch). Also, icalcomp is not freed here.
Comment on attachment 253918 [details] [review] Add EShellView to E{Calendar,MemoList,TaskList}Selector Attachment 253918 [details] pushed as 96c6e7b - Add EShellView to E{Calendar,MemoList,TaskList}Selector
Created attachment 258952 [details] [review] updated evo patch for evolution; This is all the same as the previous fidencio's patch only the body of cal_comp_transfer_item_to_sync() contains fixes for the concerns from the previous review and other fixes with enhancements to support detached instances and all about it.
Created commit ed2bc85 in evo master (3.11.2+) Created commit 56db847 in evo gnome-3-10 (3.10.2+)