Re: [evolution-patches] Proxy Login Patches

[Shreyas <sshreyas novell com>]

Hey, 

The Indentation was really bad in the patch cos i used diff -upw  and
not because the  code is not indented well.  This is especially true
when i do 

existing : 
if (foo) {
 /*balh*/
 } 

to 

if (foo_bar) {
	if (foo) {
	/*blah*/
	/*blah_blah*/
	}
}

diff mistakes the level indentation as redundant white space (cos of the
w option) and hence ignores it in the patch. Will keep that in mind next
time.

> i.e. not this.
>  
> >         char *drafts_folder_uri, *sent_folder_uri;
> >  
> > @@ -130,6 +139,7 @@ typedef struct _EAccount {
> >         gboolean smime_sign_default;
> >         gboolean smime_encrypt_to_self;
> >         gboolean smime_encrypt_default;
> > +       GList *proxy_children;
> 
> You dont need to store this at all, it can be determined by iterating
> over all accounts and checking if the parent uid matches the current
> one.  It saves having to try to keep a bunch of different values up to
> date.  The parent uid is the only thing which can't change, so only
> store that.
> 

A few assumptions we made :

If the parent has created proxies then it cannot be  edited unless  all
the proxies are "logged out".  

This was done as we believed that revalidating whether some change to
the parents properties broke the "validity" of the proxy it created
unnecessary. E.g: If the parent url was changed or the parent protocol
was changed. The existence of the proxy in terms of a user view is
purely as a logical account  associated with the parent. So anything
that would break that association would mean that the proxy accounts
would be disabled/ removed. 

Because of the above lines of thought we enforced the following rules
1) Any account which has created proxies would not be editable in the
account -> preferences setup 2) The proxy accounts wont be listed in the
Account Editor  3) Whenever an account which has proxies is disabled/
removed all its proxies will be removed. 

You are right i will remove the Glist which stores uids of the proxies
it has created.

> > @@ -486,6 +505,17 @@ e_account_set_from_xml (EAccount *accoun
> >                                         }
> >                                 }
> >                         }
> > +               } else if (!strcmp (node->name, "proxy")) {
> > +                       if (node->children) {
> > +                               for (cur = node->children; cur; cur =
> > cur->next) {
> > +                                       if (!strcmp (cur->name,
> > "parent-source-url")) {
> > +                                               changed |=
> > xml_set_content (cur, &account->proxy_account->parent_url);
> > +                                       } else if (!strcmp (cur->name,
> > "parent-uid")) {
> > +                                               changed |=
> > xml_set_content (cur, &account->proxy_account->parent_uid);
> > +                                               break;
> > +                                       }
> > +                               }
> > +                       }
> >                 }
> 
> Do you need to save these settings?  Dont they just need to be around at
> runtime?  Do you need this to communicate the values around?  Aren't
> they stored on the server anyway?
> 
> If they dont need to be saved, then you dont need to clear the proxies
> on startup, and you just dont save them to gconf when they are created,
> or need to handle loading them.  But if they are needed for
> comminication then i guess there's not much that can be done about
> it ...
> 
The Groupwise listener needs that to add corresponding ESources as you
already guessed. 

> > +
> > +void 
> > +e_account_list_remove_account_proxies (EAccountList *accounts,
> > EAccount *account)
> > +{
> > +       int i,n;
> > +       EAccount *child_account;
> > +       char *uid;
> > +
> > +       n = g_list_length (account->proxy_children);
> > +       for (i=0; i<n; i++) {
> > +               uid = g_list_nth_data(account->proxy_children, i);
> 
> Ugh, please dont iterate through a list like this, it is excrutiating to
> even look at.  Iterate it properly, using g_list_next.  You've got a
> nice O(n*n) algorithm for an inherently O(n) problem.
> 
doh. "sad" mistake. Will take care of it.

> >                 
> > -               if (account) {
> > +               if (account && !account->proxy_account->parent_url
> > && !g_list_length(account->proxy_children)) {
> >                         EMAccountEditor *emae;
> >  
> >                         /** @HookPoint-EMConfig: Mail Account Editor
> > @@ -227,6 +227,7 @@ account_delete_clicked (GtkButton *butto
> >         GtkTreeModel *model;
> >         GtkTreeIter iter;
> >         int ans;
> > +       gboolean has_spawned_proxies = FALSE;
> 
> No need to write a book to explain the variable name.  The teaching of
> computer programming must've changed since I was at uni; we were told
> explictly to make variable names readable, but also short and to the
> point.  has_proxies would be more than enough here.

heh, You think the book will sell ;)? Just an indication of the time i
wrote the code.
Will name it better.

> > -
> > -               /* let the rest of the application know it changed */
> > +                               mail_config_remove_account_proxies
> > (account);
> > +                               gtk_widget_set_sensitive (GTK_WIDGET
> > (prefs->mail_edit), 1);
> 
> Why are you changing the edit sensitivity here?  Can't you edit accounts
> with proxies?  How do you add more proxies?  Aren't the proxy accounts
> never listed in the first place?
> 
Nope, The account which has created proxies cannot be edited. The proxy
addition as of now is through "right click" on the store. And i need to
validate it as a menu entry since its only valid on "Groupwise
Accounts".

Thanks for the review. Will fix all the issues and re-send the patch. 

Cheers,
Shreyas