GNOME.org
 

Re: [evolution-patches] Another custom headers patch


it still leaks :)

anyway only a copule of real issues left, see below

On Fri, 2003-12-05 at 00:44, Grahame Bowland wrote: 
Hi guys,

Here's another crack at it. I think I've addressed everything you guys
have pointed out :-) In particular, it should definitely not be leaking
any more (I've stared at it a lot, anyway) and the sensitivity /
listview population code is much, much neater.

Cheers
Grahame


Apart from a crash and some (more) memory problems, getting down to
really niggling things now - don't have to change some of them if you
don't care to.


> Index: mail/ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
> retrieving revision 1.2896
> diff -u -r1.2896 ChangeLog
> --- mail/ChangeLog 23 Nov 2003 17:02:09 -0000 1.2896
> +++ mail/ChangeLog 4 Dec 2003 13:41:10 -0000
> @@ -1,3 +1,44 @@
> +2003-12-04  Grahame Bowland  <grahame angrygoats net>
> +
> + * mail-config.glade: add tab to mail configuration
> + dialog to allow custom headers to be specified for
> + display.
> +
> + * em-mailer-prefs.h: modify struct _EMMailerPrefs to
> + add widgets for custom header tab. Add defines for custom
> + header flags. Add struct EMMailerCustomHeader to describe
> + custom headers, and add function
> + em_mailer_custom_headers_from_xml to allow XML from gconf
> + key to be parsed into this structure.
> +
> + * em-folder-view.c (emfv_setting_notify): catch changes to
> + custom header gconf key and update mail view to correspond
> +
> + * em-mailer-prefs.c (em_mailer_prefs_apply): save custom
> + headers to gconf
> + (header_list_enabled_toggled): toggle clicked toggle column
> + (add_header): add header to custom header list if valid
> + (remove_header): remove selected custom header
> + (is_valid_header): return true if passed header is valid,
> + otherwise false
> + (entry_header_changed): call add_header_update_sensitivity
> + (em_mailer_prefs_construct): initialise header selection tab.
> + Load gconf data for header selection dialog.
> + (em_mailer_custom_header_to_xml): load a header structure
> + from XML document structure
> + (em_mailer_custom_header_from_xml): load a header
> + structure from a string containing valid XML. if any failure,
> + the header.name is set to NULL.
> + (header_list_row_selected): call
> + remove_header_update_sensitivity
> + (remove_header_update_sensitivity): set the sensitivity of
> + the remove button to FALSE if the list is empty or nothing
> + is selected. Otherwise, set it to TRUE.
> + (add_header_update_sensitivity): set the sensitivity of the
> + the add button to FALSE if the entry box is empty, contains
> + a duplicate header, or contains an invalid header. Otherwise,
> + set it to TRUE.
> +
>  2003-11-22  Jeffrey Stedfast  <fejj ximian com>
> 
>  * em-folder-tree-model.c (model_drag_data_received)
> Index: mail/em-folder-view.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/em-folder-view.c,v
> retrieving revision 1.14
> diff -u -r1.14 em-folder-view.c
> --- mail/em-folder-view.c 14 Nov 2003 16:20:17 -0000 1.14
> +++ mail/em-folder-view.c 4 Dec 2003 13:41:20 -0000
> @@ -62,6 +62,7 @@
>  #include "em-format-html-print.h"
>  #include "em-folder-selection.h"
>  #include "em-folder-view.h"
> +#include "em-mailer-prefs.h"
>  #include "em-message-browser.h"
>  #include "message-list.h"
>  #include "em-utils.h"
> @@ -1896,6 +1897,7 @@
>  EMFV_MARK_SEEN_TIMEOUT,
>  EMFV_LOAD_HTTP,
>  EMFV_XMAILER_MASK,
> + EMFV_HEADERS,
>  EMFV_SETTINGS /* last, for loop count */
>  };
> 
> @@ -1911,6 +1913,7 @@
>  "mark_seen_timeout",
>  "load_http_images",
>  "xmailer_mask",
> + "headers",
>  };
> 
>  static GHashTable *emfv_setting_key;
> @@ -1974,6 +1977,36 @@
>  case EMFV_XMAILER_MASK:
>  em_format_html_set_xmailer_mask((EMFormatHTML *)emfv->preview, gconf_value_get_int(gconf_entry_get_value(entry)));
>  break;
> + case EMFV_HEADERS: {
> + GSList *header_config_list, *p;
> + EMFormat *emf = (EMFormat *)emfv->preview;
> + header_config_list = gconf_client_get_list(gconf, "/apps/evolution/mail/display/headers", GCONF_VALUE_STRING, NULL);
> +      em_format_clear_headers((EMFormat *)emfv->preview);
> + /* if the list is zero length, they have not been through the configuration dialog,
> +    so should get the default headers. otherwise, the default headers will be copied
> +    into gconf, and we can just go from that
> + */
> + if (g_slist_length(header_config_list) == 0) {
> + em_format_default_headers((EMFormat *)emfv->preview);
> + } else {
> + p = header_config_list;
> + while (p) {
> + EMMailerPrefsHeader *h;
> + char *xml = (char *)p->data;
> +
> + h = em_mailer_prefs_header_from_xml(xml);
> + if (h && h->enabled)
> + em_format_add_header(emf, g_strdup(h->name), EM_FORMAT_HEADER_BOLD);

^^ don't g_strdup the header!  em_format_add_header takes a const char
*, which means it assumes it only exists for the duration of the call.
a leak.

> + em_mailer_prefs_header_free(h);

^^ first time i ran this it crashed, because the headers value was the
old one, invalid xml.

Make em_mailer_prefs_header_free accept null if you're going to pass
it null potentially (may as well, most other functions accept it).

Actually given this ... you might want to change the default_headers
logic, so instead of checking the header_config_list is empty (which
btw you can do with 'header_config_list == NULL') ... count how many
headers you actually output, and if that is zero, then set the default
headers.

its a more graceful fallback.

> + p = g_slist_next(p);
> + }
> + }
> + g_slist_foreach(header_config_list, (GFunc) g_free, NULL);
> + g_slist_free(header_config_list);
> + /* force a redraw */
> + if (emf->message)
> + em_format_format_clone(emf, emf->message, emf);
> + break; }
>  }
>  }
> 
> Index: mail/em-mailer-prefs.h
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/em-mailer-prefs.h,v
> retrieving revision 1.2
> diff -u -r1.2 em-mailer-prefs.h
> --- mail/em-mailer-prefs.h 29 Oct 2003 17:49:55 -0000 1.2
> +++ mail/em-mailer-prefs.h 4 Dec 2003 13:41:20 -0000
> @@ -37,6 +37,7 @@
>  #include <libgnomeui/gnome-font-picker.h>
> 
>  #include "evolution-config-control.h"
> +#include "em-format.h"
> 
>  #include <shell/Evolution.h>
> 
> @@ -48,6 +49,13 @@
> 
>  typedef struct _EMMailerPrefs EMMailerPrefs;
>  typedef struct _EMMailerPrefsClass EMMailerPrefsClass;
> +typedef struct _EMMailerPrefsHeader EMMailerPrefsHeader;
> +
> +struct _EMMailerPrefsHeader {
> + char *name;
> + int enabled:1;
> + int is_default:1;
> +};

no need for the is_ bit, but thats just niggling (ok, there is a need
in this case since default is a keyword ...).

>  struct _EMMailerPrefs {
>  GtkVBox parent_object;
> @@ -98,6 +106,13 @@
>  GnomeColorPicker *color;
>  } labels[5];
>  GtkButton *restore_labels;
> +
> + /* Headers tab */
> + GtkButton *add_header;
> + GtkButton *remove_header;
> + GtkEntry *entry_header;
> + GtkTreeView *header_list;
> + GtkListStore *header_list_store;
>  };
> 
>  struct _EMMailerPrefsClass {
> @@ -114,6 +129,12 @@
> 
>  void em_mailer_prefs_apply (EMMailerPrefs *prefs);
> 
> +EMMailerPrefsHeader *em_mailer_prefs_header_from_xml(const char *xml);
> +
> +char *em_mailer_prefs_header_to_xml(EMMailerPrefsHeader *header);
> +
> +void em_mailer_prefs_header_free(EMMailerPrefsHeader *header);
> +

^^ group these together without whitespace

>  /* needed by global config */
>  #define EM_MAILER_PREFS_CONTROL_ID "OAFIID:GNOME_Evolution_Mail_MailerPrefs_ConfigControl_2"
> 
> Index: mail/em-mailer-prefs.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/em-mailer-prefs.c,v
> retrieving revision 1.1
> diff -u -r1.1 em-mailer-prefs.c
> --- mail/em-mailer-prefs.c 24 Oct 2003 19:31:22 -0000 1.1
> +++ mail/em-mailer-prefs.c 4 Dec 2003 13:41:23 -0000
> @@ -31,6 +31,7 @@
> 
>  #include <gal/util/e-iconv.h>
>  #include <gtkhtml/gtkhtml-properties.h>
> +#include <libxml/tree.h>
>  #include "widgets/misc/e-charset-picker.h"
>  #include <bonobo/bonobo-generic-factory.h>
> 
> @@ -43,6 +44,30 @@
> 
>  static GtkVBoxClass *parent_class = NULL;
> 
> +enum {
> + HEADER_LIST_NAME_COLUMN, /* displayable name of the header (may be a translation) */
> + HEADER_LIST_ENABLED_COLUMN, /* is the header enabled? */
> + HEADER_LIST_IS_DEFAULT_COLUMN,  /* is this header a default header, eg From: */
> + HEADER_LIST_HEADER_COLUMN, /* the real name of this header */
> + HEADER_LIST_N_COLUMNS,
> +};
> +
> +/* temporarily copied from em-format.c */
> +static const struct {
> + const char *name;
> + guint32 flags;
> +} default_headers[] = {
> + { N_("From"), EM_FORMAT_HEADER_BOLD },
> + { N_("Reply-To"), EM_FORMAT_HEADER_BOLD },
> + { N_("To"), EM_FORMAT_HEADER_BOLD },
> + { N_("Cc"), EM_FORMAT_HEADER_BOLD },
> + { N_("Bcc"), EM_FORMAT_HEADER_BOLD },
> + { N_("Subject"), EM_FORMAT_HEADER_BOLD },
> + { N_("Date"), EM_FORMAT_HEADER_BOLD },
> + { "x-evolution-mailer", 0 }, /* DO NOT translate */
> +};
> +
> +#define EM_FORMAT_HEADER_XMAILER "x-evolution-mailer"
> 
>  GtkType
>  em_mailer_prefs_get_type (void)
> @@ -202,6 +227,155 @@
>  }
>  }
> 
> +
> +static void
> +remove_header_update_sensitivity (EMMailerPrefs *prefs)

kinda long name for a simple function.

> +{
> + GtkTreeIter iter;
> + GtkTreeSelection *selection = gtk_tree_view_get_selection (prefs->header_list);
> + gboolean is_default;
> +
> + /* remove button should be sensitive if the currenlty selected entry in the list view
> +           is not a default header. if there are no entries, or none is selected, it should be
> +           disabled
> + */
> + if (gtk_tree_selection_get_selected (selection, NULL, &iter)) {
> + gtk_tree_model_get (GTK_TREE_MODEL (prefs->header_list_store), &iter,
> +     HEADER_LIST_IS_DEFAULT_COLUMN, &is_default,
> +     -1);
> + if (is_default)
> + gtk_widget_set_sensitive (GTK_WIDGET (prefs->remove_header), FALSE);
> + else
> + gtk_widget_set_sensitive (GTK_WIDGET (prefs->remove_header), TRUE);
> + } else {
> + gtk_widget_set_sensitive (GTK_WIDGET (prefs->remove_header), FALSE);
> + }
> +}
> +
> +static gboolean
> +is_valid_header(const char *header)
> +{
> + const char *p = header;
> +
> + if (strlen (header) == 0)
> + return FALSE;
> + if (g_ascii_strcasecmp (header, EM_FORMAT_HEADER_XMAILER) == 0)
> + return FALSE;

this wont be necessary if its added to the list.

> + while (*p) {
> + if ((*p == ':') || (*p == ' '))
> + return FALSE;
> + p++;
> + }
> + return TRUE;
> +}
> +
> +static void
> +add_header_update_sensitivity (EMMailerPrefs *prefs)
> +{
> + const char *entry_contents;
> + GtkTreeIter iter;
> + gboolean valid;
> +
> + /* the add header button should be sensitive if the text box contains
> +    a valid header string, that is not a duplicate with something already
> +    in the list view
> + */
> + entry_contents = gtk_entry_get_text (GTK_ENTRY (prefs->entry_header));
> + if (!is_valid_header (entry_contents)) {
> + gtk_widget_set_sensitive (GTK_WIDGET (prefs->add_header), FALSE);
> + return;
> + }
> + /* check if this is a duplicate */
> + valid = gtk_tree_model_get_iter_first (GTK_TREE_MODEL (prefs->header_list_store), &iter);
> + while (valid) {
> + char *header_name;
> +
> + gtk_tree_model_get (GTK_TREE_MODEL (prefs->header_list_store), &iter,
> +     HEADER_LIST_HEADER_COLUMN, &header_name,
> +     -1);
> + if (g_ascii_strcasecmp (header_name, entry_contents) == 0) {
> + gtk_widget_set_sensitive (GTK_WIDGET(prefs->add_header), FALSE);
> + return;
> + }
> + valid = gtk_tree_model_iter_next (GTK_TREE_MODEL(prefs->header_list_store), &iter);
> + }
> + gtk_widget_set_sensitive (GTK_WIDGET (prefs->add_header), TRUE);
> +}
> +
> +static void
> +header_list_enabled_toggled (GtkCellRendererToggle *cell, const char *path_string, gpointer user_data)
> +{
> + EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> + GtkTreeModel *model = GTK_TREE_MODEL (prefs->header_list_store);
> + GtkTreePath *path = gtk_tree_path_new_from_string (path_string);
> + GtkTreeIter iter;
> + int enabled;
> +
> + gtk_tree_model_get_iter (model, &iter, path);
> + gtk_tree_model_get (model, &iter, HEADER_LIST_ENABLED_COLUMN, &enabled, -1);
> + enabled = !enabled;
> + gtk_list_store_set (GTK_LIST_STORE (model), &iter, HEADER_LIST_ENABLED_COLUMN,
> +    enabled, -1);
> + gtk_tree_path_free (path);
> +
> + if (prefs->control)
> + evolution_config_control_changed (prefs->control);
> +}
> +
> +static void
> +add_header (GtkWidget *widget, gpointer user_data)

it would be nice to namespace some of this stuff into emmp_header  or something.

but not necessary either.

> +{
> + EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> + GtkTreeModel *model = GTK_TREE_MODEL (prefs->header_list_store);
> + GtkTreeIter iter;
> +
> + gtk_list_store_append(GTK_LIST_STORE (model), &iter);
> + gtk_list_store_set(GTK_LIST_STORE (model), &iter,
> +    HEADER_LIST_NAME_COLUMN, gtk_entry_get_text (prefs->entry_header),
> +    HEADER_LIST_ENABLED_COLUMN, TRUE,
> +    HEADER_LIST_HEADER_COLUMN, gtk_entry_get_text (prefs->entry_header),
> +    HEADER_LIST_IS_DEFAULT_COLUMN, FALSE,
> +    -1);
> + gtk_entry_set_text (prefs->entry_header, "");
> + remove_header_update_sensitivity (prefs);
> + add_header_update_sensitivity (prefs);
> +
> + if (prefs->control)
> + evolution_config_control_changed (prefs->control);
> +}
> +
> +static void
> +remove_header (GtkWidget *button, gpointer user_data)
> +{
> + EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> + GtkTreeModel *model = GTK_TREE_MODEL (prefs->header_list_store);
> + GtkTreeSelection *selection = gtk_tree_view_get_selection (prefs->header_list);
> + GtkTreeIter iter;
> +
> + if (gtk_tree_selection_get_selected (selection, NULL, &iter)) {
> + gtk_list_store_remove (GTK_LIST_STORE (model), &iter);
> + remove_header_update_sensitivity (prefs);
> + if (prefs->control)
> + evolution_config_control_changed (prefs->control);
> + }
> +}
> +
> +static void
> +header_list_row_selected (GtkTreeSelection *selection, gpointer user_data)
> +{
> + EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> +
> + remove_header_update_sensitivity (prefs);
> +}
> +
> +static void
> +entry_header_changed (GtkWidget *entry, gpointer user_data)
> +{
> + EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> +
> + add_header_update_sensitivity (prefs);
> +}
> +
>  static void
>  em_mailer_prefs_construct (EMMailerPrefs *prefs)
>  {
> @@ -209,10 +383,15 @@
>  GSList *list;
>  GladeXML *gui;
>  gboolean bool;
> - char *font;
> + char *font, *buf;
>  int i, val;
> - char *buf;
> -
> + GSList *header_config_list, *header_add_list, *p;
> + GtkTreeIter iter;
> + GHashTable *default_header_hash;
> + GtkTreeSelection *selection;
> + GtkCellRenderer *header_list_name_renderer;
> + GtkCellRenderer *header_list_enabled_renderer;
> +
>  gui = glade_xml_new (EVOLUTION_GLADEDIR "/mail-config.glade", "preferences_tab", NULL);
>  prefs->gui = gui;
> 
> @@ -360,6 +539,96 @@
> 
>  prefs->restore_labels = GTK_BUTTON (glade_xml_get_widget (gui, "cmdRestoreLabels"));
>  g_signal_connect (prefs->restore_labels, "clicked", G_CALLBACK (restore_labels_clicked), prefs);
> +
> + /* headers */
> + prefs->add_header = GTK_BUTTON (glade_xml_get_widget (gui, "cmdHeadersAdd"));
> + prefs->remove_header = GTK_BUTTON (glade_xml_get_widget (gui, "cmdHeadersRemove"));
> + prefs->entry_header = GTK_ENTRY (glade_xml_get_widget (gui, "txtHeaders"));
> + prefs->header_list = GTK_TREE_VIEW (glade_xml_get_widget (gui, "treeHeaders"));
> + selection = gtk_tree_view_get_selection (prefs->header_list);
> + g_signal_connect (selection, "changed", G_CALLBACK (header_list_row_selected), prefs);
> + g_signal_connect (prefs->entry_header, "changed", G_CALLBACK (entry_header_changed), prefs);
> + g_signal_connect (prefs->entry_header, "activate", G_CALLBACK (add_header), prefs);
> + /* initialise the tree with appropriate headings */
> + prefs->header_list_store = gtk_list_store_new (HEADER_LIST_N_COLUMNS, G_TYPE_STRING, G_TYPE_BOOLEAN, G_TYPE_BOOLEAN, G_TYPE_STRING);
> + g_signal_connect (prefs->add_header, "clicked", G_CALLBACK (add_header), prefs);
> + g_signal_connect (prefs->remove_header, "clicked", G_CALLBACK (remove_header), prefs);
> + gtk_tree_view_set_model (prefs->header_list, GTK_TREE_MODEL (prefs->header_list_store));
> +
> + header_list_enabled_renderer = gtk_cell_renderer_toggle_new ();
> + g_object_set (header_list_enabled_renderer, "activatable", TRUE, NULL);
> + g_signal_connect (header_list_enabled_renderer, "toggled", G_CALLBACK (header_list_enabled_toggled), prefs);
> +        gtk_tree_view_insert_column_with_attributes (GTK_TREE_VIEW (prefs->header_list), -1,
> +      "Enabled", header_list_enabled_renderer,
> +      "active", HEADER_LIST_ENABLED_COLUMN,
> +      NULL);
> + header_list_name_renderer = gtk_cell_renderer_text_new ();
> + gtk_tree_view_insert_column_with_attributes (GTK_TREE_VIEW (prefs->header_list), -1,
> +      "Name", header_list_name_renderer,
> +      "text", HEADER_LIST_NAME_COLUMN,
> +      NULL);
> +
> + /* populated the listview with entries; firstly we add all the default headers, and then
> +    we add read header configuration out of gconf. If a header in gconf is a default header,
> +    we update the enabled flag accordingly
> + */
> + header_add_list = NULL;
> + default_header_hash = g_hash_table_new (g_str_hash, g_str_equal);
> + for (i = 0; i < sizeof (default_headers) / sizeof (default_headers[0]); i++) {
> + struct _EMMailerPrefsHeader *h;
> +
> + /* ensure we do not add the special x-evolution header */
> + if (g_ascii_strcasecmp (default_headers[i].name, EM_FORMAT_HEADER_XMAILER) == 0)
> + continue;

I think we want to add this header.

I patched the code a bit to do it, and i saw no ill effects.  I had to
modify em-format-html.c a little though, so it ignored the
xmailer_mask thing (which is an undocumented way to add it normally).
And I had to special case adding it to the tree store.

> + h = g_malloc (sizeof (struct _EMMailerPrefsHeader));
> + h->is_default = TRUE;
> + h->name = g_strdup(default_headers[i].name);
> + h->enabled = TRUE;
> + g_hash_table_insert (default_header_hash, (gpointer)default_headers[i].name, (gpointer)h);
> + header_add_list = g_slist_append (header_add_list, h);
> + }
> +
> + /* read stored headers from gconf */
> + header_config_list = gconf_client_get_list (prefs->gconf, "/apps/evolution/mail/display/headers", GCONF_VALUE_STRING, NULL);
> + p = header_config_list;
> + while (p) {
> + char *xml = (char *)p->data;
> + struct _EMMailerPrefsHeader *h, *def;
> +
> + h = em_mailer_prefs_header_from_xml (xml);
> + if (h && (g_ascii_strcasecmp (h->name, EM_FORMAT_HEADER_XMAILER) != 0)) {
> + def = (struct _EMMailerPrefsHeader *)g_hash_table_lookup (default_header_hash, (gpointer)h->name);

fwiw you don't need to cast to a gpointer here.  by definition a void
* (which is all a gpointer is) can take any pointer.

also note that you leak h if xmailer ever got in there (but if you
remove that check it isn't an issue).

> + if (def) {
> + def->enabled = h->enabled;
> + em_mailer_prefs_header_free(h);
> + } else {
> + h->is_default = FALSE;
> + header_add_list = g_slist_append (header_add_list, h);
> + }
> + }
> + p = g_slist_next (p);
> + }
> + g_hash_table_destroy (default_header_hash);
> + g_slist_foreach (header_config_list, (GFunc) g_free, NULL);
> + g_slist_free (header_config_list);
> +
> + p = header_add_list;
> + while (p) {
> + struct _EMMailerPrefsHeader *h = (struct _EMMailerPrefsHeader *)p->data;

if you add in the xmailer stuff, need to set the NAME_COLUMN to 'Mailer' here, e.g.

const char *name;

if (g_ascii_strcasecmp(h->name, EM_FORMAT_HEADER_XMAILER) == 0)
name = _("Mailer");
else
name = _(h->name);

etc.

> + gtk_list_store_append (prefs->header_list_store, &iter);
> + gtk_list_store_set (prefs->header_list_store, &iter,
> +     HEADER_LIST_NAME_COLUMN, _ (h->name),
> +     HEADER_LIST_ENABLED_COLUMN, h->enabled,
> +     HEADER_LIST_IS_DEFAULT_COLUMN, h->is_default,
> +     HEADER_LIST_HEADER_COLUMN, h->name,
> +     -1);
> + em_mailer_prefs_header_free(h);
> + p = g_slist_next(p);
> + }
> + g_slist_free (header_add_list);
> +
> + remove_header_update_sensitivity (prefs);
>  }
> 
> 
> @@ -384,6 +653,9 @@
>  GSList *list, *l, *n;
>  guint32 rgb;
>  int i, val;
> + GtkTreeIter iter;
> + gboolean valid;
> + GSList *header_list;
> 
>  /* General tab */
> 
> @@ -469,6 +741,109 @@
>  g_slist_free_1 (l);
>  l = n;
>  }
> -
> +
> + /* Headers */
> + header_list = NULL;
> + valid = gtk_tree_model_get_iter_first (GTK_TREE_MODEL (prefs->header_list_store), &iter);
> + while (valid) {
> + struct _EMMailerPrefsHeader h;
> + gboolean enabled;
> + char *xml;
> +
> + gtk_tree_model_get (GTK_TREE_MODEL (prefs->header_list_store), &iter,
> +     HEADER_LIST_HEADER_COLUMN, &h.name,
> +     HEADER_LIST_ENABLED_COLUMN, &enabled,
> +     -1);
> + h.enabled = enabled;
> + xml = em_mailer_prefs_header_to_xml (&h);
> + if (xml != NULL)
> + header_list = g_slist_append (header_list, xml);
> + valid = gtk_tree_model_iter_next (GTK_TREE_MODEL (prefs->header_list_store), &iter);
> + }
> + gconf_client_set_list (prefs->gconf, "/apps/evolution/mail/display/headers", GCONF_VALUE_STRING, header_list, NULL);
>  gconf_client_suggest_sync (prefs->gconf, NULL);
> + g_slist_foreach (header_list, (GFunc) g_free, NULL);
> + g_slist_free (header_list);
> +}
> +
> +static struct _EMMailerPrefsHeader *
> +emmp_header_from_xmldoc(xmlDocPtr doc)
> +{
> + xmlNodePtr root;
> + xmlChar *name;
> + struct _EMMailerPrefsHeader *h;
> +
> + if (doc == NULL)
> + return NULL;
> + root = doc->children;
> + if (strcmp (root->name, "header") != 0)
> + return NULL;
> + name = xmlGetProp (root, "name");
> + if (name == NULL)
> + return NULL;
> + h = g_malloc (sizeof (struct _EMMailerPrefsHeader));

use g_malloc0() here, or ensure you initialise all fields (i.e. is_default).

using g_malloc0 is 'easier', but can hide uninitialisations.  forgetting to
intialise something could lead to harder to track down bugs though.

> + h->name = g_strdup(name);
> + xmlFree (name);
> + if (xmlHasProp (root, "enabled"))
> + h->enabled = 1;
> + else
> + h->enabled = 0;
> + return h;
> +}
> +
> +/* we read from gconf a XML structure that looks like:
> +   <header name="foo" enabled />
> +*/
> +struct _EMMailerPrefsHeader *
> +em_mailer_prefs_header_from_xml(const char *xml)
> +{
> + xmlDocPtr doc;
> + struct _EMMailerPrefsHeader *header;
> +
> + doc = xmlParseDoc ((char *)xml);
> + if (doc == NULL)
> + return NULL;
> + header = emmp_header_from_xmldoc (doc);

almost hardly worth making emmp_header_from_xmldoc a separate function, since its only
used by this one.

> + xmlFreeDoc (doc);
> + return header;
> +}
> +
> +void
> +em_mailer_prefs_header_free(struct _EMMailerPrefsHeader *h)
> +{

add:

if (h==NULL)
return;

i.e. it is valid to pass it null.

> + if (h->name)
> + g_free (h->name);

don't check h->name, g_free already does this check.

> + g_free (h);
> +}
> +
> +
> +char *
> +em_mailer_prefs_header_to_xml(struct _EMMailerPrefsHeader *header)

This (and the other exported functions) should have some gtk-doc
comments on them.  This is where you can explain the format too (i
know its there, but this is a good place too).

> +{
> + xmlDocPtr doc;
> + xmlNodePtr root;
> + xmlChar *xml_buffer;
> + char *returned_buffer;
> + int xml_buffer_size;

pretty ugly names.  how about just 'xml' 'out' and 'size'?
(you use 'xml' elsewhere, keep it consistent at least)

> + g_return_val_if_fail (header != NULL, NULL);
> + g_return_val_if_fail (header->name != NULL, NULL);
> +
> + doc = xmlNewDoc ("1.0");
> +
> + root = xmlNewDocNode (doc, NULL, "header", NULL);
> + xmlSetProp (root, "name", header->name);
> + if (header->enabled)
> + xmlSetProp (root, "enabled", NULL);
> +
> + xmlDocSetRootElement (doc, root);
> + xmlDocDumpMemory (doc, &xml_buffer, &xml_buffer_size);
> + xmlFreeDoc (doc);
> +
> + returned_buffer = g_malloc (xml_buffer_size + 1);
> + memcpy (returned_buffer, xml_buffer, xml_buffer_size);
> + returned_buffer[xml_buffer_size] = '\0';

you can just use 0 here.  People seem to use '\0' because they think
its some magical trasnportable ascii null.  Its not, its just octal
for 0, and less readable.  (this is just a pet niggle, really
irrelevent to the patch).

> + xmlFree (xml_buffer);
> +
> + return returned_buffer;
>  }


> Index: mail/evolution-mail.schemas
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/Attic/evolution-mail.schemas,v
> retrieving revision 1.20
> diff -u -r1.20 evolution-mail.schemas
> --- mail/evolution-mail.schemas 30 Sep 2003 06:52:13 -0000 1.20
> +++ mail/evolution-mail.schemas 4 Dec 2003 13:42:40 -0000

This got renamed to evolution-mail.schemas.in.in so make sure you cvs
update and put in the new entry there.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]