Re: [evolution-patches] Bug #57001

[Chris Toshok <toshok ximian com>]

So, I never noticed this patch since it didn't include the component
name in the subject line.  I basically scan/search for "addressbook",
"contacts", "e-d-s", etc..  subjects without component names usually get
passed over (by me, at least).

comments below.

On Mon, 2004-05-31 at 20:23 +0530, Nirnimesh wrote:
> Bug #57001: Fixed.
> 
> Actually, one could get cleaner with this code (could incorporate for
> 'cut' and 'delete' prompts separately) and this made me do a lot deal
> more work for this than actually expected. :-)
> But the good thing is that it works!
> 
> Nirnimesh <nirnimesh students iiit net>
> IIIT-Hyderabad.


> Index: eab-editor.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/addressbook/gui/contact-editor/eab-
> editor.c,v
> retrieving revision 1.2
> diff -u -r1.2 eab-editor.c
> --- a/eab-editor.c      26 Mar 2004 04:09:18 -0000      1.2
> +++ b/eab-editor.c      31 May 2004 14:36:25 -0000
> @@ -301,10 +301,12 @@
>  }
>  
>  gboolean
> -eab_editor_confirm_delete (GtkWindow *parent)
> +eab_editor_confirm_delete (GtkWindow *parent, const char* name)
>  {
>         GtkWidget *dialog;
>         gint result;
> +       char* message=(char*) malloc(sizeof(char)*100+strlen(name));
> +       sprintf(message, "Are you sure you want\nto delete the contact
> (%s)?", name);

There are several things wrong with the above couple of lines:

1. You can combine both lines into 1 g_strdup_printf.
2. You didn't wrap the string with _(..), so it can't be localized.
3. You don't free message.
 
>         dialog = gtk_message_dialog_new (parent,
>                                          0,
> @@ -316,8 +318,7 @@
>                                           ? _("Are you sure you want
> \n"
>                                               "to delete these
> contacts?"))
>  #endif
> -                                         _("Are you sure you want\n"
> -                                           "to delete this
> contact?"));
> +                                         _(message));
>  
>         gtk_dialog_add_buttons (GTK_DIALOG (dialog),
>                                 GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
> @@ -331,6 +332,38 @@
>         return (result == GTK_RESPONSE_ACCEPT);
>  }

As it stands I don't think this method should take the name of a
contact.  It clearly breaks in the face of multiple selection (it asks
you if you're sure you want to delete the first contact in the
selection.)  Instead it should take a GList* of contacts.  The smarts
for generating the correct message to show the user can live in this
method then.

1. If the GList has 1 element, show _("Are you sure you want\nto delete
this contact (%s)?").
2. If the GList has more than 1 element, use:
   ngettext ("Are you sure you want to delete this %d contact", "Are you
sure you want to delete these %d contacts?", g_list_length (contacts))
3. Of course, the actual text of these messages need to be decided by
someone on Anna's team, most likely.
 
Also, make sure the method is robust in the face of a NULL list (a
g_return_val_if_fail is enough).

> +/* Prompt for 'cut the contact' instead of 'delete the contact' while
> actually deleting */
> +gboolean
> +eab_editor_confirm_delete_after_cutting (GtkWindow *parent, const
> char* name)
> +{
> +       GtkWidget *dialog;
> +       gint result;
> +       char* message=(char*) malloc(sizeof(char)*100+strlen(name));
> +       sprintf(message, "Are you sure you want\nto cut the contact (%
> s)?", name);
> +
> +       dialog = gtk_message_dialog_new (parent,
> +                                        0,
> +                                        GTK_MESSAGE_QUESTION,
> +                                        GTK_BUTTONS_NONE,
> +#if notyet
> +                                        /* XXX we really need to
> handle the plural case here.. */
> +                                        (plural
> +                                         ? _("Are you sure you want
> \n"
> +                                             "to delete these
> contacts?"))
> +#endif
> +                                         _(message));
> +
> +       gtk_dialog_add_buttons (GTK_DIALOG (dialog),
> +                               GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
> +                               GTK_STOCK_DELETE, GTK_RESPONSE_ACCEPT,
> +                               NULL);
> +
> +       result = gtk_dialog_run(GTK_DIALOG (dialog));
> +
> +       gtk_widget_destroy (dialog);
> +
> +       return (result == GTK_RESPONSE_ACCEPT);
> +}

We don't need this entirely new method.  We can either pass a boolean
flag to eab_editor_confirm_delete (whether it's deleting or cutting), or
we can just drop the confirmation in the case of cutting.  I'm not sure
it's necessary.

> Index: e-addressbook-view.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/addressbook/gui/widgets/e-addressbook-
> view.c,v
> retrieving revision 1.139
> diff -u -r1.139 e-addressbook-view.c
> --- e-addressbook-view.c        11 May 2004 00:42:19 -0000      1.139
> +++ e-addressbook-view.c        31 May 2004 14:39:14 -0000
> @@ -920,7 +920,64 @@
>  static void
>  delete (GtkWidget *widget, ContactAndBook *contact_and_book)
>  {
> -       if (eab_editor_confirm_delete(GTK_WINDOW
> (gtk_widget_get_toplevel(contact_and_book->view->widget)))) {
> +       /* XXX get the file as name .. not handled multiple
> selections :( */
> +       EContact *contact_pre = get_contact_list(contact_and_book)-
> >data;
> +       gpointer contact_file_as = e_contact_get_const (contact_pre,
> E_CONTACT_FILE_AS);
> +
> +       if (eab_editor_confirm_delete(GTK_WINDOW
> (gtk_widget_get_toplevel(contact_and_book->view->widget)),
> contact_file_as)) {
> +               EBook *book;
> +               GList *list = get_contact_list(contact_and_book);
> +               GList *iterator;
> +               gboolean bulk_remove = FALSE;
> +
> +               bulk_remove = e_book_check_static_capability
> (contact_and_book->view->model->book,
> +                                                             "bulk-
> remove");
> +
> +               g_object_get(contact_and_book->view->model,
> +                            "book", &book,
> +                            NULL);
> +
> +               if (bulk_remove) {
> +                       GList *ids = NULL;
> +
> +                       for (iterator = list; iterator; iterator =
> iterator->next) {
> +                               EContact *contact = iterator->data;
> +                               ids = g_list_prepend (ids,
> (char*)e_contact_get_const (contact, E_CONTACT_UID));
> +                       }
> +
> +                       /* Remove the cards all at once. */
> +                       /* XXX no callback specified... ugh */
> +                       e_book_async_remove_contacts (book,
> +                                                     ids,
> +                                                     NULL,
> +                                                     NULL);
> +                       
> +                       g_list_free (ids);
> +               }
> +               else {
> +                       for (iterator = list; iterator; iterator =
> iterator->next) {
> +                               EContact *contact = iterator->data;
> +                               /* Remove the card. */
> +                               /* XXX no callback specified... ugh */
> +                               e_book_async_remove_contact (book,
> +                                                            contact,
> +                                                            NULL,
> +                                                            NULL);
> +                       }
> +               }
> +               e_free_object_list(list);
> +               g_object_unref(book);
> +       }
> +}
> 

Hm, not sure why this entire method shows up as being changed - all you
did was add the file_as stuff at the start of it, right?  At any rate,
this doesn't need to be done here if we pass the list into
eab_editor_confirm_delete.  Just pass "get_contact_list
(contact_and_book)" to eab_editor_confirm_delete.

> +static void
> +delete_after_cutting (GtkWidget *widget, ContactAndBook
> *contact_and_book)
> +{
> +       /* XXX get the file as name .. not handled multiple
> selections :( */
> +       EContact *contact_pre = get_contact_list(contact_and_book)-
> >data;
> +       gpointer contact_file_as = e_contact_get_const (contact_pre,
> E_CONTACT_FILE_AS);
> +
> +       if (eab_editor_confirm_delete_after_cutting (GTK_WINDOW
> (gtk_widget_get_toplevel(contact_and_book->view->widget)),
> contact_file_as)) {
>                 EBook *book;
>                 GList *list = get_contact_list(contact_and_book);
>                 GList *iterator;
> @@ -1976,6 +2033,16 @@
>         delete (GTK_WIDGET (view), &contact_and_book);
>  }

This method is unnessary.
 
> +void
> +eab_view_delete_selection_after_cutting(EABView *view)
> +{
> +       ContactAndBook contact_and_book;
> +
> +       memset (&contact_and_book, 0, sizeof (contact_and_book));
> +       contact_and_book.view = view;
> +
> +       delete_after_cutting (GTK_WIDGET (view), &contact_and_book);
> +}

Neither is this one.

>  static void
>  invisible_destroyed (gpointer data, GObject *where_object_was)
>  {
> @@ -2107,7 +2174,7 @@
>  eab_view_cut (EABView *view)
>  {
>         eab_view_copy (view);
> -       eab_view_delete_selection (view);
> +       eab_view_delete_selection_after_cutting (view);
>  }

If we add a boolean flag to eab_view_delete_selection (gboolean cutting,
say.. TRUE if the user is cutting, false if they're deleting), we don't
need the extra method.

Chris