Re: [evolution-patches] Proxy Login Patches
- From: Not Zed <notzed ximian com>
- To: Shreyas <sshreyas novell com>
- Cc: Evolution Patches <evolution-patches lists ximian com>, psankar novell com
- Subject: Re: [evolution-patches] Proxy Login Patches
- Date: Wed, 13 Jul 2005 14:09:23 +0800
Why was this patch included anyway, before it was approved?
On Fri, 2005-07-08 at 16:06 +0530, Shreyas wrote:
> Hey,
>
> Attaching a patch which hopefully fixes all the issues mentioned.
>
> Cheers,
> Shreyas
>
> On Thu, 2005-07-07 at 00:50 +0800, Not Zed wrote:
> > On Wed, 2005-07-06 at 15:58 +0530, Shreyas wrote:
> > > Index: e-util/ChangeLog
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/e-util/ChangeLog,v
> > > retrieving revision 1.544
> > > diff -u -p -w -r1.544 ChangeLog
> > > --- e-util/ChangeLog 18 Jun 2005 15:59:56 -0000 1.544
> > > +++ e-util/ChangeLog 5 Jul 2005 09:31:12 -0000
> > > @@ -1,3 +1,9 @@
> > > +2005-07-05 Shreyas Srinivasan
> > > +
> > > + * e-util/e-account.[ch]: Add structures to handle proxies.
> > > + * e-util/e-account-list.[ch]: Add functions to remove proxy
> > > + accounts and account's proxies.
> > > +
> > > 2005-06-18 Tor Lillqvist <tml novell com>
> > >
> > > * Makefile.am (WIN32_BOOTSTRAP_LIBS): Use bootstrap library
> > > for
> > > Index: e-util/e-account.h
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/e-util/e-account.h,v
> > > retrieving revision 1.9
> > > diff -u -p -w -r1.9 e-account.h
> > > --- e-util/e-account.h 8 Apr 2005 04:34:04 -0000 1.9
> > > +++ e-util/e-account.h 5 Jul 2005 09:31:29 -0000
> > > @@ -69,6 +69,9 @@ typedef enum _e_account_item_t {
> > > E_ACCOUNT_SMIME_ENCRYPT_TO_SELF,
> > > E_ACCOUNT_SMIME_ENCRYPT_DEFAULT,
> > >
> > > + E_ACCOUNT_PROXY_PARENT_URL,
> >
> > Dont store the parent url. You can get that, and SHOULD get that from
> > the account item which you can get from the parent uid.
> >
> > > + E_ACCOUNT_PROXY_PARENT_UID,
> > > +
> > > E_ACCOUNT_ITEM_LAST
> > > } e_account_item_t;
> > >
> > > @@ -98,6 +101,11 @@ typedef struct _EAccountService {
> > > gboolean save_passwd;
> > > } EAccountService;
> > >
> > > +typedef struct _EAccountProxyInfo {
> > > + char *parent_url;
> > > + char *parent_uid;
> > > +} EAccountProxyInfo;
> >
> > Because you dont need all this, just store the parent_uid as a simple
> > pointer on the main EAccount.
> >
> > > typedef struct _EAccount {
> > > GObject parent_object;
> > >
> > > @@ -109,6 +117,7 @@ typedef struct _EAccount {
> > > EAccountIdentity *id;
> > > EAccountService *source;
> > > EAccountService *transport;
> > > + EAccountProxyInfo *proxy_account;
> >
> > 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.
> >
> > > } EAccount;
> > >
> > > typedef struct {
> > > Index: e-util/e-account.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/e-util/e-account.c,v
> > > retrieving revision 1.15
> > > diff -u -p -w -r1.15 e-account.c
> > > --- e-util/e-account.c 8 Apr 2005 04:34:04 -0000 1.15
> > > +++ e-util/e-account.c 5 Jul 2005 09:32:58 -0000
> > > @@ -107,6 +107,10 @@ e_account_init (EAccount *account)
> > > account->id = g_new0 (EAccountIdentity, 1);
> > > account->source = g_new0 (EAccountService, 1);
> > > account->transport = g_new0 (EAccountService, 1);
> > > + account->proxy_account = g_new0 (EAccountProxyInfo, 1);
> > > +
> > > + account->proxy_account->parent_url = NULL;
> > > + account->proxy_account->parent_uid = NULL;
> > >
> > > account->source->auto_check = FALSE;
> > > account->source->auto_check_time = 10;
> > > @@ -139,6 +143,17 @@ service_destroy (EAccountService *servic
> > > }
> > >
> > > static void
> > > +proxy_destroy (EAccountProxyInfo *proxy_info)
> > > +{
> > > + if (proxy_info->parent_url)
> > > + g_free (proxy_info->parent_url);
> > > + if (proxy_info->parent_uid)
> > > + g_free (proxy_info->parent_uid);
> > > +
> > > + g_free (proxy_info);
> > > +}
> > > +
> > > +static void
> > > e_account_finalize (GObject *object)
> > > {
> > > EAccount *account = E_ACCOUNT (object);
> > > @@ -149,7 +164,11 @@ e_account_finalize (GObject *object)
> > > identity_destroy (account->id);
> > > service_destroy (account->source);
> > > service_destroy (account->transport);
> > > + proxy_destroy (account->proxy_account);
> > >
> > > + g_list_foreach (account->proxy_children, (GFunc) g_free,
> > > NULL);
> > > + g_list_free (account->proxy_children);
> > > + account->proxy_children = NULL;
> > > g_free (account->drafts_folder_uri);
> > > g_free (account->sent_folder_uri);
> > >
> > > @@ -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 ...
> >
> > Once the EAccountList is loaded for the mailer (&used by the composer),
> > each object will remain in memory, it shouldn't be overwritten even when
> > a change to the object is made - it is only other copies of eaccountlist
> > that matter.
> >
> > > }
> > >
> > > @@ -564,6 +594,7 @@ e_account_import (EAccount *dest, EAccou
> > > g_free (dest->smime_encrypt_key);
> > > dest->smime_encrypt_key = g_strdup (src->smime_encrypt_key);
> > >
> > > + dest->proxy_children = g_list_copy(src->proxy_children);
> > > g_signal_emit(dest, signals[CHANGED], 0, -1);
> > > }
> > >
> > > @@ -652,6 +683,12 @@ e_account_to_xml (EAccount *account)
> > > if (account->smime_encrypt_key)
> > > xmlNewTextChild (node, NULL, "encrypt-key-id",
> > > account->smime_encrypt_key);
> > >
> > > + if (account->proxy_account &&
> > > account->proxy_account->parent_url) {
> > > + node = xmlNewChild (root, NULL, "proxy", NULL);
> > > + xmlNewTextChild (node, NULL, "parent-source-url",
> > > account->proxy_account->parent_url);
> > > + xmlNewTextChild (node, NULL, "parent-uid",
> > > account->proxy_account->parent_uid);
> > > + }
> > > +
> > > xmlDocDumpMemory (doc, &xmlbuf, &n);
> > > xmlFreeDoc (doc);
> > >
> > > @@ -778,6 +815,9 @@ static struct _account_info {
> > > { /* E_ACCOUNT_SMIME_SIGN_DEFAULT */ 0, TYPE_BOOL,
> > > G_STRUCT_OFFSET(EAccount, smime_sign_default) },
> > > { /* E_ACCOUNT_SMIME_ENCRYPT_TO_SELF */ 0, TYPE_BOOL,
> > > G_STRUCT_OFFSET(EAccount, smime_encrypt_to_self) },
> > > { /* E_ACCOUNT_SMIME_ENCRYPT_DEFAULT */ 0, TYPE_BOOL,
> > > G_STRUCT_OFFSET(EAccount, smime_encrypt_default) },
> > > +
> > > + { /* E_ACCOUNT_PROXY_PARENT_URL, */ 0, TYPE_STRING|
> > > TYPE_STRUCT, G_STRUCT_OFFSET(EAccount, proxy_account),
> > > G_STRUCT_OFFSET(EAccountProxyInfo, parent_url) },
> > > + { /* E_ACCOUNT_PROXY_PARENT_UID, */ 0, TYPE_STRING|
> > > TYPE_STRUCT, G_STRUCT_OFFSET(EAccount, proxy_account),
> > > G_STRUCT_OFFSET(EAccountProxyInfo, parent_uid) },
> > > };
> > >
> > > static GHashTable *ea_option_table;
> > > Index: e-util/e-account-list.h
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/e-util/e-account-list.h,v
> > > retrieving revision 1.4
> > > diff -u -p -w -r1.4 e-account-list.h
> > > --- e-util/e-account-list.h 31 Mar 2004 10:08:38 -0000 1.4
> > > +++ e-util/e-account-list.h 5 Jul 2005 09:33:13 -0000
> > > @@ -71,5 +71,7 @@ void e_account_list_remove
> > > const EAccount *e_account_list_get_default(EAccountList *);
> > > void e_account_list_set_default(EAccountList *, EAccount
> > > *);
> > > const EAccount *e_account_list_find (EAccountList *,
> > > e_account_find_t type, const char *key);
> > > +void e_account_list_prune_proxies (EAccountList *);
> > > +void e_account_list_update_parent (EAccountList *, EAccount *);
> > >
> > > #endif /* __E_ACCOUNT_LIST__ */
> > > Index: e-util/e-account-list.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/e-util/e-account-list.c,v
> > > retrieving revision 1.10
> > > diff -u -p -w -r1.10 e-account-list.c
> > > --- e-util/e-account-list.c 21 Dec 2004 15:50:38 -0000 1.10
> > > +++ e-util/e-account-list.c 5 Jul 2005 09:33:32 -0000
> > > @@ -293,6 +293,56 @@ e_account_list_save (EAccountList *accou
> > > gconf_client_suggest_sync (account_list->priv->gconf, NULL);
> > > }
> > >
> > > +void
> > > +e_account_list_prune_proxies (EAccountList *account_list)
> > > +{
> > > + EAccount *account;
> > > + EIterator *iter;
> > > +
> > > + for (iter = e_list_get_iterator (E_LIST (account_list));
> > > + e_iterator_is_valid (iter);
> > > + e_iterator_next (iter)) {
> > > + account = (EAccount *)e_iterator_get (iter);
> > > + if (account->proxy_account->parent_url) {
> > > + e_account_list_remove (account_list, account);
> > > + e_account_list_save (account_list);
> >
> > You don't want to save the list every time it changes. Only do it once
> > after you're finished, if you made any changes.
> >
> > > + }
> > > + }
> > > + g_object_unref (iter);
> > > +}
> > > +
> > > +void
> > > +e_account_list_update_parent (EAccountList *accounts, EAccount
> > > *account)
> > > +{
> > > + char *parent = NULL;
> > > + EAccount *parent_account;
> > > +
> > > + parent = e_account_get_string (account,
> > > E_ACCOUNT_PROXY_PARENT_URL);
> > > + parent_account = e_account_list_find (accounts,
> > > E_ACCOUNT_FIND_UID, account->proxy_account->parent_uid);
> > > + if (parent_account) {
> > > + parent_account->proxy_children = g_list_remove
> > > (parent_account->proxy_children, account->uid);
> > > + e_account_list_change (accounts, parent_account);
> > > + }
> >
> > This is one of the reasons you dont want to keep an account list or
> > parent url on the account. If you get rid of those, the need for this
> > function above evaporates.
> >
> > > +
> > > +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.
> >
> > Anyway, since you're going to remove the proxy_children list anyway,
> > just iterate over the accounts and remove any that have a parent_uid
> > which matches the account->uid.
> >
> > > + child_account = e_account_list_find (accounts,
> > > E_ACCOUNT_FIND_UID, uid);
> > > + e_account_list_remove (accounts, child_account);
> > > + e_account_list_save (accounts);
> > > + account->proxy_children = g_list_remove
> > > (account->proxy_children, uid);
> > > + }
> > > + g_list_free (account->proxy_children);
> > > + e_account_list_save (accounts);
> > > +}
> > > /**
> > > * e_account_list_add:
> > > * @accounts:
> > > Index: mail/ChangeLog
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
> > > retrieving revision 1.3649
> > > diff -u -p -w -r1.3649 ChangeLog
> > > --- mail/ChangeLog 1 Jul 2005 03:29:22 -0000 1.3649
> > > +++ mail/ChangeLog 5 Jul 2005 10:33:10 -0000
> > > @@ -1,3 +1,13 @@
> > > +2005-07-05 Shreyas Srinivasan <sshreyas novell com>
> > > +
> > > + * mail-config.[ch]: Add new functions to remove proxy accounts
> > > and
> > > + proxies created by the account.
> > > + * mail-compnent.c: Remove proxy accounts at startup
> > > + * mail.error.xml: Add warnings for proxy disable and removal
> > > of a
> > > + proxy when its parents are removed.
> > > + * em-account-prefs.c: Add checks not to display proxies and
> > > + handling of parent removal/ disabling.
> > > +
> > > 2005-06-24 Matt Brown <matt mattb net nz>
> > >
> > > * em-inline-filter.c: implement extraction of inline
> > > signed/encrypted pgp
> > > Index: mail/mail-config.h
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/mail/mail-config.h,v
> > > retrieving revision 1.119
> > > diff -u -p -w -r1.119 mail-config.h
> > > --- mail/mail-config.h 19 Oct 2004 06:35:44 -0000 1.119
> > > +++ mail/mail-config.h 5 Jul 2005 10:33:42 -0000
> > > @@ -133,6 +133,9 @@ struct _EAccountList *mail_config_get_ac
> > > void mail_config_add_account (struct _EAccount *account);
> > > void mail_config_remove_account (struct _EAccount *account);
> > > void mail_config_set_default_account (struct _EAccount *account);
> > > +void mail_config_update_parent (struct _EAccount *account);
> > > +void mail_config_remove_account_proxies (struct _EAccount *account);
> > > +void mail_config_prune_proxies (void);
> > >
> > > struct _EAccountIdentity *mail_config_get_default_identity (void);
> > > struct _EAccountService *mail_config_get_default_transport (void);
> > > Index: mail/mail-config.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/mail/mail-config.c,v
> > > retrieving revision 1.317
> > > diff -u -p -w -r1.317 mail-config.c
> > > --- mail/mail-config.c 23 Jun 2005 09:11:06 -0000 1.317
> > > +++ mail/mail-config.c 5 Jul 2005 10:33:53 -0000
> > > @@ -671,6 +671,25 @@ mail_config_get_account_by_transport_url
> > > return NULL;
> > > }
> > >
> > > +
> > > +void
> > > +mail_config_update_parent (EAccount *account)
> > > +{
> > > + e_account_list_update_parent (config->accounts, account);
> > > +}
> >
> > Wont be needed if you remove the data duplication in EAccount.
> >
> > > +void
> > > +mail_config_remove_account_proxies (EAccount *account)
> > > +{
> > > + e_account_list_remove_account_proxies (config->accounts,
> > > account);
> > > +}
> > > +
> > > +void
> > > +mail_config_prune_proxies (void)
> > > +{
> > > + e_account_list_prune_proxies (config->accounts);
> > > +}
> > > +
> > > EAccountList *
> > > mail_config_get_accounts (void)
> > > {
> > > Index: mail/mail-component.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/mail/mail-component.c,v
> > > retrieving revision 1.120
> > > diff -u -p -w -r1.120 mail-component.c
> > > --- mail/mail-component.c 23 Jun 2005 09:11:06 -0000 1.120
> > > +++ mail/mail-component.c 5 Jul 2005 10:34:12 -0000
> > > @@ -359,6 +359,7 @@ mc_startup(MailComponent *mc)
> > > started = 1;
> > >
> > > mc_setup_local_store(mc);
> > > + mail_config_prune_proxies ();
> > > load_accounts(mc, mail_config_get_accounts());
> > > vfolder_load_storage();
> > > }
> > > Index: mail/mail.error.xml
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/mail/mail.error.xml,v
> > > retrieving revision 1.3
> > > diff -u -p -w -r1.3 mail.error.xml
> > > --- mail/mail.error.xml 7 Jun 2005 05:05:43 -0000 1.3
> > > +++ mail/mail.error.xml 5 Jul 2005 10:34:29 -0000
> > > @@ -249,6 +249,25 @@ The message is stored in the Outbox fold
> > > <button stock="gtk-no" _label="Don't delete"
> > > response="GTK_RESPONSE_NO"/>
> > > </error>
> > >
> > > +<error id="ask-delete-account-with-proxies" type="question"
> > > default="GTK_RESPONSE_NO" modal="true">
> > > + <_title>Delete account?</_title>
> > > + <_primary>Are you sure you want to delete this account and all its
> > > proxies?</_primary>
> > > + <_secondary xml:space="preserve">If you proceed, the account
> > > information and
> > > +all proxy information will be deleted permanently.</_secondary>
> > > + <button stock="gtk-delete" response="GTK_RESPONSE_YES"/>
> > > + <button stock="gtk-no" _label="Don't delete"
> > > response="GTK_RESPONSE_NO"/>
> > > + </error>
> > > +
> > > +<error id="ask-delete-proxy-accounts" type="question"
> > > default="GTK_RESPONSE_NO" modal="true">
> > > + <_title>Disable account and log out all proxies?</_title>
> > > + <_primary>Are you sure you want to disable?</_primary>
> > > + <_secondary xml:space="preserve">If you proceed, the account will
> > > be disabled
> > > +and all its proxies will be logged out.</_secondary>
> > > + <button stock="gtk-ok" response="GTK_RESPONSE_YES"/>
> > > + <button stock="gtk-cancel" response="GTK_RESPONSE_NO"/>
> > > + </error>
> > > +
> > > +
> >
> > dont add 2 spaces here.
> >
> > > <error id="no-save-signature" type="error">
> > > <_primary>Could not save signature file.</_primary>
> > > <_secondary xml:space="preserve">Because
> > > "{0}".</_secondary>
> > > Index: mail/em-account-prefs.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/mail/em-account-prefs.c,v
> > > retrieving revision 1.19
> > > diff -u -p -w -r1.19 em-account-prefs.c
> > > --- mail/em-account-prefs.c 16 May 2005 06:15:37 -0000 1.19
> > > +++ mail/em-account-prefs.c 5 Jul 2005 10:34:50 -0000
> > > @@ -192,7 +192,7 @@ account_edit_clicked (GtkButton *button,
> > > if (gtk_tree_selection_get_selected (selection,
> > > &model, &iter))
> > > gtk_tree_model_get (model, &iter, 3, &account,
> > > -1);
> > >
> > > - 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.
> >
> > > selection = gtk_tree_view_get_selection (prefs->table);
> > > if (gtk_tree_selection_get_selected (selection, &model,
> > > &iter))
> > > @@ -236,7 +237,11 @@ account_delete_clicked (GtkButton *butto
> > > if (account == NULL || prefs->editor != NULL)
> > > return;
> > >
> > > + if (g_list_length(account->proxy_children))
> >
> > you're going to have to add a helper to find out if it has proxies,
> > hwich searches all accounts for a proxy entry on that account.
> >
> > > + ans = e_error_run(PREFS_WINDOW(prefs),
> > > "mail:ask-delete-account-with-proxies",NULL);
> > > + else
> > > ans = e_error_run(PREFS_WINDOW(prefs),
> > > "mail:ask-delete-account", NULL);
> > > +
> >
> > something like this might be neater:
> >
> > e_error_run(prefs, has_proxies?"error1":"error2", NULL);
> >
> > > if (ans == GTK_RESPONSE_YES) {
> > > int len;
> > >
> > > @@ -244,6 +249,12 @@ account_delete_clicked (GtkButton *butto
> > > if (account->enabled && account->source &&
> > > account->source->url)
> > > mail_component_remove_store_by_uri
> > > (mail_component_peek (), account->source->url);
> > >
> > > + /* remove all the proxies you created*/
> > > + if (g_list_length (account->proxy_children)) {
> > > + has_spawned_proxies = TRUE;
> > > + mail_config_remove_account_proxies(account);
> > > + }
> > > +
> > > /* remove it from the config file */
> > > mail_config_remove_account (account);
> > > accounts = mail_config_get_accounts ();
> > > @@ -252,6 +263,8 @@ account_delete_clicked (GtkButton *butto
> > >
> > > gtk_list_store_remove ((GtkListStore *) model, &iter);
> > >
> > > + if (has_spawned_proxies)
> > > + mail_accounts_load (prefs);
> >
> > Hmm,I dont follow why this is needed - aren't proxy account entries
> > never actually shown in the account list anyway? So it doesn't have
> > anything to remove here ...?
> >
> > > len = e_list_length ((EList *) accounts);
> > > if (len > 0) {
> > > gtk_tree_selection_select_iter (selection,
> > > &iter);
> > > @@ -344,17 +357,30 @@ account_able_toggled (GtkCellRendererTog
> > >
> > > if (gtk_tree_model_get_iter (model, &iter, path)) {
> > > gtk_tree_model_get (model, &iter, 3, &account, -1);
> > > +
> > > + if (g_list_length (account->proxy_children)) {
> > > + char *ans;
> > > + ans = e_error_run(PREFS_WINDOW(prefs),
> > > "mail:ask-delete-proxy-accounts",NULL);
> >
> > errr, e_error_run returns an int, not a char *. Didn't you check
> > compiler warnings????
> >
> > > + if (ans == GTK_RESPONSE_YES) {
> > > account->enabled = !account->enabled;
> > > + e_account_list_change(mail_config_get_accounts(), account);
> > > gtk_list_store_set ((GtkListStore *) model, &iter, 0,
> > > account->enabled, -1);
> > > -
> > > if (gtk_tree_selection_iter_is_selected (selection,
> > > &iter))
> > > gtk_button_set_label (prefs->mail_able,
> > > account->enabled ? _("Disable") : _("Enable"));
> >
> > Umm, it appears as though you've added 2 levels of indent here, but
> > lines like the one above (and others) haven't been moved to match it?
> >
> > > -
> > > - /* 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?
> >
> > > + account_able_changed (account);
> > > + }
> > > + } else {
> > > + account->enabled = !account->enabled;
> > > e_account_list_change(mail_config_get_accounts(),
> > > account);
> > > + gtk_list_store_set ((GtkListStore *) model,
> > > &iter, 0, account->enabled, -1);
> > > + if (gtk_tree_selection_iter_is_selected
> > > (selection, &iter))
> > > + gtk_button_set_label
> > > (prefs->mail_able, account->enabled ? _("Disable") : _("Enable"));
> > > + /* let the rest of the application know it
> > > changed */
> > > account_able_changed(account);
> > > }
> > > -
> > > + }
> >
> > Again, extra level of indent, no update to the content.
> >
> > The code above looks a bit confusing - like you've copied a bunch of
> > code you didn't really need to. Can't you just add a special case
> > within that block to remove the proxies, rather than adding awhole new
> > copy of the logic block.
> >
> > > gtk_tree_path_free (path);
> > > }
> > >
> > > @@ -371,13 +397,17 @@ account_cursor_change (GtkTreeSelection
> > > EAccount *account = NULL;
> > > GtkTreeModel *model;
> > > GtkTreeIter iter;
> > > + char *url = NULL;
> > > int state;
> > >
> > > +/* This function needs to get the account variable rather than parse
> > > for stupid proxy = yes*/
> > > +
> >
> > indent the comment within functions please, add FIXME if it its a fixme.
> >
> > > state =
> > > gconf_client_key_is_writable(mail_config_get_gconf_client(),
> > > "/apps/evolution/mail/accounts", NULL);
> > > if (state) {
> > > state = gtk_tree_selection_get_selected (selection,
> > > &model, &iter);
> > > if (state) {
> > > gtk_tree_model_get (model, &iter, 3, &account,
> > > -1);
> > > + url = e_account_get_string (account,
> > > E_ACCOUNT_SOURCE_URL);
> > > if (account->source && account->enabled)
> > > gtk_button_set_label
> > > (prefs->mail_able, _("Disable"));
> > > else
> > > @@ -390,7 +420,12 @@ account_cursor_change (GtkTreeSelection
> > > gtk_widget_set_sensitive (GTK_WIDGET (prefs), FALSE);
> > > }
> > >
> > > + if( url != NULL ) {
> >
> > > + if( g_list_length(account->proxy_children))
> > > + gtk_widget_set_sensitive (GTK_WIDGET
> > > (prefs->mail_edit), 0);
> > > + else
> > > gtk_widget_set_sensitive (GTK_WIDGET (prefs->mail_edit),
> > > state);
> > > + }
> >
> > Umm, this seems to not set the edit button sensitive state if you dont
> > have a source url - which is a perfectly valid case (no receiving source
> > set). This logic could be done a lot simpler and not miss a case.
> >
> > e.g. set_sensitive(mail_edit, state && !account_is_proxy(account))
> >
> >
> > > gtk_widget_set_sensitive (GTK_WIDGET (prefs->mail_delete),
> > > state);
> > > gtk_widget_set_sensitive (GTK_WIDGET (prefs->mail_default),
> > > state);
> > > gtk_widget_set_sensitive (GTK_WIDGET (prefs->mail_able),
> > > state);
> > > @@ -420,6 +455,7 @@ mail_accounts_load (EMAccountPrefs *pref
> > >
> > > account = (EAccount *) e_iterator_get (node);
> > >
> > > + if (!account->proxy_account->parent_url) {
> > > url = account->source && account->source->url ?
> > > camel_url_new (account->source->url, NULL) : NULL;
> > >
> > > gtk_list_store_append (model, &iter);
> > > @@ -443,6 +479,7 @@ mail_accounts_load (EMAccountPrefs *pref
> > > camel_url_free (url);
> > >
> > > row++;
> > > + }
> >
> > Umm, again, more indenting without fixing up the indent of the code
> > within it. If you don't indent properly it makes the logic much harder
> > to understand when you look at it.
> >
> > > e_iterator_next (node);
> > > }
> > > Index: composer/ChangeLog
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/composer/ChangeLog,v
> > > retrieving revision 1.700
> > > diff -u -p -w -r1.700 ChangeLog
> > > --- composer/ChangeLog 3 Jun 2005 09:52:02 -0000 1.700
> > > +++ composer/ChangeLog 5 Jul 2005 11:37:48 -0000
> > > @@ -1,3 +1,8 @@
> > > +2005-07-05 Shreyas Srinivasan <sshreyas novell com>
> > > +
> > > + * e-msg-composer-hdrs.c: Take into account store
> > > + permissions (Read Only). These are proxy specific as of now.
> > > +
> > > 2005-06-03 Srinivasa Ragavan <sragavan novell com>
> > >
> > > * e-msg-composer.c (e_msg_composer_attach): Fixed to show the
> > > Index: composer/e-msg-composer-hdrs.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/composer/e-msg-composer-hdrs.c,v
> > > retrieving revision 1.137
> > > diff -u -p -w -r1.137 e-msg-composer-hdrs.c
> > > --- composer/e-msg-composer-hdrs.c 17 Jun 2005 15:20:28
> > > -0000 1.137
> > > +++ composer/e-msg-composer-hdrs.c 5 Jul 2005 13:06:55 -0000
> > > @@ -56,6 +56,9 @@
> > > #include "e-util/e-error.h"
> > >
> > > #include <camel/camel.h>
> > > +#include <camel/camel-store.h>
> > > +#include <camel/camel-session.h>
> > > +#include <e-util/e-proxy.h>
> >
> > Ugh no no ... more on this later.
> >
> > > #include "e-msg-composer-hdrs.h"
> > > #include "mail/mail-config.h"
> > > /*#include "mail/em-folder-selection-button.h"*/
> > > @@ -65,6 +68,7 @@
> > > /*#include "mail/mail-component.h"*/
> > > struct _MailComponent *mail_component_peek(void);
> > > extern struct _EMFolderTreeModel
> > > *mail_component_peek_tree_model(struct _MailComponent *);
> > > +CamelSession *session;
> >
> > This should be extern, or maybe include mail-session.h (mail-component.h
> > isn't included because of some bonobo related header issues).
> >
> > > #include "mail/em-folder-tree.h"
> > >
> > > @@ -228,6 +232,8 @@ account_added_cb (EAccountList *accounts
> > > omenu = e_msg_composer_hdrs_get_from_omenu (hdrs);
> > > menu = gtk_option_menu_get_menu (GTK_OPTION_MENU (omenu));
> > >
> > > + if (account_can_write_mails (account))
> > > + {
> >
> > c'mon, get the style right.
> >
> > And if you add a block, you gotta indent it!
> >
> > account_can_write_mails is a bit bulky, what about account_can_send(),
> > or even account_check_perms(can_send)
> >
> >
> > > label = g_strdup_printf ("%s <%s>", account->id->name,
> > > account->id->address);
> > > item = gtk_menu_item_new_with_label (label);
> > > gtk_widget_show (item);
> > > @@ -245,6 +251,7 @@ account_added_cb (EAccountList *accounts
> > > toplevel = gtk_widget_get_toplevel ((GtkWidget *) hdrs);
> > > gtk_widget_set_sensitive (toplevel, TRUE);
> > > }
> > > +}
> > >
> > > static void
> > > account_changed_cb (EAccountList *accounts, EAccount *account,
> > > EMsgComposerHdrs *hdrs)
> > > @@ -316,6 +323,26 @@ account_removed_cb (EAccountList *accoun
> > > }
> > > }
> > >
> > > +gboolean
> > > +account_can_write_mails (EAccount *account)
> > > +{
> > > + static CamelStore *store;
> > > + CamelException ex;
> > > +
> > > + if (!account->proxy_account->parent_url)
> > > + return TRUE;
> > > +
> > > + if (!(store = (CamelStore *) camel_session_get_service
> > > (session, account->source->url, CAMEL_PROVIDER_STORE, &ex))) {
> > > + camel_exception_clear (&ex);
> > > + return FALSE;
> > > + }
> > > +
> > > + if (store->permissions & PROXY_MAIL_WRITE)
> > > + return TRUE;
> >
> > You can't have a value on a camel object, which is determined by a flag
> > defined elsewhere. It must be a CAMEL_STORE_* flag, which must be
> > defined in camel-store.h. A little more on this later.
> >
> > Since you wont have the parent source url anymore, you'll have to lookup
> > the parent account - but its only an extra line of code.
> >
> > Also, try to use the accessors, e_account_get_string(), etc rather than
> > using account->blah.
> >
> > > + return FALSE;
> > > +}
> > > +
> > > static GtkWidget *
> > > create_from_optionmenu (EMsgComposerHdrs *hdrs)
> > > {
> > > @@ -341,7 +368,7 @@ create_from_optionmenu (EMsgComposerHdrs
> > > while (e_iterator_is_valid (iter)) {
> > > account = (EAccount *) e_iterator_get (iter);
> > >
> > > - if (account->id->address)
> > > + if (account->id->address && account_can_write_mails
> > > (account))
> > > g_ptr_array_add (addresses,
> > > account->id->address);
> > >
> > > e_iterator_next (iter);
> > > @@ -360,7 +387,7 @@ create_from_optionmenu (EMsgComposerHdrs
> > > continue;
> > > }
> > >
> > > - if (account->id->address && *account->id->address) {
> > > + if (account->id->address && *account->id->address &&
> > > account_can_write_mails (account)) {
> > > /* If the account has a unique email address,
> > > just
> > > * show that. Otherwise include the account
> > > name.
> > > */
> > > @@ -1104,6 +1131,8 @@ e_msg_composer_hdrs_set_from_account (EM
> > > item = l->data;
> > >
> > > account = g_object_get_data ((GObject *) item,
> > > "account");
> > > + if (account_can_write_mails (account))
> > > + {
> > > if (account_name) {
> >
> > Umm, apart from terrible indenting, what is this logic? Isn't it just a
> > && you want? Dont nest two if if's that dont nest other logic but
> > another if.
> >
> > > if (account->name && !strcmp (account_name,
> > > account->name)) {
> > > /* set the correct optionlist item */
> > > @@ -1121,7 +1150,7 @@ e_msg_composer_hdrs_set_from_account (EM
> > >
> > > return;
> > > }
> > > -
> > > + }
> > > l = l->next;
> > > i++;
> > > }
> > > Index: ChangeLog
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution-data-server/camel/ChangeLog,v
> > > retrieving revision 1.2451
> > > diff -u -p -w -r1.2451 ChangeLog
> > > --- ChangeLog 1 Jul 2005 03:30:35 -0000 1.2451
> > > +++ ChangeLog 5 Jul 2005 14:14:48 -0000
> > > @@ -1,3 +1,8 @@
> > > +2005-07-05 Shreyas Srinivasan <sshreyas novell com>
> > > +
> > > + * camel-store.h: Add a new #define to identify proxy stores
> > > + and a new data member permissions to hold store specific
> > > permissions.
> > > +
> > > 2005-06-24 Matt Brown <matt mattb net nz>
> > >
> > > * camel-gpg-context.c: Extend verify and decrypt functions to
> > > support
> > > Index: camel-store.h
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution-data-server/camel/camel-store.h,v
> > > retrieving revision 1.75
> > > diff -u -p -w -r1.75 camel-store.h
> > > --- camel-store.h 9 May 2005 20:13:13 -0000 1.75
> > > +++ camel-store.h 5 Jul 2005 14:15:13 -0000
> > > @@ -112,6 +112,7 @@ typedef struct _CamelRenameInfo {
> > > #define CAMEL_STORE_VTRASH (1 << 1)
> > > #define CAMEL_STORE_FILTER_INBOX (1 << 2)
> > > #define CAMEL_STORE_VJUNK (1 << 3)
> > > +#define CAMEL_STORE_PROXY (1 << 4)
> > >
> > > struct _CamelStore {
> > > CamelService parent_object;
> > > @@ -120,6 +121,7 @@ struct _CamelStore {
> > > CamelObjectBag *folders;
> > >
> > > int flags;
> > > + gint16 permissions;
> >
> > Again, just remember to define the permission flags here. The e-proxy.h
> > stuff shouldn't be included either, that seems to be very gw-backend
> > specific. They can be used to remap the flags in the gw-backend if they
> > dont match, but if they're the same they wont need to be.
> >
> > (e-proxy.h should probably remain as part of the e-gw-connection stuff
> > rather than a separate file, the camel interface should stand alone here
> > - other backends also have permission possibilities).
> >
> > > };
> > >
> > > /* open mode for folder */
> > > Index: camel-store.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution-data-server/camel/camel-store.c,v
> > > retrieving revision 1.163
> > > diff -u -p -w -r1.163 camel-store.c
> > > --- camel-store.c 9 Jun 2005 07:00:23 -0000 1.163
> > > +++ camel-store.c 5 Jul 2005 14:15:40 -0000
> > > @@ -138,6 +138,7 @@ camel_store_init (void *o)
> > >
> > > /* set vtrash and vjunk on by default */
> > > store->flags = CAMEL_STORE_VTRASH | CAMEL_STORE_VJUNK;
> > > + store->permissions = 0;
> >
> > Just FYI, you dont need to initialise things to zero (it doesn't hurt
> > though).
> >
> >
> > > store->priv = g_malloc0 (sizeof (*store->priv));
> > > store->priv->folder_lock = e_mutex_new (E_MUTEX_REC);
> > >
> > > _______________________________________________
> > > evolution-patches mailing list
> > > evolution-patches lists ximian com
> > > http://lists.ximian.com/mailman/listinfo/evolution-patches
> > >
> >
> > _______________________________________________
> > evolution-patches mailing list
> > evolution-patches lists ximian com
> > http://lists.ximian.com/mailman/listinfo/evolution-patches
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
