Re: [evolution-patches] camel provider changes for connector

[Jeffrey Stedfast <fejj ximian com>]

On Thu, 2004-04-22 at 10:17, Sarfraaz Ahmed wrote:
> Hi Jeff/NotZed,
> 
> Thanks a lot for those review comments. I have done the changes you
> and NotZed had suggested. I have attached the updated patch with this
> mail. But, i wanted to understand what i was doing wrong, so i have a
> few silly questions written down in the mail. Please help me
> understand them better.

sure thing

> 
> > I would say get rid of the BUFSIZE macro.
> > 
> Done
> > +       GtkTextIter iter;
> > +       g_sprintf (fname, "%s/../%s", EVOLUTION_GLADEDIR, filename);
> > 
> > this looks pretty dodgy.
> > 
> > 1. don't use a static buffer for the filename
> > 2. don't use "../" in a path, better to create a new string macro
> > defined in configure.in or wherever for these license files - or
> > maybe the connector can just hold onto the entire filename itself
> > (and may in fact need to?)
> > 
> Yeah, that was a lazy workaround ... now, i have modified the code so
> that the mail provider should directly provide the absolute file name.
> camel config, just uses it. Apart from that, what's this funda of
> static buffer ?? I did not understand why i should not be using a
> static buffer for the filename

you can't be sure how large a filename path can be and so you could have
overflowed that static buffer.

> 
> > +
> > +               count = fread (filebuf, 1, (BUFSIZE - 1), fd);
> > 
> > count = fread (filebuf, 1, sizeof (filebuf), fd);
> > 
> Done ...
> 
> > +               filebuf [count] = '\0';
> > 
> > get rid of this statement
> > 
> Done ...
> 
> > +               gtk_text_buffer_insert (buffer, &iter, filebuf, -1);
> > 
> > gtk_text_buffer_insert (buffer, &iter, filebuf, count);
> > 
> Done ... but wanted to understand why using -1, and the null
> termination was wrong ..

well, it wasn't so much wrong as unneeded (and less efficient). since
you already know how long the string is, why not take advantage of it?
:-)

> > +       }
> > +               accepted = gconf_client_get_bool (gconf,
> > prov->gconf_license_bool_key, NULL);
> > 
> > I don't like prov->gconf_license_bool_key. a different name needs to
> > be used and it shouldn't be a gconf key.
> > 
> > maybe something more like prov->license which is a string that the
> > mailer appends to another string,
> > "/apps/evolution/mail/licenses/%s_accepted" to get the actual gconf
> > key.
> > 
> Yeah, your solution looks pretty neat. Now, i have modified the code
> so that the mail providers will create their respective gconf keys
> when they get installed [ so there are no new changes in the evolution
> camel schema files ], and all the providers would have to create their
> own keys under /apps/evolution/mail/licenses .. with the key name
> being <provider_specified_name>_accepted. 

cool

Jeff