Re: [evolution-patches] seeking review for patch for bug 55102
- From: Jeffrey Stedfast <fejj ximian com>
- To: Not Zed <notzed ximian com>
- Cc: calvin liu sun com, evolution-patches ximian com
- Subject: Re: [evolution-patches] seeking review for patch for bug 55102
- Date: Sat, 13 Mar 2004 08:43:05 -0500
On Sat, 2004-03-13 at 05:11, Not Zed wrote:
> Also check the bug. I closed it since it actually works in 1.5.x (or
> one very like it), since the correct fix has actually been applied
> (although may be incomplete).
>
> '%' is still an issue. I'm not sure of the best fix for it yet. It
> may just be practical to disable its use (if existing folders use it it
> is ok, but if you create a new folder it wont).
Actually I fixed the % issue for local folders. the problem was that
mbox/mh/maildir stores weren't properly encoding the fragment.
the IMAP code is the only one left to fix, and that code seems as though
it is hex decoding the 'name'. According to your comments in bugzilla,
it is doing this on purpose because it is expecting the mail/ code to be
hex escaping certain chars in the names? which chars is it expecting to
be hex escaped? I think I remember something about hex escaping '/' so
that IMAP servers that didn't use '/' as their dir-sep could use that
char in the folder name, but this hack is no longer needed with the new
tree code (since it doesn't use path strings to populate the tree).
I guess we'd still have to worry about code that tried to use the
fi->path to figure out the basename/etc.
Jeff
>
> On Thu, 2004-03-11 at 11:27 -0500, Jeffrey Stedfast wrote:
>
> > 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;
> > > > +}
> > > >
>
> _______________________________________________
> Evolution-patches mailing list
> Evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches
--
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com - www.ximian.com
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
