Hi,
In general, the patch is OK. However, there are a few more issues
than what the patch fixes, which are related, and I've gone ahead
and fixed in my tree anyway. Some annotated comments are below.
On Hën , 2004-03-22 at 15:40 -0500, William Jon McCann wrote:
> Index: src/evolution-webcal-main.c
> ===================================================================
> RCS file: /cvs/gnome/evolution-webcal/src/evolution-webcal-main.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 evolution-webcal-main.c
> --- src/evolution-webcal-main.c 9 Mar 2004 05:42:47 -0000 1.3
> +++ src/evolution-webcal-main.c 22 Mar 2004 20:27:40 -0000
> @@ -35,12 +35,15 @@ static void e_webcal_load (const gchar *
>
> comp = icalparser_parse_string (body);
> if (comp == NULL) {
> + SoupUri * suri;
> gchar * message;
>
> + suri = soup_uri_new (uri);
> message = g_strdup_printf (_("There was an error parsing the calendar, "
> "\"%s\". Please verify that it is a valid "
> "calendar, and try again."),
> - g_basename (uri));
> + g_basename (suri->path));
> + soup_uri_free (suri);
>
> e_webcal_display_error (_("Error Parsing Calendar"),
> message,
While this is fine, and basically the same in my tree, g_basename is
deprecated (which I just realized), so we need to create a new string
here, and free it as well, and use g_path_get_basename. I also prefer
more descriptive variable names, so I changed it to tmpuri, and the
new string is tmpname.
> @@ -140,11 +150,17 @@ static void e_webcal_open_cal_http (cons
> message = soup_message_new (SOUP_METHOD_GET, uri);
> if (!SOUP_IS_MESSAGE (message)) {
> gchar * errorstring;
> + gchar * location;
> + SoupUri * suri;
>
> - errorstring = g_strdup_printf (_("The URI \"%s\" is invalid."), olduri);
> + suri = soup_uri_new (olduri);
> + location = g_strdup_printf ("%s%s", suri->host, suri->path);
> + soup_uri_free (suri);
> + errorstring = g_strdup_printf (_("The URI \"%s\" is invalid."), location);
> e_webcal_display_error (_("Invalid URI Specified"),
> errorstring,
> NULL);
> + g_free (location);
> g_free (errorstring);
> g_free (olduri);
> bonobo_main_quit ();
I also prefer that items that are created at the same time, are also
destroyed at the same time, and in the reverse order of their creation.
It generally makes the code easier to read, and it's easier to add stuff
in the middle that might need one or more of the variables, in the
future.
> Index: src/evolution-webcal-notify.c
> ===================================================================
> RCS file: /cvs/gnome/evolution-webcal/src/evolution-webcal-notify.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 evolution-webcal-notify.c
> --- src/evolution-webcal-notify.c 9 Mar 2004 05:42:47 -0000 1.4
> +++ src/evolution-webcal-notify.c 22 Mar 2004 20:27:40 -0000
> @@ -178,6 +178,7 @@ void e_webcal_query_user (const gchar *
> ESourceGroup * group;
> ESourceList * sources;
> GSList * l;
> + SoupUri * suri;
>
> sources = e_source_list_new_for_gconf_default ("/apps/evolution/calendar/sources");
>
> @@ -276,7 +277,9 @@ void e_webcal_query_user (const gchar *
> gtk_widget_show (vbox);
>
> /* Name */
> - mrkname = g_strdup_printf ("<b>%s</b>", name ? name : g_basename (caluri));
> + suri = soup_uri_new (caluri);
> + mrkname = g_strdup_printf ("<b>%s</b>", name ? name : g_basename (suri->path));
> + soup_uri_free (suri);
> label = gtk_label_new (mrkname);
> gtk_label_set_use_markup (GTK_LABEL (label), TRUE);
> g_free (mrkname);
Again, need to do the extra stuff and use g_path_get_basename instead.
Also, if the calendar does not have a name defined inside it with the
X-WR-CALNAME property, the source doesn't get created properly, and
subscribing to the calendar fails. This means we need to get the
basename of the URI's path earlier on, before we do e_source_new, and
then we should destroy the URI and temporary string at the bottom of the
code, when we don't need them anymore.
Thanks for the patch. I'll commit the modified version in my tree
shortly. I'll also try to get a bugzilla product set up on gnome.org for
evolution-webcal as soon as bugzilla is back up.
-- dobey
Attachment:
signature.asc
Description: This is a digitally signed message part