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

[Not Zed <notzed ximian com>]

On Mon, 2004-04-19 at 16:44 -0400, Jeffrey Stedfast wrote:
>     a few comments below
>     
>     On Mon, 2004-04-19 at 18:40 +0530, Sarfraaz Ahmed wrote:
> >         Hi,
> >         
> >         I should have sent this patch earlier, but remembered it only today.
> >         
> >         This patch essentially adds a flag in camel-provider, which the providers can set to indicate that they need a license file to be displayed before completing the configuration through the autoconfig wizard.
> >         
> >         It also has the code to display the license file. [ a text file ]. I am also attaching the glade file for this.
> >         
> >         We would be needing this for exchange connector currently, though the code for making the autoconfig wizard invoke the provider backend apis is yet to be done. It would be good if this could be reviewed soon, so that atleast the UI changes get in before the freeze.
> >     
>     
>     Index: camel/ChangeLog
>     ===================================================================
>     RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
>     retrieving revision 1.2103
>     diff -u -r1.2103 ChangeLog
>     --- camel/ChangeLog     16 Apr 2004 18:34:55 -0000      1.2103
>     +++ camel/ChangeLog     19 Apr 2004 13:00:18 -0000
>     @@ -1,3 +1,8 @@
>     +2004-04-19  Sarfraaz Ahmed <asarfraaz novell com>
>     +
>     +       * camel-provider.h: Added flags for allowing camel providers to 
>     +       display license file.
>     +
>      2004-04-16  Jeffrey Stedfast  <fejj ximian com>
>      
>             * camel-vee-store.c (change_folder): (flags & 0) will never be
>     Index: camel/ChangeLog
>     ===================================================================
>     RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
>     retrieving revision 1.2103
>     diff -u -r1.2103 ChangeLog
>     --- camel/ChangeLog     16 Apr 2004 18:34:55 -0000      1.2103
>     +++ camel/ChangeLog     19 Apr 2004 13:01:01 -0000
>     @@ -1,3 +1,8 @@
>     +2004-04-19  Sarfraaz Ahmed <asarfraaz novell com>
>     +
>     +       * camel-provider.h: Added flags for allowing camel providers to 
>     +       display license file.
>     +
>      2004-04-16  Jeffrey Stedfast  <fejj ximian com>
>      
>             * camel-vee-store.c (change_folder): (flags & 0) will never be
>     Index: mail/mail-account-gui.c
>     ===================================================================
>     RCS file: /cvs/gnome/evolution/mail/mail-account-gui.c,v
>     retrieving revision 1.158
>     diff -u -r1.158 mail-account-gui.c
>     --- mail/mail-account-gui.c     8 Apr 2004 22:02:36 -0000       1.158
>     +++ mail/mail-account-gui.c     19 Apr 2004 13:02:02 -0000
>     @@ -27,6 +27,8 @@
>      #include <config.h>
>      #endif
>      
>     +#include <glib.h>
>     +
>      #include <string.h>
>      #include <stdarg.h>
>      
>     @@ -56,6 +58,9 @@
>      
>      #define d(x)
>      
>     +#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).

>      static void save_service (MailAccountGuiService *gsvc, GHashTable *extra_conf, EAccountService *service);
>      static void service_changed (GtkEntry *entry, gpointer user_data);
>      
>     @@ -155,6 +160,109 @@
>             g_free (value);
>      }
>      
>     +static void 
>     +set_license_rejected (GtkWidget *widget, gpointer data)
>     +{
>     +       gtk_dialog_response (GTK_DIALOG (data), GTK_RESPONSE_DELETE_EVENT);
>     +       return;
>     +}
>     +
>     +static void
>     +set_license_accepted (GtkWidget *widget, gpointer data)
>     +{
>     +       gtk_dialog_response (GTK_DIALOG (data), GTK_RESPONSE_ACCEPT);
>     +       return;
>     +
>     +}
>     +
>     +static void
>     +check_button_state (GtkToggleButton *button, gpointer data)
>     +{
>     +       if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON (button)))
>     +               gtk_widget_set_sensitive (GTK_WIDGET (data), TRUE);
>     +       else
>     +               gtk_widget_set_sensitive (GTK_WIDGET (data), FALSE);
>     +}
>     +
>     +#define BUFSIZE 1024
>     +
>     +static gboolean
>     +populate_text_entry (GtkTextView *view, const char *filename)
>     +{
>     +       FILE *fd;
>     +       char filebuf[BUFSIZE];
>     
>     I would say get rid of the BUFSIZE macro.
>     
>     +       GtkTextIter iter;
>     +       GtkTextBuffer *buffer;
>     +       char fname[BUFSIZE];
>     +       int count;
>     +
>     +       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?)

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.

>     +
>     +       fd = fopen (fname, "r");
>     +       
>     +       if (!fd) {
>     +               /* FIXME: Should never come here */
>     +               return FALSE;
>     +       }
>     +
>     +       buffer =  gtk_text_buffer_new (NULL);
>     +       gtk_text_buffer_get_start_iter (buffer, &iter);
>     +
>     +       while (!feof (fd)) {
>     +               count = fread (filebuf, 1, (BUFSIZE - 1), fd);
>     
>     count = fread (filebuf, 1, sizeof (filebuf), fd);
>     
>     +               filebuf [count] = '\0';
>     
>     get rid of this statement
>     
>     +               gtk_text_buffer_insert (buffer, &iter, filebuf, -1);
>     
>     gtk_text_buffer_insert (buffer, &iter, filebuf, count);

You probalby also need to check ferror(), i dont think feof() will check that.

>     +       }
>     +       gtk_text_view_set_buffer (GTK_TEXT_VIEW (view), 
>     +                                       GTK_TEXT_BUFFER (buffer));
>     +       fclose (fd);
>     +       return TRUE;
>     +}
>     +
>     +static gboolean
>     +display_license(const char *filename)
>     +{
>     +       GladeXML *xml;
>     +       GtkWidget *top_widget;
>     +       GtkTextView *text_entry;
>     +       GtkButton *button_yes, *button_no;
>     +       GtkCheckButton *check_button;
>     +       GtkResponseType *response;
>     +       gboolean status;
>     +       
>     +       xml = glade_xml_new (FILENAME, ROOTNODE, NULL);
>     +       
>     +       top_widget = glade_xml_get_widget (xml, ROOTNODE);
>     +       text_entry = GTK_TEXT_VIEW (glade_xml_get_widget (xml, "textview1"));
>     +       status = populate_text_entry (GTK_TEXT_VIEW (text_entry), filename);
>     +       if (!status)
>     +               goto failed;
>     +
>     +       gtk_text_view_set_editable (GTK_TEXT_VIEW (text_entry), FALSE);
>     +
>     +       button_yes = GTK_BUTTON (glade_xml_get_widget (xml, "lic_yes_button"));
>     +       gtk_widget_set_sensitive (GTK_WIDGET (button_yes), FALSE);
>     +
>     +       button_no = GTK_BUTTON (glade_xml_get_widget (xml, "lic_no_button"));
>     +
>     +       check_button = GTK_CHECK_BUTTON (glade_xml_get_widget (xml, "lic_checkbutton"));
>     +
>     +       g_signal_connect (check_button, "toggled", G_CALLBACK (check_button_state), button_yes);
>     +       g_signal_connect (button_yes, "clicked", G_CALLBACK (set_license_accepted), GTK_WIDGET (top_widget));
>     +       g_signal_connect (button_no, "clicked", G_CALLBACK (set_license_rejected), GTK_WIDGET (top_widget));
>     +
>     +       response = gtk_dialog_run (GTK_DIALOG (top_widget));
>     +       if (response == GTK_RESPONSE_ACCEPT) {
>     +               gtk_widget_destroy (top_widget);
>     +               g_object_unref (xml);
>     +               return TRUE;
>     +       }
>     +failed:
>     +       gtk_widget_destroy (top_widget);
>     +       g_object_unref (xml);
>     +       return FALSE;
>     +}
>     +
>      static gboolean
>      service_complete (MailAccountGuiService *service, GHashTable *extra_config, GtkWidget **incomplete)
>      {
>     @@ -216,6 +324,34 @@
>      }
>      
>      gboolean
>     +mail_account_gui_check_for_license (CamelProvider *prov)
>     +{
>     +       GConfClient *gconf;
>     +       gboolean accepted, status;
>     +
>     +       if (prov->flags & CAMEL_PROVIDER_CHECK_LICENSE) {
>     +               gconf = mail_config_get_gconf_client ();
>     +               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.

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

Is there any likelyhook of requiring multiple licenses?
>     +
>     +               if (accepted)
>     +                       return TRUE;
>     +
>     +               /* Since the license is not yet accepted, pop-up a dialog 
>     +                * to display the license agreement and check if the user
>     +                * accepts it
>     +                */
>     +
>     +               status = display_license (prov->license_file);

This should use a full pathname.
>     +               if (!status)
>     +                       return FALSE;
>     +               
>     +               status = gconf_client_set_bool (gconf, prov->gconf_license_bool_key, TRUE, NULL);
>     +
>     +       }
>     +       return TRUE;
>     +}
>     +
>     +gboolean
>      mail_account_gui_source_complete (MailAccountGui *gui, GtkWidget **incomplete)
>      {
>             return service_complete (&gui->source, gui->extra_config, incomplete);
>     @@ -492,6 +628,7 @@
>             GtkWidget *file_entry, *label, *frame, *dwidget = NULL;
>             CamelProvider *provider;
>             gboolean writeable;
>     +       gboolean license_accepted = TRUE;
>             
>             provider = g_object_get_data ((GObject *) widget, "provider");
>             
>     @@ -521,8 +658,13 @@
>             else
>                     gtk_label_set_text (gui->source.description, "");
>             
>     +       printf("provider found \n");
>     +
>     +       if (gui->source.provider)       
>     +               license_accepted = mail_account_gui_check_for_license (gui->source.provider);
>     +
>             frame = glade_xml_get_widget (gui->xml, "source_frame");
>     -       if (provider) {
>     +       if (provider && license_accepted) {
>                     gtk_widget_show (frame);
>                     
>                     /* hostname */
>     Index: camel/camel-provider.h
>     ===================================================================
>     RCS file: /cvs/gnome/evolution/camel/camel-provider.h,v
>     retrieving revision 1.40
>     diff -u -r1.40 camel-provider.h
>     --- camel/camel-provider.h      19 Feb 2004 07:27:35 -0000      1.40
>     +++ camel/camel-provider.h      19 Apr 2004 13:02:24 -0000
>     @@ -59,6 +59,8 @@
>       * _IS_STORAGE  mail is stored there. it will appear in the folder tree.
>       * _IS_EXTERNAL it appears in the folder tree but is not created by
>       *                the mail component.
>     + * _CHECK_LICENSE  the provider configuration first needs the license to
>     + *                be accepted.
>       */
>      #define CAMEL_PROVIDER_IS_REMOTE       (1 << 0)
>      #define CAMEL_PROVIDER_IS_LOCAL                (1 << 1)
>     @@ -66,6 +68,7 @@
>      #define CAMEL_PROVIDER_IS_SOURCE       (1 << 3)
>      #define CAMEL_PROVIDER_IS_STORAGE      (1 << 4)
>      #define CAMEL_PROVIDER_SUPPORTS_SSL    (1 << 5)
>     +#define CAMEL_PROVIDER_CHECK_LICENSE    (1 << 6)
how about 

HAS_LICENSE

It doesn't do any checks itself.
 
>      
>      /* Flags for url_flags. "ALLOW" means the config dialog will let
>     @@ -182,6 +185,17 @@
>              * evolution source tree).
>              */
>             char *translation_domain;
>     +
>     +       /* This holds the gconf key string which needs to be checked/set
>     +        * for accepting the license 
>     +        */
>     +       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.

>     +
>     +       /* 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?  Should the license file be html for better presentation?

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

Novell, Inc.