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

[Not Zed <notzed ximian com>]

On Fri, 2004-04-30 at 20:17 +0530, Sarfraaz Ahmed wrote:
>     Hi,
>     
>     I have incorporated all your review comments. I am attaching my new updated patch now. Please review it and let me know if it is fine.

There are some style issues mainly.  And an important one wrt i18n.

>      static gboolean
>     -display_license(const char *filename)
>     +display_license (const char *filename, const char *lic_name)
>      {
>             GladeXML *xml;
>             GtkWidget *top_widget;
>     @@ -225,6 +210,8 @@
>             GtkButton *button_yes, *button_no;
>             GtkCheckButton *check_button;
>             GtkResponseType response;
>     
you need to initialise response, since in the 'goto failed' case it wont be set.
>     
>     +       GtkLabel *top_label;
>     +       char *label_text, *dialog_title;
>             gboolean status;
>             
>             xml = glade_xml_new (EVOLUTION_GLADEDIR "/mail-license.glade", 
>     @@ -246,25 +233,36 @@
>             check_button = GTK_CHECK_BUTTON (glade_xml_get_widget (xml, 
>                                                             "lic_checkbutton"));
>      
>     +       top_label = GTK_LABEL (glade_xml_get_widget (xml, "lic_top_label"));
>     +
>     +       label_text = g_strdup_printf ("
>     +                Please read carefully the license agreement
>     +                for %s displayed below
>     +                and tick the check box for accepting it
>     +                       ", lic_name);
multiline strings like this are illegal, you need to break them up such as:
"Please read carefully ..."
"for %s ..."

etc.  i.e. separate strings on different lines.  They can't include embedded newlines.

Also, this string must be translatable.

Also .. since this funciton is just using stuff from the provider, jsut pass the provider, rather than each field separately.

>     +       gtk_label_set_label (top_label, label_text);
>     +
>     +       dialog_title = g_strdup_printf ("%s License Agreement", lic_name);
This also needs translation (wrapped in _()).
>     +       gtk_window_set_title (GTK_WINDOW (top_widget), dialog_title);
>     +
>             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;
>     -       }
>     +       
>     +       g_free (label_text);
>     +       g_free (dialog_title);
>     +
>      failed:
>             gtk_widget_destroy (top_widget);
>             g_object_unref (xml);
>     -       return FALSE;
>     +       
>     +       if (response == GTK_RESPONSE_ACCEPT)
>     +               return TRUE;
>     +       else
>     +               return FALSE;
much easier to read:
  return response == GTK_RESPONSE_ACCEPT;

>      }
>      
>      static gboolean
>     @@ -331,32 +329,43 @@
>      mail_account_gui_check_for_license (CamelProvider *prov)
>      {
>             GConfClient *gconf;
>     -       gboolean accepted, status;
>     -       char *gconf_license_key;
>     +       gboolean accepted = FALSE, status;
>     +       GSList * providers_list, *tmp_list;
>     +       int i;
>      
>             if (prov->flags & CAMEL_PROVIDER_HAS_LICENSE) {
>                     gconf = mail_config_get_gconf_client ();
>     -               gconf_license_key = g_strdup_printf (
>     -                       "/apps/evolution/mail/licenses/%s_accepted",
>     -                       prov->license);
>                     
>     -               accepted = gconf_client_get_bool (gconf, gconf_license_key,
>     -                                                 NULL);
>     +               providers_list = gconf_client_get_list (gconf, 
>     +                               "/apps/evolution/mail/licenses", 
>     +                               GCONF_VALUE_STRING, NULL);
dont wrap at 80 columns.  it doesn't work in evo source.

>     +               tmp_list = providers_list;
>     +               while (tmp_list)
>     +               {
>     +                       if (!strcmp ((char *)tmp_list->data, prov->protocol)) {
>     +                                accepted = TRUE;
>     +                                break;
>     +                        }
>     +                       tmp_list = g_slist_next (tmp_list);
>     +               }
better:
     for (l = providers_list;l && !accepted;l=g_slist_next(l))
        accepted = strcmp(tmp_list->data, prov->protocol) == 0;

(we use 'l' for a temp list all over the place, so its simpler than tmp_list too, but it isn't so important).

>                     if (!accepted) {
>                             /* Since the license is not yet accepted, pop-up a 
>                              * dialog to display the license agreement and check 
>                              * if the user accepts it
>                              */
>      
>     -                       if (display_license (prov->license_file)) {
>     -                               status = gconf_client_set_bool (gconf, 
>     -                                               gconf_license_key, TRUE, NULL);
>     +                       if (display_license (prov->license_file, 
>     +                                               prov->license_name)) {
>     +                               providers_list = g_slist_append (providers_list,
>     +                                                (gpointer)prov->protocol);
>     +                               status = gconf_client_set_list (gconf, 
>     +                                               "/apps/evolution/mail/licenses",
>     +                                               GCONF_VALUE_STRING,
>     +                                                providers_list, NULL);
>                             } else {
>     -                               g_free (gconf_license_key);
>                                     return FALSE;
>                             }
>                     }

you still need to free the providers list.

>     -               g_free (gconf_license_key);
>             }
>             return TRUE;
>      }

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

Novell, Inc.