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

[Sarfraaz Ahmed <asarfraaz novell com>]

Hi,

Thanks a lot for your comments, and helping me understand about using gconf on irc

> >         +#define FILENAME EVOLUTION_GLADEDIR "/mail-license.glade"
> >         +#define ROOTNODE "lic_dialog"
> >     
>     
>     ROOTNODE of what?  FILENAME of what? This file has a lot of shit in it, these mean nothing, just put them direfctly where they're used - they're only used in one place anyway.  Like every other single instance of the many other glade calls already do in this file (consistency).
>     
Done ... maintained the consistency

>     Just using EVOLUTION_PRIVDATADIR should be sufficient, I think.  Actually the filename should be a full path anyway.  Not relative to any evolution directory, its a camel provided path.  Or it should be relative to something in the camel provider, not a hardcoded evolution path.
>     
Yeah, i think the decision about where to put the license file should be left to the provider, as it might install its files in its own location. This way, the autoconfig code can be more generic. I have modified the code to directly use the absolute filename provided by the camel-provider.

>     You probalby also need to check ferror(), i dont think feof() will check that.
>     
Done ...
> >         +       }
> >         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.
> >     
>     
>     I agree.  Since presumably you can only have 1 license, it should be based on the provider name, no need for any 'key' value, that is an application-level issue, not a provider one (maybe).
Yeah, each provider could have just one license. I have now left the provider to decide on this name, since the provider needs to create the gconf entry/key when it gets installed.

>     
>     Is there any likelyhook of requiring multiple licenses?
No, i dont think so. But even if we have such a case in future, I guess, we can always club the licenses into one single license file. But yeah, that may not be needed for now.

> >         +
> >         +               status = display_license (prov->license_file);
> >     
>     
>     This should use a full pathname.
Yeah, this makes sense also, since the provider would install the files in its respective directories, which the camel need not have to worry about. Modified the code to have absolute filenames.

> >         +#define CAMEL_PROVIDER_CHECK_LICENSE    (1 << 6)
> >     
>     how about 
>     
>     HAS_LICENSE
>     
>     It doesn't do any checks itself.
>     
This was cool ... Looks much better now

> >          
> >         +       const char *gconf_license_bool_key;
> >         
> >         as stated above, I don't think this should be a gconf key but rather just the name of the connector license or something.
> >     
>     
>     If we only have one license, it shouldn't even exist.
>     
True, but for now, added this, in case we have to extend this to any other provider which would also need license acceptance.

> >         +
> >         +       /* This holds the license file name [ ascii text format ] containing
> >         +        * the license agreement. This is read only when the CHECK_LICENSE
> >         +        * flag is set
> >         +        */
> >         +       const char *license_file;
> >          } CamelProvider;
> >     
>     Full path?  Relative to camel provider location?  Or should it just be implied by having a libcamel<xxx>-license.txt file in the right llocation as the .urls stuff is?  
Thanks for mentioning about these .urls files. The code in this actually helped me in fixing the ugly ../%s code i had added earlier in this patch. I was wondering, how i could use a configure.in variable in my c code, it just had to be added as a defined variable name for the compiler [ using the -D option in the includes variable in Makefile ]

>     Should the license file be html for better presentation?
>     
This is a good idea, but i have usually seen license agreements just displayed as text docs. I have left this as it is for now. Moreover, we would need a html renderer for displaying this html file in the autoconfig code.

Michael Zucchi <notzed ximian com> Ximian Evolutionand Free Software Developer

Novell, Inc.

        Thanks
                        --     Sarfraaz Ahmed <asarfraaz novell com>