Re: [evolution-patches] seeking review for patch for bug 55102

[Jeffrey Stedfast <fejj ximian com>]

As Zucchi responded to your original patch, the proper fix for this is to fix the the code to properly encode/decode these characters and to make sure all the mailer (cand camel) code uses CamelURL to parse them rather than hackishly doing it themselves.

Jeff

On Thu, 2004-03-11 at 19:09 +0800, Calvin Liu wrote:

> Hi, Jeff,
>
> I renew the patch according to your comments. Please review it. Thanks.
>
> Calvin
>
> On Thu, 2004-03-04 at 03:26, Jeffrey Stedfast wrote:
> > looks mostly ok, but a few comments below (and PLEASE, send patches with
> > Content-Disposition: inline so it is easier to reply to them and comment
> > on the patch)
> > 
> > On Wed, 2004-03-03 at 03:54, Calvin Liu wrote:
> > > Hi, there,
> > > 
> > > This bug will affect all platforms. If the mail account name contains
> > > special characters like "#", user can't access that account.
> > > 
> > > The patch is coming from the code of shell folder name validation.
> > > Please review it.
> > > 
> > > Thanks.
> > > Calvin
> > 
> > Index: mail/ChangeLog
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
> > retrieving revision 1.3142
> > diff -u -r1.3142 ChangeLog
> > --- mail/ChangeLog      3 Mar 2004 08:11:15 -0000       1.3142
> > +++ mail/ChangeLog      3 Mar 2004 09:01:38 -0000
> > @@ -1,3 +1,9 @@
> > +2004-03-03  Calvin Liu <calvin liu sun com>
> > +       * mail-account-gui.c
> > +       add mail_account_name_is_valid () and change
> > mail_account_gui_save
> > +       to check if account name is valid before save it.
> > +       fixes bug 55102
> > +
> >  2004-03-03  Not Zed  <NotZed Ximian com>
> >  
> >         ** See bug #54924.
> > 
> > Index: mail/mail-account-gui.c
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/mail/mail-account-gui.c,v
> > retrieving revision 1.148
> > diff -u -r1.148 mail-account-gui.c
> > --- mail/mail-account-gui.c     24 Feb 2004 21:31:19 -0000      1.148
> > +++ mail/mail-account-gui.c     3 Mar 2004 08:58:01 -0000
> > @@ -1949,6 +1949,7 @@
> >         const char *new_name;
> >         gboolean is_storage;
> >         GSList *signatures;
> > +       const char *reason = NULL;
> > 
> > 
> > probably doesn't need to be set to NULL here, but ok...
> > 
> >         
> >         if (!mail_account_gui_identity_complete (gui, NULL) ||
> >             !mail_account_gui_source_complete (gui, NULL) ||
> > @@ -1963,6 +1964,12 @@
> >          * here. */
> >         
> >         new_name = gtk_entry_get_text (gui->account_name);
> > +        if (! mail_account_name_is_valid (new_name, &reason)){
> > 
> > remove the space after the ! and add a space before the {
> > (to be consistent with the rest of the code formatting)
> > 
> > +                e_notice (gui->account_name, GTK_MESSAGE_ERROR,
> > +                                  _("The specified account name is not
> > valid: %s"), reason);
> > 
> > looks like you used spaces here rather than tab to indent the wrapped
> > line (use tab)
> > 
> > 
> > +                return FALSE;
> > +        }
> > +
> >         account = mail_config_get_account_by_name (new_name);
> >         
> >         if (account && account != new) {
> > @@ -2092,3 +2099,43 @@
> >         g_free (gui->sent_folder_uri);
> >         g_free (gui);
> >  }
> > +
> > +gboolean
> > +mail_account_name_is_valid (const char *name,
> > +                              const char **reason_return)
> > 
> > 
> > don't bother wrapping this line. this is the age of 1024x768
> > resolutions, we don't need to wrap at 80 columns :-)
> > 
> > also, "reason_return" is inconsistent naming with the way we do this
> > everywhere else in the code. Just call it "reason".
> > 
> > 
> > +{
> > +        if (name == NULL || *name == '\0') {
> > +                if (reason_return != NULL)
> > 
> > since this function is only ever called from one place which always
> > passes in &reason, do we really need this check? I don't think we do...
> > 
> > +                        *reason_return = _("No account name
> > specified.");
> > +                return FALSE;
> > +        }
> > +                                                                                                                                         
> > 
> > instead of doing the following check, what I would do is check for \r
> > above the empty-name check and simply remove \r's. No need to trouble
> > the user about characters he clearly didn't mean to have as part of his
> > account name.
> > 
> > +        /* GtkEntry is broken - if you hit KP_ENTER you get a \r
> > inserted... */
> > +        if (strchr (name, '\r')) {
> > +                if (reason_return != NULL)
> > +                        *reason_return = _("Account name cannot contain
> > the Return character.");
> > +                return FALSE;
> > +        }
> > +                                                                                                                                         
> > +        if (strchr (name, '/') != NULL) {
> > +                if (reason_return != NULL)
> > +                        *reason_return = _("Account name cannot contain
> > the character \"/\".");
> > +                return FALSE;
> > +        }
> > +                                                                                                                                         
> > +        if (strchr (name, '#') != NULL) {
> > +                if (reason_return != NULL)
> > +                        *reason_return = _("Account name cannot contain
> > the character \"#\".");
> > +                return FALSE;
> > +        }
> > +                                                                                                                                         
> > +        if (strcmp (name, ".") == 0 || strcmp (name, "..") == 0) {
> > +                if (reason_return != NULL)
> > +                        *reason_return = _("Account name cannot contain
> > the character \".\".");
> > +                return FALSE;
> > +        }
> > +                                                                                                                                         
> > +        *reason_return = NULL;
> > 
> > hehe, you went thru all the trouble of checking that reason_return !=
> > NULL in the error conditions above and didn't bother checking it here?
> > Shame on you :-)
> > 
> > also, get rid of this. it is unneeded (reason_return only gets used if
> > this function returns FALSE, so no need to ever set it if we return
> > TRUE).
> > 
> > +                                                                                                                                         
> > +        return TRUE;
> > +}
> >