Re: [evolution-patches] Exchange: Patch for bug #311324

[Sarfraaz Ahmed <asarfraaz novell com>]

Looks better now. But, there are a few more tweaks needed. 


-- Sarfraaz

On Mon, 2005-08-01 at 16:38 +0530, shakti wrote:
> 
> 
> 

> 
> 
> Index: exchange-config-listener.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/plugins/exchange-operations/exchange-
> config-listener.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 exchange-config-listener.c
> --- exchange-config-listener.c  1 Aug 2005 06:46:30 -0000       1.9
> +++ exchange-config-listener.c  1 Aug 2005 11:08:29 -0000
> @@ -839,6 +839,36 @@ idle_construct (gpointer data)
>         return FALSE;
>  }
>  
> +ExchangeConfigListenerStatus
> +exchange_config_listener_get_offline_status (ExchangeConfigListener
> *excl,
> +                                            gint *mode)
> +{
> +       ExchangeConfigListenerPrivate *priv;
> +       GConfValue *value;
> +       ExchangeConfigListenerStatus status;
> +       gboolean offline = FALSE;
> +
> +       if (!excl)
> +               return CONFIG_LISTENER_ACCOUNT_NOT_CREATED; 
> 
> Change this enum name. We cannot always conclude that there is no
> account created if config listener is null. You could probably call
> it, something like, LISTENER NOT FOUND or some such.
> 
> Also, you could use g_val_return_if_fail, so that we know its a
> precondition check.
> 
> +       
> +       priv = excl->priv;
> +       value = gconf_client_get (priv->gconf,
> +                                       "/apps/evolution/shell/start_offline", NULL);
> +       if (value)
> +               offline = gconf_value_get_bool (value);
> +
> +       if (!offline) {
> +               *mode = IS_OFFLINE;
> +               status = CONFIG_LISTENER_STATUS_OK;
> +       }
> +       else {
> +               *mode = IS_ONLINE;
> +               status = CONFIG_LISTENER_STATUS_OK;
> +       }          
> 
> You could reduce some code here. status is never changing in this
> function. So initialise it to OK earlier itself. And for the if-else
> loop, you could also reduce it, by initialising *mode to some state
> just before you check the gconf value, and change it only if your
> assumption is wrong.
> 
> Also, we already have Online/offline enums defined in exchange-
> constants.h. You could make use of them, and may be even eliminate the
> use of the status flag you are returning.     
> 
> +
> +       return status;
> +
> +}      
>  /**
>   * exchange_config_listener_new:
>   *
> Index: exchange-config-listener.h
> ===================================================================
> RCS file: /cvs/gnome/evolution/plugins/exchange-operations/exchange-
> config-listener.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 exchange-config-listener.h
> --- exchange-config-listener.h  13 Jun 2005 12:39:20 -0000      1.1
> +++ exchange-config-listener.h  1 Aug 2005 11:08:29 -0000
> @@ -16,6 +16,16 @@ extern "C" {
>  #pragma }
>  #endif /* __cplusplus */
>  
> +typedef enum {
> +       CONFIG_LISTENER_STATUS_OK,
> +       CONFIG_LISTENER_ACCOUNT_NOT_CREATED
> +} ExchangeConfigListenerStatus;
> +
> +enum {
> +       IS_OFFLINE,
> +       IS_ONLINE
> +};
> 
> This enum is not needed.
> 
> +
> 
> +ExchangeConfigListenerStatus
> exchange_config_listener_get_offline_status (ExchangeConfigListener
> *excl, gint *mode);
> 
> The mode is sometimes used as a gint* and sometimes as gboolean*. This
> must have given some compiler warnings :)
>  
> 
>        if (status == CONFIG_LISTENER_ACCOUNT_NOT_CREATED) {
> +               g_warning ("Exchange account is not created");
> +               return;
> +       }       
> 
> how about having a if (status != STATUS_OK) do something; else do
> soemthingelse ?. Looks much cleaner this way.
> 
> +
> +       if (status == CONFIG_LISTENER_STATUS_OK && mode ==
> OFFLINE_MODE) {
> +               g_warning ("Unsubscribe to Other User's Folder is not
> allowed in Offline mode\n");
> 
> 
>  }
>  
> +ExchangeConfigListenerStatus
> +exchange_is_offline (gint *mode)
> 
> I guess you want a gboolean* here for mode ?
> 
>  
> 
>