After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696663 - Save without close should update comp-editor
Save without close should update comp-editor
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Calendar
3.8.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-calendar-maintainers
Evolution QA team
Depends on: 559710
Blocks:
 
 
Reported: 2013-03-26 22:52 UTC by Fabiano Fidêncio
Modified: 2013-05-22 12:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Bug #696663 - Error to modify a newly saved event (2.24 KB, patch)
2013-03-26 23:46 UTC, Fabiano Fidêncio
needs-work Details | Review
Error (99.49 KB, image/png)
2013-03-26 23:47 UTC, Fabiano Fidêncio
  Details
Bug #696663 - Save without close should update comp-editor (2.59 KB, patch)
2013-04-05 19:34 UTC, Fabiano Fidêncio
needs-work Details | Review
Bug #696663 - Save without close should update comp-editor (3.01 KB, patch)
2013-04-09 21:57 UTC, Fabiano Fidêncio
reviewed Details | Review
Bug #696663 - Save without close should update comp-editor (4.67 KB, patch)
2013-04-10 09:01 UTC, Fabiano Fidêncio
none Details | Review
Bug #696663 - Save without close should update comp-editor (4.49 KB, patch)
2013-04-10 09:31 UTC, Fabiano Fidêncio
committed Details | Review

Description Fabiano Fidêncio 2013-03-26 22:52:27 UTC
To reproduce:
1 - Add an event in an EWS calendar
2 - Fill the event as you want
3 - Save the event (Save only, NOT Save and Close)
4 - Change something in the event
5 - Click in Save again
Comment 1 Fabiano Fidêncio 2013-03-26 23:46:27 UTC
Created attachment 239900 [details] [review]
Bug #696663 - Error to modify a newly saved event
Comment 2 Fabiano Fidêncio 2013-03-26 23:47:17 UTC
Created attachment 239901 [details]
Error
Comment 3 Matthew Barnes 2013-03-27 00:00:45 UTC
Review of attachment 239900 [details] [review]:

The e_cal_client_get_object_sync() could potentially lock the UI while attempting to save, but I imagine CompEditor is guilty of that in numerous other places already.  If Milan's okay with it, I could let it slide for now.

At some point we need to add an EActivityBar to CompEditor and submit data asynchronously like the mail composer does.  Nice little chore for someone.
Comment 4 Milan Crha 2013-03-27 08:04:12 UTC
(In reply to comment #3)
> At some point we need to add an EActivityBar to CompEditor and submit data
> asynchronously like the mail composer does.  Nice little chore for someone.

Unfortunately true, calendar is full of UI-blocking calls. I plan to do something with it, including editors rewrite (for example, there was a plan to use tabs instead of popup dialogs, which sounds interesting, from my point of view. And that way, I'll be able to kill few more code duplications.)
Comment 5 Milan Crha 2013-03-27 08:16:16 UTC
Review of attachment 239900 [details] [review]:

::: calendar/gui/dialogs/comp-editor.c
@@ +1009,3 @@
 
+	update_after_save = !cal_comp_is_on_server (
+			priv->comp, priv->source_client);

This is not needed, it blocks UI too, and is done within save_comp().

@@ +1053,3 @@
+
+		if (!update_after_save)
+			return;

Do the below only if (!can_close), all other cases are useless, because the editor is going to close itself.

@@ +1056,3 @@
+
+		/*
+		 * EWS calendar searches for icalcomponent property with ItemID (the

EWS is not the only reason, as I mentioned on IRC, Google calendar (through CalDAV) can modify the component on save too, like adding a default alarm to it. Thus the reason is "A server can modify event on save, fetch the updated version..."

@@ +1065,3 @@
+				priv->cal_client,
+				uid,
+				NULL,

This is getting complicated with recurrences. This way you get the master object with detached instances, not the instance user is editing. Note the rid == NULL can be correct, because other instances are generated on demand with e_cal_client_generate_instances[_sync]().

@@ +1069,3 @@
+				NULL,
+				NULL);
+		e_cal_component_set_icalcomponent (priv->comp, icalcomp);

a) check for error, what if the fetch fails?
b) even the e_cal_component_set_icalcomponent() can fail (it's with detached instances, the component will be of type VCALENDAR, not VEVENT/VTODO/VJOURNAL)
c) after updating internal component you might want to update editor as well, with all its pages, thus the values shown in the editor reflect the component (remember the alarm added from Google, the event at view will have a bell icon, while the component editor will not show any alarm).
Comment 6 Fabiano Fidêncio 2013-03-27 08:46:48 UTC
(In reply to comment #5)
> Review of attachment 239900 [details] [review]:
> 
> ::: calendar/gui/dialogs/comp-editor.c
> @@ +1009,3 @@
> 
> +    update_after_save = !cal_comp_is_on_server (
> +            priv->comp, priv->source_client);
> 
> This is not needed, it blocks UI too, and is done within save_comp().

Hmmm. How is the better way to check if the event is new?

> 
> @@ +1053,3 @@
> +
> +        if (!update_after_save)
> +            return;
> 
> Do the below only if (!can_close), all other cases are useless, because the
> editor is going to close itself.

My bad. It was there, but cleaning up the patch before send I just did this mistake :-\

> 
> @@ +1056,3 @@
> +
> +        /*
> +         * EWS calendar searches for icalcomponent property with ItemID (the
> 
> EWS is not the only reason, as I mentioned on IRC, Google calendar (through
> CalDAV) can modify the component on save too, like adding a default alarm to
> it. Thus the reason is "A server can modify event on save, fetch the updated
> version..."

Duh, right.

> 
> @@ +1065,3 @@
> +                priv->cal_client,
> +                uid,
> +                NULL,
> 
> This is getting complicated with recurrences. This way you get the master
> object with detached instances, not the instance user is editing. Note the rid
> == NULL can be correct, because other instances are generated on demand with
> e_cal_client_generate_instances[_sync]().

Right.

> 
> @@ +1069,3 @@
> +                NULL,
> +                NULL);
> +        e_cal_component_set_icalcomponent (priv->comp, icalcomp);
> 
> a) check for error, what if the fetch fails?

Kk, check for errors..

> b) even the e_cal_component_set_icalcomponent() can fail (it's with detached
> instances, the component will be of type VCALENDAR, not VEVENT/VTODO/VJOURNAL)

Kk, same here.

> c) after updating internal component you might want to update editor as well,
> with all its pages, thus the values shown in the editor reflect the component
> (remember the alarm added from Google, the event at view will have a bell icon,
> while the component editor will not show any alarm).

Kk.
Comment 7 Matthew Barnes 2013-03-27 11:24:36 UTC
(In reply to comment #4)
> Unfortunately true, calendar is full of UI-blocking calls. I plan to do
> something with it, including editors rewrite (for example, there was a plan to
> use tabs instead of popup dialogs, which sounds interesting, from my point of
> view. And that way, I'll be able to kill few more code duplications.)

Yes!  The CompEditor design for Express Mode was the main thing I wanted to salvage before axing Express Mode.
Comment 8 Milan Crha 2013-03-28 09:00:50 UTC
(In reply to comment #6)
> Hmmm. How is the better way to check if the event is new?

The comp-editor has a flag for it, COMP_EDITOR_NEW_ITEM, but I do not thing it's necessary to check whether the item is new. If we'll expect that any server upload can modify the event, then the "Save without close" can safely refetch the item from the backend before processing further (returning to editor).
Comment 9 Fabiano Fidêncio 2013-04-05 19:34:35 UTC
Created attachment 240788 [details] [review]
Bug #696663 - Save without close should update comp-editor
Comment 10 Milan Crha 2013-04-08 11:11:57 UTC
Review of attachment 240788 [details] [review]:

Thanks for the update. I tried this with a Google calendar and I got a crash. Below is the reason. If I fix the crash, then I see the component editor is properly updated, which is nice.

::: calendar/gui/dialogs/comp-editor.c
@@ +1042,3 @@
+			const gchar *uid = NULL;
+			gchar *rid = NULL;
+			GError *error;

'error' should be initialized to NULL, otherwise one gets a critical warning like this: GLib-GIO-CRITICAL **: g_dbus_proxy_call_sync_internal: assertion `error == NULL || *error == NULL' failed

@@ +1047,3 @@
+
+			/*
+			 * A server can modify the event on save. Considering this, is needed to fetch the updated

...it is needed...

@@ +1048,3 @@
+			/*
+			 * A server can modify the event on save. Considering this, is needed to fetch the updated
+			 * version of the event from server, updating the component, than user can keep editing the

s/than/then/

@@ +1065,3 @@
+				GtkWidget *dialog;
+
+				dialog = gtk_message_dialog_new (

Please use e_notice(), with comp_editor's window as a parent.

@@ +1070,3 @@
+					GTK_BUTTONS_OK,
+					"%s",
+					_("Unable to edit the current version! Please, close and reopen the event"));

It'll be good to show the returned error too, thus user knows what failed. Apart of usual OBJECT_NOT_FOUND, the error can be basically anything, like the server not being available now (in offline), and so on. I would use message like this:

"Unable to retrieve saved component from the calendar, returned error was: %s"

It is tricky, because the priv->cal_client can be a task list or memo list too, not only calendar.

@@ +1073,3 @@
+				gtk_dialog_run (GTK_DIALOG (dialog));
+				gtk_widget_destroy (dialog);
+			}

the 'error' is leaking

@@ +1075,3 @@
+			}
+
+			comp = e_cal_component_new ();

why to do this, if 'error != NULL'?

@@ +1085,3 @@
+
+				comp_editor_edit_comp (editor, comp);
+			}

the created 'comp' is leaking here.
Comment 11 Fabiano Fidêncio 2013-04-09 21:57:44 UTC
Created attachment 241101 [details] [review]
Bug #696663 - Save without close should update comp-editor
Comment 12 Milan Crha 2013-04-10 06:14:50 UTC
Review of attachment 241101 [details] [review]:

Nice, it works (I tested a single instance with my Google calendar). I guess it's just about the below things and we are done here.

::: calendar/gui/dialogs/comp-editor.c
@@ +1083,3 @@
+					GTK_WINDOW (editor),
+					GTK_MESSAGE_ERROR,
+					_("Unable to retrieve saved component from the %s, returned error was: %s"),

While this is localized, the 'source' is not. In any case, doing this sentence split is suboptimal, and it's a horror for translators, thus please make three "almost the same" sentences above, localized in once.

@@ +1087,3 @@
+					error->message);
+				g_clear_error (&error);
+			} else {

if one wants to be paranoid, then it checks for icalcomp != NULL too

@@ +1089,3 @@
+			} else {
+				comp = e_cal_component_new ();
+				if (e_cal_component_set_icalcomponent (comp, icalcomp)) {

if this fails, then icalcomp will leak, otherwise it's eaten by the 'comp'.
Comment 13 Fabiano Fidêncio 2013-04-10 09:01:19 UTC
Created attachment 241127 [details] [review]
Bug #696663 - Save without close should update comp-editor
Comment 14 Fabiano Fidêncio 2013-04-10 09:31:31 UTC
Created attachment 241130 [details] [review]
Bug #696663 - Save without close should update comp-editor
Comment 15 Milan Crha 2013-04-10 11:23:05 UTC
Review of attachment 241130 [details] [review]:

Looks good, please fix the below and commit to master.

::: calendar/gui/dialogs/comp-editor.c
@@ +1062,3 @@
+						case (E_CAL_CLIENT_SOURCE_TYPE_TASKS):
+							msg = g_strdup_printf (
+								  _("Unable to retrieve saved component from the memo list, returned error was: %s"),

this is not a memo list :)
Comment 16 Milan Crha 2013-04-11 11:56:24 UTC
I see this was committed to sources as 5a389fa7bd736 (3.9.1+). Do not forget to update the bug report too.