Re: [evolution-patches] Patch to add open menu to right click popup for attachment(s) in meetings.

[Harish Krishnaswamy <kharish novell com>]

Hi,
  
 A few suggestions...

      *  The patch seems to address multiple issues - open as well as
        modify  attachments, (de)sensitizing  widgets for DnD events.
        Kindly split them into different cohesive patches. This makes it
        easy to manipulate them while merging them with other branches,
        reverting them etc.
      * mention the bug ids that the patch proposes to solve.
      * As this patch modifies the ui and we are beyond the UI freeze,
        this must be approved by the release team and GDP should be
        notified as well. 
  
> 
> +       E_CAL_POPUP_MODIFY_ATTACHMENTS = 1<<2,/* check to open/remove
> attachments */
Opening an attachment does not modify it. Hence the name is not entirely
consistent with its use.

>  }; 
> +ECalPopupTargetAttachments * e_cal_popup_target_new_attachments
> (ECalPopup *ecp, CompEditor *editor, GSList *attachments);
pl. remove the unwanted space. 


> +       ECal *client;
> +       gboolean read_only;
> +       CompEditorFlags flags;
> +
> +       client = comp_editor_get_e_cal (editor);
> +       flags = comp_editor_get_flags (editor);
> +       e_cal_is_read_only (client, &read_only, NULL);

Use the return value from e_cal_is_read_only and handle errors here.

>         t->attachments = attachments;
> +       
> +       if (!read_only && ((flags & COMP_EDITOR_USER_ORG) || 
> +                               (flags & COMP_EDITOR_NEW_ITEM) ||
> +                               !(flags &
> COMP_EDITOR_MEETING)))        
Checking if it is not a meeting before checking if the user is an
organizer makes this more readable.

 
> 
> -       gboolean read_only, custom, alarm, sens = TRUE;
> +       gboolean read_only, custom, alarm, user_org = TRUE;


> +       gtk_widget_set_sensitive (priv->summary_label, !read_only &&
> user_org);
> +       gtk_entry_set_editable (GTK_ENTRY (priv->summary), !read_only
> && user_org);
> +       gtk_widget_set_sensitive (priv->location_label, !read_only &&
> user_org);
> +       gtk_entry_set_editable (GTK_ENTRY (priv->location), !read_only
> && user_org);
> +       gtk_widget_set_sensitive (priv->start_time, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->start_timezone, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->end_time, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->end_timezone, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->all_day_event, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->description, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->classification, !read_only &&
> user_org);
> +       gtk_widget_set_sensitive (priv->show_time_as_busy, !read_only
> && user_org);
The !read_only && user_org is computed 12 times. 
How about calling 'sensitize' or something as before, 
computing sensitize = !read_only && <get static cap here> once
and using it.

> 
> +

comp_uid is being leaked here. (evil of copy-paste - the original code
(authored by yours truly (sigh)) leaks this too :(
can you make this a function call instead of duplicating code ?
  
Thanks,
Harish