Re: [evolution-patches] Patch for the receipt bounty (#127534)

[Not Zed <notzed ximian com>]

Comments inline.  Generally on the right track, although i think you
can make the patch much smaller and simpler.  Its touching a lot of
places (like camel) which aren't necessary for the functionality.

> Index: camel/camel-folder-summary.h
> ===================================================================
> RCS file: /cvs/gnome/evolution/camel/camel-folder-summary.h,v
> retrieving revision 1.78
> diff -u -u -r1.78 camel-folder-summary.h
> --- camel/camel-folder-summary.h        21 May 2004 09:08:09 -0000      1.78
> +++ camel/camel-folder-summary.h        9 Aug 2004 12:48:53 -0000
> @@ -70,6 +70,8 @@
>         CAMEL_MESSAGE_JUNK = 1<<7,
>         CAMEL_MESSAGE_SECURE = 1<<8,
>  
> +       CAMEL_MESSAGE_RECEIPT_HANDLED = 1 << 9,
> +       
>         /* following flags are for the folder, and are not really permanent flags */
>         CAMEL_MESSAGE_FOLDER_FLAGGED = 1<<16, /* for use by the folder implementation */
>  
> Index: camel/camel-folder.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/camel/camel-folder.c,v
> retrieving revision 1.203
> diff -u -u -r1.203 camel-folder.c
> --- camel/camel-folder.c        21 May 2004 09:08:09 -0000      1.203
> +++ camel/camel-folder.c        9 Aug 2004 12:48:58 -0000
> @@ -166,6 +166,7 @@
>  
>         /* events */
>         camel_object_class_add_event(camel_object_class, "folder_changed", folder_changed);
> +       camel_object_class_add_event(camel_object_class, "handle_receipt", NULL);
>         camel_object_class_add_event(camel_object_class, "deleted", NULL);
>         camel_object_class_add_event(camel_object_class, "renamed", NULL);
>  }
> @@ -766,6 +767,17 @@
>         if (old != info->flags) {
>                 info->flags |= CAMEL_MESSAGE_FOLDER_FLAGGED;
>                 camel_folder_summary_touch(folder->summary);
> +       }
> +
> +       /* If we're setting the SEEN flag on a message that asks for a
> +        * receipt, fire an appropriate event */
> +       if (((set & flags) | CAMEL_MESSAGE_SEEN) &&
> +           !(old & CAMEL_MESSAGE_SEEN) &&
> +           !(info->flags & CAMEL_MESSAGE_RECEIPT_HANDLED))
> +       {
> +               CamelMimeMessage *message = camel_folder_get_message (folder, uid, NULL);
> +               if (camel_medium_get_header (CAMEL_MEDIUM (message), "Disposition-Notification-To"))
> +                       camel_object_trigger_event(folder, "handle_receipt", uid);
>         }
>  

I think these camel changes are unecessary.  Having this request go
through an event is very unecessary, this is only a client-side
process, and should require zero changes to camel.  Events are for a
different purpose.  The formatting is all whack too, use k&r braces
and gnu operators.

You can just set a user-flag to say you've handled the receipt if you
must.  Infact, you dont ever need to know that you have, presumably
you either send it or you don't when you first read the message and
thats that.

You know when the user has read the message from the client only.  You
can also set the seen flag from filters, and i very much doubt you
care to send a read-receipt at such a time.

> +       gboolean receipt;
>         
>         if (composer->persist_stream_interface == CORBA_OBJECT_NIL)
>                 return NULL;
> @@ -437,6 +438,16 @@
>                                          composer->extra_hdr_names->pdata[i],
>                                          composer->extra_hdr_values->pdata[i]);
>         }
> +
> +       /* Message Disposition Notification */
> +       receipt = atoi (bonobo_ui_component_get_prop (composer->uic, "/commands/RequestReceipt", "state", NULL));

This should work like all of the other menu state/sending option code.
None of it gets the prop like this.

> @@ -655,6 +658,7 @@
>         { /* E_ACCOUNT_SOURCE_AUTO_CHECK */ 1<<EAP_LOCK_AUTOCHECK },
>         { /* E_ACCOUNT_SOURCE_AUTO_CHECK_TIME */ 1<<EAP_LOCK_AUTOCHECK },
>         { /* E_ACCOUNT_SOURCE_SAVE_PASSWD */ 1<<EAP_LOCK_SAVE_PASSWD },
> +       { /* E_ACCOUNT_SROUCE_RECEIPT_POLICY */ },
>  
>         { /* E_ACCOUNT_TRANSPORT_URL */ 1<<EAP_LOCK_TRANSPORT },
>         { /* E_ACCOUNT_TRANSPORT_SAVE_PASSWD */ 1<<EAP_LOCK_SAVE_PASSWD },

It probably should be separately lockable, but it isn't a biggy.

I also wonder if it should just be a global option rather than
per-account, UI? (who never reads this list anyway).

> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
> retrieving revision 1.3428
> diff -u -u -r1.3428 ChangeLog
> --- mail/ChangeLog      4 Aug 2004 15:20:57 -0000       1.3428
> +++ mail/ChangeLog      9 Aug 2004 12:49:50 -0000
> @@ -1,3 +1,12 @@
> +2004-08-09  ERDI Gergo  <cactus cactus rulez org>
> +
> +       * em-folder-view.c (emfv_send_receipt): New function to send an
> +       RFC 2298-compliant message delivery notification
> +
> +2004-08-07  ERDI Gergo  <cactus cactus rulez org>
> +
> +       * em-utils.c (em_utils_guess_account): Moved here from em-composer-utils.c

Uh, WHY?  Please don't randomly move functions around.  There's
absolutely no reason to do this.  If it needed to be public it should
just be exported from emcu, although i dont think it needs to be.

All of the actual functionality of asking and sending the receipt
should be in em-composer-utils.c anyway since it is actually sending a
message, and thats where all the sending message code resides.

EMFolderView should just call those (re-usable) functions, rather than
implementing it itself, for this non-trivial bit of code.  As it does
with many other functions, such as sending mail or composing replies.

>  
>  struct _EMFolderViewPrivate {
> @@ -525,6 +532,9 @@
>         if (folder) {
>                 emfv->priv->folder_changed_id = camel_object_hook_event(folder, "folder_changed",
>                                                                         (CamelObjectEventHookFunc)emfv_folder_changed, emfv);
> +               /* Listen for delivery notification requests */
> +               camel_object_hook_event(folder, "handle_receipt", emfv_handle_receipt, emfv);
> +               

Although this must be removed anyway since it shouldn't be going
through an event, for future reference you need to keep track of the
hook id and remove it, the folder can have a life much longer than the
folder-view object, and infact since you can change folders you could
get events from the wrong folder, etc.

> +static gboolean
> +emfv_ask_receipt (EMFolderView *emfv, CamelMimeMessage *message)
> +{
> +       /* Check the account's receipt policy settings, and pop up a
> +        * dialog if the policy is ASK */
> +       
> +       EAccount *account = em_utils_guess_account (message, emfv->folder);
> +
> +       if (account->source->receipt_policy == E_ACCOUNT_RECEIPT_ASK) {
> +               GtkWidget *dialog;
> +               GtkWidget *toplevel = gtk_widget_get_toplevel (GTK_WIDGET (emfv));
> +               GtkWindow *dialog_parent = (toplevel && GTK_IS_WINDOW (toplevel)) ? GTK_WINDOW (toplevel) : NULL;
> +               const char *receipt_address = camel_medium_get_header (CAMEL_MEDIUM (message), "Disposition-Notification-To");
> +               const char *subject = camel_mime_message_get_subject (message);
> +               int response;
> +               
> +               dialog = gtk_message_dialog_new (dialog_parent,
> +                                                GTK_DIALOG_MODAL,
> +                                                GTK_MESSAGE_QUESTION,
> +                                                GTK_BUTTONS_YES_NO,
> +                                                _("Send message receipt to %s about message \"%s\"?"),
> +                                                receipt_address, subject);
> +               response = gtk_dialog_run (GTK_DIALOG (dialog));

All questions and errors must go through e_error, you should never use
a gtk_message_dialog or similar.

If you use e_error then this function doesn't need to exist, it can
just be part of send_receipt.

> +               gtk_widget_destroy (dialog);
> +       
> +               return (response == GTK_RESPONSE_YES);
> +       }
> +       
> +       return (account->source->receipt_policy == E_ACCOUNT_RECEIPT_ALWAYS);
> +}
> +
> +static void
> +emfv_send_receipt (EMFolderView *emfv, CamelMimeMessage *message)
> +{
> +       EAccount *account = em_utils_guess_account (message, emfv->folder);
> +
> +       CamelMimeMessage *receipt = camel_mime_message_new ();
> +       CamelMultipart *body = camel_multipart_new ();
> +       CamelMimePart *part;
> +       CamelDataWrapper *receipt_text, *receipt_data;
> +       char *receipt_text_buf, *receipt_data_buf;
> +       CamelContentType *type;
> +       CamelInternetAddress *addr;
> +       CamelStream *stream;    
> +       CamelFolder *folder;
> +       CamelMessageInfo *info;
> +       
> +       const char *message_id = camel_medium_get_header (CAMEL_MEDIUM (message), "Message-ID");
> +       const char *message_date = camel_medium_get_header (CAMEL_MEDIUM (message), "Date");
> +       const char *message_subject = camel_mime_message_get_subject (message);
> +       const char *receipt_address = camel_medium_get_header (CAMEL_MEDIUM (message), "Disposition-Notification-To");
> +       char hostname[MAXHOSTNAMELEN + 1];
> +       char *self_address, *receipt_subject;

This function should be in em-composer-utils since it is fairly large,
and sending a message.

It should be invoked from em-folder-view when em-folder-view times out
and sets the seen flag on the message.  Although it isn't necessary,
it could, for example, set a user-flag to say it's sent one if it
really needs to keep track of it.

FWIW its neater if you don't separate lines of the declaration section.


> +       
> +       g_return_if_fail (receipt_address);
> +       g_return_if_fail (account);
> +
> +       
> +       /* Collect information for the receipt */
> +       gethostname (hostname, MAXHOSTNAMELEN);
> +       self_address = account->id->address;

> +       
> +       /* Create toplevel container */
> +       camel_data_wrapper_set_mime_type (CAMEL_DATA_WRAPPER (body),
> +                                         "multipart/report;"
> +                                         "report-type=\"disposition-notification\"");
> +       camel_multipart_set_boundary (body, NULL);
> +       
> +       
> +       /* Create textual receipt */
> +       receipt_text = camel_data_wrapper_new ();
> +       receipt_text_buf = g_strdup_printf ("Your message to %s about \"%s\" "
> +                                           "(dated %s) has been read by the user.",
> +                                           self_address, message_subject, message_date);

This should probably be translated.

> +       stream = camel_stream_mem_new_with_buffer (receipt_text_buf, strlen (receipt_text_buf));
> +       g_free (receipt_text_buf);
> +       type = camel_content_type_new ("text", "plain");
> +       camel_data_wrapper_set_mime_type_field (receipt_text, type);
> +       camel_content_type_unref (type);
> +       camel_data_wrapper_construct_from_stream (receipt_text, stream);
> +       camel_object_unref (stream);
> +       
> +       part = camel_mime_part_new ();
> +       camel_medium_set_content_object (CAMEL_MEDIUM (part), receipt_text);
> +       camel_object_unref (receipt_text);
> +       camel_multipart_add_part (body, part);
> +       camel_object_unref (part);
> +       
> +       
> +       /* Create the machine-readable receipt */
> +       receipt_data = camel_data_wrapper_new ();
> +       receipt_data_buf = g_strdup_printf ("Reporting-UA: %s; %s\n"
> +                                           "Final-Recipient: rfc822; %s\n"
> +                                           "Original-Message-ID:%s\n"
> +                                           "Disposition: manual-action/MDN-sent-manually; displayed",
> +                                           hostname, "Evolution " VERSION SUB_VERSION " " VERSION_COMMENT,
> +                                           self_address, message_id);
> +       stream = camel_stream_mem_new_with_buffer (receipt_data_buf, strlen (receipt_data_buf));
> +       g_free (receipt_data_buf);
> +       type = camel_content_type_new ("message", "disposition-notification");
> +       camel_data_wrapper_set_mime_type_field (receipt_data, type);
> +       camel_content_type_unref (type);
> +       camel_data_wrapper_construct_from_stream (receipt_data, stream);
> +       camel_object_unref (stream);

I'm not sure how this is transported, but if this is supposed to be a
section whose headers are significant, then you should set the headers
directly and not construct it from a stream.  If the headers are in
the body then it's different of course.

> +       part = camel_mime_part_new ();
> +       camel_medium_set_content_object (CAMEL_MEDIUM (part), receipt_data);
> +       camel_object_unref (receipt_data);
> +       camel_multipart_add_part (body, part);
> +       camel_object_unref (part);
> +       
> +       
> +       /* Finish creating the message */
> +       camel_medium_set_content_object (CAMEL_MEDIUM (receipt), CAMEL_DATA_WRAPPER (body));
> +       camel_object_unref (body);
> +       
> +       receipt_subject = g_strdup_printf ("Delivery Notification for: \"%s\"", message_subject);

I presume this should be translated?  I understand the receiver might
not speak the same language, but its probably better than leaving it
english.

> +       camel_mime_message_set_subject (receipt, receipt_subject);
> +       g_free (receipt_subject);
> +       
> +       addr = camel_internet_address_new ();
> +       camel_address_decode (CAMEL_ADDRESS (addr), self_address);
> +       camel_mime_message_set_recipients (receipt, CAMEL_RECIPIENT_TYPE_TO, addr);
> +       camel_object_unref (addr);
> +       
> +       addr = camel_internet_address_new ();
> +       camel_address_decode (CAMEL_ADDRESS (addr), receipt_address);
> +       camel_mime_message_set_from (receipt, addr);
> +       camel_object_unref (addr);
> +
> +       camel_medium_set_header (CAMEL_MEDIUM (receipt), "Return-Path", "<>");
> +
> +       /* Send the receipt */
> +       folder = mail_component_get_folder(NULL, MAIL_COMPONENT_FOLDER_OUTBOX);
> +       info = camel_message_info_new ();
> +       info->flags = CAMEL_MESSAGE_SEEN;
> +       mail_append_mail (folder, receipt, info, 0, 0);

You should set the callback for the mail appended thing, and if you're
online, fire off a mail_send() as well.


On Mon, 2004-08-09 at 14:56 +0200, ERDI Gergo wrote:
> Hi,
>
> Attached is a preliminary version of message receipt handling. The UI to 
> set the receipt policy for accounts is not yet written, I'm just sending 
> this to hammer out problems in the backend code before worrying about the 
> UI. I'd also like to get some input on the text contents of the receipt 
> because right now it's pretty lame.
>
> Bye,
>  	Gergo
>
>
> -- 
>     .--= ULLA! =---------------------.   `We are not here to give users what
>     \     http://cactus.rulez.org     \   they want'  -- RMS, at GUADEC 2001
>      `---= cactus cactus rulez org =---'
> Sajnálom, de nem tudok segíteni. Engem csak a szépségem miatt vettek fel.

-- 







Michael Zucchi <notzed ximian com>

"born to die, live to work, it's all downhill from here"

Novell's Evolution and Free Software Developer