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 _______________________________________________ Evolution-patches mailing list Evolution-patches lists ximian com http://lists.ximian.com/mailman/listinfo/evolution-patches
|
Thanks -- Sarfraaz Ahmed <asarfraaz novell com> |