Re: [evolution-patches] Another custom headers patch

[Not Zed <notzed ximian com>]

On Thu, 2003-12-04 at 04:31, Grahame Bowland wrote:
> Hi all
>
> Changes from the last patch:
>   * default headers are now included in the list of headers, and 
>     can be disabled and enabled, but not removed from the list
>     (eg. From: and To: headers)
>   * default headers are displayed in their translated form
>   * it should no longer be leaking h->name
>   * we don't have flags any more, we merely have enabled or 
>     disabled
>
> I think I've addressed everything in NotZed's last email.
Looking good.  Just a few technical and style niggles left.  The functionality looks good.

Comments below.

 Michael

> Anyway, patch attached. Feedback, everyone? :)
>
> Cheers
> Grahame
>
> PS: copyright assignment letter is in the mail to Ximian. 


> 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	3 Dec 2003 17:16:19 -0000
> @@ -1,3 +1,36 @@
> +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): set add button sensitive if entered
> +	header is valid, otherwise set add button unsensitive
> +	(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): set the sensitivity of the 
> +	remove button to FALSE if the selected header is a default, or 
> +	to TRUE otherwise.
> +	
>  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	3 Dec 2003 17:16:24 -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,30 @@
>  	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 empty (i.e. it hasn't been edited yet), this should just set the default headers, all enabled.

> +		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);
> +			}
> +			em_mailer_prefs_header_free(h);
> +			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	3 Dec 2003 17:16:24 -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,9 +49,15 @@
>  
>  typedef struct _EMMailerPrefs EMMailerPrefs;
>  typedef struct _EMMailerPrefsClass EMMailerPrefsClass;
> +typedef struct _EMMailerPrefsHeader EMMailerPrefsHeader;
> +
> +struct _EMMailerPrefsHeader {
> +	char *name;
> +	int enabled:1;
> +};
>  
>  struct _EMMailerPrefs {
> -	GtkVBox parent_object;
> +        GtkVBox parent_object;
>  	
>  	GNOME_Evolution_Shell shell;
>  	
> @@ -98,6 +105,15 @@
>  		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;
> +	GtkCellRenderer *header_list_name_renderer;
> +	GtkCellRenderer *header_list_enabled_renderer;
>  };
>  
>  struct _EMMailerPrefsClass {
> @@ -114,6 +130,10 @@
>  
>  void em_mailer_prefs_apply (EMMailerPrefs *prefs);
>  
> +EMMailerPrefsHeader *em_mailer_prefs_header_from_xml(const char *xml);
> +
> +void em_mailer_prefs_header_free(EMMailerPrefsHeader *header);
> +
>  /* 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	3 Dec 2003 17:16:26 -0000
> @@ -31,18 +31,44 @@
>  
>  #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>
>  
>  #include "mail-config.h"
>  
>  
> -static void em_mailer_prefs_class_init (EMMailerPrefsClass *class);
> -static void em_mailer_prefs_init       (EMMailerPrefs *dialog);
> -static void em_mailer_prefs_finalise   (GObject *obj);
> +static void em_mailer_prefs_class_init     (EMMailerPrefsClass *class);
> +static void em_mailer_prefs_init           (EMMailerPrefs *dialog);
> +static void em_mailer_prefs_finalise       (GObject *obj);
> +static char *em_mailer_prefs_header_to_xml (struct _EMMailerPrefsHeader *h);
>  
>  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)
> @@ -203,16 +229,161 @@
>  }
>  
>  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)
> +{
> +	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);
> +	if (!GTK_WIDGET_SENSITIVE(GTK_WIDGET(prefs->remove_header))) {
> +		GtkTreeSelection *selection = gtk_tree_view_get_selection (prefs->header_list);
> +		gtk_tree_model_get_iter_first(GTK_TREE_MODEL(model), &iter);
> +		gtk_widget_set_sensitive(GTK_WIDGET(prefs->remove_header), TRUE);
> +		gtk_tree_selection_select_iter(selection, &iter);
> +	}
> +	/* now clear out the text entry, and make the add button insensitive */
> +	gtk_entry_set_text(prefs->entry_header, "");
> +	gtk_widget_set_sensitive(GTK_WIDGET(prefs->add_header), FALSE);

The 'sensitive' stuff is all over the place.  I've usually found it a lot easier to put it in a single
function, and then call that function whenever it might need updating.  Its just an idea.

It only needs to check the validity of the entry and whether something is selected, so should be pretty cheap.

> +	if (prefs->control)
> +		evolution_config_control_changed (prefs->control);
> +}
> +
> +static void
> +remove_header (GtkWidget *button, gpointer user_data)
> +{
> +	EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> +	GtkTreeView *treeview = prefs->header_list;
> +	GtkTreeModel *model = GTK_TREE_MODEL(prefs->header_list_store);
> +	GtkTreeSelection *selection = gtk_tree_view_get_selection (treeview);
> +	GtkTreeIter iter;
> +	
> +	if (gtk_tree_selection_get_selected (selection, NULL, &iter)) {
> +		gtk_list_store_remove(GTK_LIST_STORE(model), &iter);
> +		if (!gtk_tree_model_get_iter_first(GTK_TREE_MODEL(model), &iter)) {
> +			/* list is empty - disable the remove button*/
> +			gtk_widget_set_sensitive(GTK_WIDGET(prefs->remove_header), FALSE);
> +		} else {
> +			/* select the first entry */
> +			gtk_tree_selection_select_iter(selection, &iter);
> +		}
> +		if (prefs->control)
> +			evolution_config_control_changed (prefs->control);
> +	}
> +}
> +
> +static gboolean
> +is_valid_header(const char *header)
> +{
> +	const char *p = header;
> +
> +	if (strlen(header) == 0) {
> +		return FALSE;
> +	}
> +	if (strcasecmp(header, EM_FORMAT_HEADER_XMAILER) == 0) {
> +		return FALSE;
> +	}
> 
use g_ascii_strcasecmp if you ever want to do a case-insensitive compare of this stuff.

is this test necessary still?

> +	while (*p) {
> +		if ((*p == ':') || (*p == ' ')) {
> +			return FALSE;
> +		}
> +		p++;
> +	}

there are actually more characters that are invalid.  rfc822 defines the valid ones.  although this is probably fine,
the user will just enter something that can never show up.

> +	return TRUE;
> +}
> +
> +static void 
> +header_list_row_selected (GtkTreeSelection *selection, gpointer user_data)
> +{
> +	EMMailerPrefs *prefs = (EMMailerPrefs *) user_data;
> +	gboolean is_default;
> +	GtkTreeIter iter;
> +
> +	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);
> +		}
> +	}
> +}
> +
> +static void
> +entry_header_changed (GtkWidget *entry, gpointer user_data)
> +{
> +	EMMailerPrefs *prefs = (EMMailerPrefs *)user_data;
> +	const char *entry_contents;
> +	GtkTreeIter iter;
> +	gboolean valid;
> +
> +	entry_contents = gtk_entry_get_text(GTK_ENTRY(entry));
> +	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_NAME_COLUMN, &header_name, 
> +				    -1);
> +		if (strcasecmp(header_name, entry_contents) == 0) {

g_ascii_strcasecmp() here.  the reason is strcasecmp is locale dependent.

> +			/* duplicate, not valid */

^^ no need for htis comment, 'check if this is a duplicate' covers this whole function adequately.

> +			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);
> +	}
> +	/* if we got here, we are valid and unique */
> +	gtk_widget_set_sensitive (GTK_WIDGET(prefs->add_header), TRUE);
> +}
> +
> +static void
>  em_mailer_prefs_construct (EMMailerPrefs *prefs)
>  {
>  	GtkWidget *toplevel, *menu;
>  	GSList *list;
>  	GladeXML *gui;
> -	gboolean bool;
> +	gboolean bool, first_row;
>  	char *font;
>  	int i, val;
>  	char *buf;
> -	
> +	GSList *header_config_list, *p;
> +	GtkTreeIter iter;
> +	GHashTable *header_loaded_hash, *default_header_hash;
> +	GtkTreeSelection *selection;
> +
>  	gui = glade_xml_new (EVOLUTION_GLADEDIR "/mail-config.glade", "preferences_tab", NULL);
>  	prefs->gui = gui;
>  	
> @@ -331,7 +502,7 @@
>  	bool = gconf_client_get_bool (prefs->gconf, "/apps/evolution/mail/prompts/unwanted_html", NULL);
>  	gtk_toggle_button_set_active (prefs->prompt_unwanted_html, bool);
>  	g_signal_connect (prefs->prompt_unwanted_html, "toggled", G_CALLBACK (settings_changed), prefs);
> -	
> +
>  	i = 0;
>  	list = mail_config_get_labels ();
>  	while (list != NULL && i < 5) {
> @@ -360,6 +531,103 @@
>  	
>  	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);

I find putting this stuff on one line is more readable, but its not important.

> +	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));
> +	prefs->header_list_enabled_renderer = gtk_cell_renderer_toggle_new();
> +	g_object_set(prefs->header_list_enabled_renderer, "activatable", TRUE, NULL);
> +	g_signal_connect(prefs->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", prefs->header_list_enabled_renderer,
> +						    "active", HEADER_LIST_ENABLED_COLUMN, 
> +						    NULL);
> +	prefs->header_list_name_renderer = gtk_cell_renderer_text_new();
> +	gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(prefs->header_list), -1, 
> +						    "Name", prefs->header_list_name_renderer,
> +						    "text", HEADER_LIST_NAME_COLUMN, 
> +						    NULL);
> +
> +	/* read stored values from gconf */
> +	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++) {
> +		g_hash_table_insert(default_header_hash, (gpointer)default_headers[i].name, GINT_TO_POINTER(1));
> +	}

generally dont put {}'s around 1 line if's and for's makes it more readable.

a sprinkling of blank lines after if's and for's also helps.

> +	header_loaded_hash = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
> +	header_config_list = gconf_client_get_list (prefs->gconf, "/apps/evolution/mail/display/headers", GCONF_VALUE_STRING, NULL);
> +	p = header_config_list;
> +	first_row = TRUE;
> +	while (p) {
> +		char *xml = (char *)p->data;
> +		gboolean is_default = FALSE;
> +		struct _EMMailerPrefsHeader *h;
> +
> +		h = em_mailer_prefs_header_from_xml (xml);
> +		if (g_hash_table_lookup(default_header_hash, (gpointer)h->name)) {
> +			is_default = TRUE;
> +		}
> +		if (h) {
> +			/* ensure we do not add the special x-evolution header */
> +			if (strcasecmp(h->name, EM_FORMAT_HEADER_XMAILER) != 0) {
> +				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, is_default, 
> +						    HEADER_LIST_HEADER_COLUMN, h->name, 
> +						    -1);
> +				g_hash_table_insert(header_loaded_hash, (gpointer)g_strdup(h->name), GINT_TO_POINTER(1));
> +				em_mailer_prefs_header_free(h);
> +				if (first_row && is_default) {
> +					gtk_widget_set_sensitive (GTK_WIDGET (prefs->remove_header), FALSE);
> +					first_row = FALSE;
> +				}
> +			}
> +		}
> +
> +		p = g_slist_next(p);
> +	}
> +	g_slist_foreach (header_config_list, (GFunc) g_free, NULL);
> +	g_slist_free(header_config_list);
> +	/* now, add default headers if they have not already been added */
> +	for (i=sizeof(default_headers)/sizeof(default_headers[0])-1;i>=0;i--) {
> +		/* ensure we do not add the special x-evolution header */
> +		if (strcasecmp(default_headers[i].name, EM_FORMAT_HEADER_XMAILER) == 0) {
> +			continue;
> +		}

I think we still want to add x-evolution-mailer, but change its display name to 'Mailer' (there is no such real header).

> +		if (!g_hash_table_lookup(header_loaded_hash, (gpointer)default_headers[i].name)) {
> +			gtk_list_store_prepend (prefs->header_list_store, &iter);
> +			gtk_list_store_set (prefs->header_list_store, &iter, 
> +					    HEADER_LIST_NAME_COLUMN, _((gpointer)default_headers[i].name), 
> +					    HEADER_LIST_ENABLED_COLUMN, TRUE, 
> +					    HEADER_LIST_IS_DEFAULT_COLUMN, TRUE, 
> +					    HEADER_LIST_HEADER_COLUMN, (gpointer)default_headers[i].name, 
> +					    -1);
> +		}
> +	}
> +	g_hash_table_destroy(header_loaded_hash);
> +	g_hash_table_destroy(default_header_hash)

^^ I have a suggestion to make the above code a little simpler.


first add the default headers to a list of MailPrefsHeader structs, with a hashtable pointing to each entry, all enabled
get the config values
for each value
  if its already in the hash
    update the enabled bit from the new value
  else
    append it to the list

for each item in the list
  append it to the list store

clean up

then you dont have to mess about with prepending and checking special cases in mutliple places.


> +	if (!gtk_tree_model_get_iter_first(GTK_TREE_MODEL(prefs->header_list_store), &iter)) {
> +		/* list is empty - disable the remove button */
> +		gtk_widget_set_sensitive(GTK_WIDGET(prefs->remove_header), FALSE);
> +	}
>  }
>  
>  
> @@ -384,6 +652,9 @@
>  	GSList *list, *l, *n;
>  	guint32 rgb;
>  	int i, val;
> +	GtkTreeIter iter;
> +	gboolean valid;
> +	GSList *header_list;
>  	
>  	/* General tab */
>  	
> @@ -444,7 +715,7 @@
>  	gconf_client_set_string (prefs->gconf, "/apps/evolution/mail/display/fonts/monospace",
>  				 gnome_font_picker_get_font_name (prefs->font_fixed), NULL);
>  	gconf_client_set_bool (prefs->gconf, "/apps/evolution/mail/display/fonts/use_custom",
> -			       !gtk_toggle_button_get_active (prefs->font_share), NULL);
> + 			       !gtk_toggle_button_get_active (prefs->font_share), NULL);
>  	gconf_client_set_bool (prefs->gconf, "/apps/evolution/mail/display/animate_images",
>  			       gtk_toggle_button_get_active (prefs->show_animated), NULL);
>  	
> @@ -469,6 +740,121 @@
>  		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);
> +		if (enabled) {
> +			h.enabled = 1;
> +		} else {
> +			h.enabled = 0;
> +		}
> +		xml = em_mailer_prefs_header_to_xml(&h);
> +		if (xml != NULL) {

can this possibly fail?  i dont think so.

> +			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);
> +	g_slist_free (header_list);

you have to free each header value too.

>  	gconf_client_suggest_sync (prefs->gconf, NULL);
>  }
> +
> +static struct _EMMailerPrefsHeader *
> +em_mailer_prefs_header_from_xmldoc(xmlDocPtr doc)

internal methods, use a shorter prefix, like emmp_header_from ...

> +{
> +	xmlNodePtr root;
> +	xmlChar *name;
> +	struct _EMMailerPrefsHeader *h;
> +
> +	h = g_malloc(sizeof(struct _EMMailerPrefsHeader));
> +	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;
> +	}

^^ have all of these checks BEFORE you allocate h and leak it!

i think xmlGetProp might allocate the string too, you might need to xmlFree it.

> +	h->name = g_strdup(name);
> +	if (xmlHasProp (root, "enabled")) {
> +		h->enabled = 1;
> +	} else {
> +		h->enabled = 0;
> +	}

as above, dont need all these {}'s.

my rule is:
 don't use {} for single line if statements unless there are nested if's, or if there is already an if or else case that is multiline.
 dont use {} for single line while's

> +	xmlFree(name);
> +	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 = em_mailer_prefs_header_from_xmldoc(doc);
> +	xmlFreeDoc(doc);
> +	return header;
> +}
> +
> +void
> +em_mailer_prefs_header_free(struct _EMMailerPrefsHeader *h)
> +{
> +	g_return_if_fail(h != NULL);
> +	if (h->name) {
> +		g_free (h->name);
> +	}

this NULL test isn't required, g_free does this already.

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

if you're going to export the other related methods, may as well export this one too.

> +{
> +	xmlDocPtr doc;
> +	xmlNodePtr root;
> +	xmlChar *xml_buffer;
> +	char *returned_buffer;
> +	int xml_buffer_size;
> +
> +	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';
> +	xmlFree (xml_buffer);
> +
> +	return returned_buffer;
> +}
> Index: mail/mail-config.glade
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-config.glade,v
> retrieving revision 1.124
> diff -u -r1.124 mail-config.glade
> --- mail/mail-config.glade	11 Nov 2003 02:38:01 -0000	1.124
> +++ mail/mail-config.glade	3 Dec 2003 17:16:43 -0000
> @@ -4475,6 +4475,177 @@
>  	  <property name="type">tab</property>
>  	</packing>
>        </child>
> +
> +      <child>
> +	<widget class="GtkVBox" id="vboxHeaderTab">
> +	  <property name="border_width">6</property>
> +	  <property name="visible">True</property>
> +	  <property name="homogeneous">False</property>
> +	  <property name="spacing">0</property>
> +
> +	  <child>
> +	    <widget class="GtkLabel" id="lblHeaders">
> +	      <property name="visible">True</property>
> +	      <property name="label" translatable="yes">&lt;b&gt;_Displayed Mail Headers&lt;/b&gt;</property>
> +	      <property name="use_underline">True</property>
> +	      <property name="use_markup">True</property>
> +	      <property name="justify">GTK_JUSTIFY_LEFT</property>
> +	      <property name="wrap">False</property>
> +	      <property name="selectable">False</property>
> +	      <property name="xalign">0</property>
> +	      <property name="yalign">0.5</property>
> +	      <property name="xpad">0</property>
> +	      <property name="ypad">0</property>
> +	      <property name="mnemonic_widget">txtHeaders</property>
> +	    </widget>
> +	    <packing>
> +	      <property name="padding">0</property>
> +	      <property name="expand">False</property>
> +	      <property name="fill">False</property>
> +	    </packing>
> +	  </child>
> +
> +	  <child>
> +	    <widget class="GtkHBox" id="hboxTreeButtons">
> +	      <property name="border_width">6</property>
> +	      <property name="visible">True</property>
> +	      <property name="homogeneous">False</property>
> +	      <property name="spacing">0</property>
> +
> +	      <child>
> +		<widget class="GtkVBox" id="vboxEntryTree">
> +		  <property name="visible">True</property>
> +		  <property name="homogeneous">False</property>
> +		  <property name="spacing">6</property>
> +
> +		  <child>
> +		    <widget class="GtkEntry" id="txtHeaders">
> +		      <property name="visible">True</property>
> +		      <property name="can_focus">True</property>
> +		      <property name="editable">True</property>
> +		      <property name="visibility">True</property>
> +		      <property name="max_length">0</property>
> +		      <property name="text" translatable="yes"></property>
> +		      <property name="has_frame">True</property>
> +		      <property name="invisible_char" translatable="yes">*</property>
> +		      <property name="activates_default">False</property>
> +		    </widget>
> +		    <packing>
> +		      <property name="padding">0</property>
> +		      <property name="expand">False</property>
> +		      <property name="fill">False</property>
> +		    </packing>
> +		  </child>
> +
> +		  <child>
> +		    <widget class="GtkScrolledWindow" id="scrolledwindow49">
> +		      <property name="visible">True</property>
> +		      <property name="can_focus">True</property>
> +		      <property name="hscrollbar_policy">GTK_POLICY_AUTOMATIC</property>
> +		      <property name="vscrollbar_policy">GTK_POLICY_AUTOMATIC</property>
> +		      <property name="shadow_type">GTK_SHADOW_NONE</property>
> +		      <property name="window_placement">GTK_CORNER_TOP_LEFT</property>
> +
> +		      <child>
> +			<widget class="GtkTreeView" id="treeHeaders">
> +			  <property name="visible">True</property>
> +			  <property name="can_focus">True</property>
> +			  <property name="headers_visible">False</property>
> +			  <property name="rules_hint">False</property>
> +			  <property name="reorderable">False</property>
> +			  <property name="enable_search">True</property>
> +			</widget>
> +		      </child>
> +		    </widget>
> +		    <packing>
> +		      <property name="padding">0</property>
> +		      <property name="expand">True</property>
> +		      <property name="fill">True</property>
> +		    </packing>
> +		  </child>
> +		</widget>
> +		<packing>
> +		  <property name="padding">0</property>
> +		  <property name="expand">True</property>
> +		  <property name="fill">True</property>
> +		</packing>
> +	      </child>
> +
> +	      <child>
> +		<widget class="GtkVBox" id="vbox162">
> +		  <property name="visible">True</property>
> +		  <property name="homogeneous">False</property>
> +		  <property name="spacing">0</property>
> +
> +		  <child>
> +		    <widget class="GtkButton" id="cmdHeadersAdd">
> +		      <property name="visible">True</property>
> +		      <property name="sensitive">False</property>
> +		      <property name="can_focus">True</property>
> +		      <property name="label">gtk-add</property>
> +		      <property name="use_stock">True</property>
> +		      <property name="relief">GTK_RELIEF_NORMAL</property>
> +		    </widget>
> +		    <packing>
> +		      <property name="padding">6</property>
> +		      <property name="expand">False</property>
> +		      <property name="fill">False</property>
> +		    </packing>
> +		  </child>
> +
> +		  <child>
> +		    <widget class="GtkButton" id="cmdHeadersRemove">
> +		      <property name="visible">True</property>
> +		      <property name="can_focus">True</property>
> +		      <property name="label">gtk-remove</property>
> +		      <property name="use_stock">True</property>
> +		      <property name="relief">GTK_RELIEF_NORMAL</property>
> +		    </widget>
> +		    <packing>
> +		      <property name="padding">0</property>
> +		      <property name="expand">False</property>
> +		      <property name="fill">False</property>
> +		    </packing>
> +		  </child>
> +		</widget>
> +		<packing>
> +		  <property name="padding">6</property>
> +		  <property name="expand">False</property>
> +		  <property name="fill">True</property>
> +		</packing>
> +	      </child>
> +	    </widget>
> +	    <packing>
> +	      <property name="padding">0</property>
> +	      <property name="expand">True</property>
> +	      <property name="fill">True</property>
> +	    </packing>
> +	  </child>
> +	</widget>
> +	<packing>
> +	  <property name="tab_expand">False</property>
> +	  <property name="tab_fill">True</property>
> +	</packing>
> +      </child>
> +
> +      <child>
> +	<widget class="GtkLabel" id="lblHeaders">
> +	  <property name="visible">True</property>
> +	  <property name="label" translatable="yes">H_eaders</property>
> +	  <property name="use_underline">True</property>
> +	  <property name="use_markup">False</property>
> +	  <property name="justify">GTK_JUSTIFY_LEFT</property>
> +	  <property name="wrap">False</property>
> +	  <property name="selectable">False</property>
> +	  <property name="xalign">0.5</property>
> +	  <property name="yalign">0.5</property>
> +	  <property name="xpad">0</property>
> +	  <property name="ypad">0</property>
> +	</widget>
> +	<packing>
> +	  <property name="type">tab</property>
> +	</packing>
> +      </child>
>      </widget>
>    </child>
>  </widget>
> 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	3 Dec 2003 17:16:45 -0000
> @@ -233,6 +233,24 @@
>      </schema>   
>  
>      <schema>
> +      <key>/schemas/apps/evolution/mail/display/headers</key>
> +      <applyto>/apps/evolution/mail/display/headers</applyto>
> +      <owner>evolution-mail</owner>
> +      <type>list</type>
> +      <list_type>string</list_type>
> +      <default>[]</default>
> +      <locale name="C">
> +         <short>List of custom headers and whether they are enabled.</short>
> +         <long>
> +           This key should contain a list of XML structures specifying custom headers, 
> +           and whether they are to be displayed. The format of the XML structure is 
> +           &lt;header enabled&gt; - set enabled if the header is to be displayed 
> +           in the mail view.
> +         </long>
> +      </locale>
> +    </schema>
> +
> +    <schema>
>        <key>/schemas/apps/evolution/mail/display/mime_types</key>
>        <applyto>/apps/evolution/mail/display/mime_types</applyto>
>        <owner>evolution-mail</owner>
>