Re: [evolution-patches] Another custom headers patch

[Jeffrey Stedfast <fejj ximian com>]

just some stylistic comments:

- in the ChangeLog, the changes to each file should be separated by a
blank line

- there should be 1 blank line after the last variable declaration

+               if (emf->message) {
+                       em_format_format_clone(emf, emf->message, emf);
+               }

- no need for the {}'s there. same for a few other locations... if there
is only one statement, no need to bracket it.

 struct _EMMailerPrefs {
-       GtkVBox parent_object;
+        GtkVBox parent_object;
        
don't change the spacing

- the changes in em-mailer-prefs.c should keep with the style and put a
space between func and ( ("func(" -> "func (")

similar for:

for (i=sizeof(default_headers)/sizeof(default_headers[0])-1;i>=0;i--) {

that should be:

for (i = sizeof (default_headers) / sizeof (default_headers[0]) - 1; i
>= 0; i--) {

+               if (enabled) {
+                       h.enabled = 1;
+               } else {
+                       h.enabled = 0;
+               }

that should just be:

h.enabled = enabled;


+       h = g_malloc(sizeof(struct _EMMailerPrefsHeader));
+       if (doc == NULL) {
+               return NULL;
+       }

you leak 'h' if doc is NULL. similar problem elsewhere in that function.
what you should do is wait to malloc h until after the checks.

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

no need for the NULL check, g_free (and even free) handles NULL.

is_valid_header() should check that the header string is us-ascii

don't hold on to these:

+	GtkCellRenderer *header_list_name_renderer;
+	GtkCellRenderer *header_list_enabled_renderer;

you never use them anywhere after you've added them to the treeview.

you cn probably get away without holding on to the model too, since you
can just get it from the treeview when you need it. but that is your
call...

Michael and I also generally prefer (Type *) type casting rather than
TYPE() type casting.

the code looked ok to me other than that, I'll give it a try as soon as
I have a working 1.5 install on my laptop here...


Jeff

On Wed, 2003-12-03 at 12: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.
> 
> Anyway, patch attached. Feedback, everyone? :)
> 
> Cheers
> Grahame
> 
> PS: copyright assignment letter is in the mail to Ximian. 
>