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

[Not Zed <notzed ximian com>]

Looks pretty good, although I was hoping you'd follow my suggestion on irc for the license key.  This way there is no need to create any extra key at install time for each license.

e.g.
in the camel provider you have a 'license id', infact if you're going to ONLY have 1 license per provider, you probably don't even need that, you can use the provider name.  i.e. 'imap' 'groupwise', etc.

in evolution, you have a single key defined
/apps/evolution/licenses
or even
/apps/evolution/mail/licenses
which is a string list.
it is initially empty.
it contains the id's of all icenses accepted.

The code follows obviously from that.

On Thu, 2004-04-22 at 19:47 +0530, 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.

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

+
+               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 ..

+       }
+               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.

+

-- 
Jeffrey Stedfast
Evolution Hacker - Novell, Inc.
fejj ximian com  - www.novell.com

        Thanks
                        --     Sarfraaz Ahmed <asarfraaz novell com>

Michael Zucchi <notzed ximian com> Ximian Evolution and Free Software Developer

Novell, Inc.