Re: [evolution-patches] Another crack at custom headers

[Not Zed <notzed ximian com>]

On Sat, 2003-11-29 at 06:10, Grahame Bowland wrote:
> Ok, I've tried to merge in as much of dobey and NotZed's feedback as 
> possible. This new patch:
>  * uses XML in gconf to store settings, avoiding the two gconf keys
>    problem of the last patch
>  * has a revamped user interface with much better feedback. The new 
>    header is typed into the edit field at the top; the Add button is 
>    active only if the entered header is valid and unique.
>    the check box now selects whether or not the custom header is 
>    displayed in the view

If you're going to do it this way, you should probably add the default system headers into the list always, so the user can disable any of those they wish to (and not let the user remove them, but allow them to disable them).  Also, the flags are no longer accessible to the ui, whats the use of storing them?

A couple of style issues:

struct names should start with _, type names are the same without the leading _.

e.g. struct _EMMailerCustomHeader

(should probably be EMMailerPrefsHeader for consistency too).

Actually I suggest you just use EMFormatHeader structure directly, it already has the right fields/flags, and you can just add a high bit flag for enabled.  Store them in an EDList if you want to keep them around.  And perhaps subclass it, and add a bitfield for the enabled flag, and extend the xml to be something like:

<header name="foo" enabled flags=0>

i.e.
struct _EMMailerPrefsHeader {
    EMFormatHeader header;
    int enabled:1;
};


Use the basic c types for the basic c types.  Not the gchar, gint, etc equivalents.

Things like header->xml and xml->header should take and return pointers to newly g_allocated data.  e.g.

  EMFormatHeader *xml_to_header(char *xml)

rather than passing around addresses of structs.  Then add a 'free header' method.  This makes memory management simpler.  (and, does a gtkliststore of a string copy the string?  i think you're leaking 'name').

Put a comment above the xml_to_header stuff describing what the xml structure is that you're reading/writing to.  even just a line like the one above showing what the xml looks like will do.

Oh and you need to add an entry to the gconf schema file (evolution-mail.schemas) for the new gconf key.


>  * correctly refreshes the mail view when apply is pressed
>  * keyboard shortcuts should be correct, and the UI should be more 
>    HIG compliant
>  * actually attempted to write a complete changelog
>
> Screenshot of the configuration interface:
> http://grahame.angrygoats.net/images/custom-mail-headers2.png
>
> Screenshot of the resulting email display:
> http://grahame.angrygoats.net/images/custom-mail-headers-view2.png
>
> Patch is attached. It should be pretty clean, and I don't think 
> it leaks memory or does anything hideous.
>
> dobey was saying on IRC that he'd like this to go into the View 
> menu somewhere, so that arbitrary custom views can be defined. 
> I'm really not sure where to start with that, but I can give it 
> a try. It'd be nice to commit this and then look for a generic 
> solution - what do people think? I'm a bit scared of bonoboui :)

Don't do this, it only belongs in the config page, the view menu already has a way to set this up - its the 'normal view'.