[PATCH v3 0/2] notmuch-show.el: text/calendar part insert fix

[PATCH v3 0/2] notmuch-show.el: text/calendar part insert fix

[Tomi Ollila]

This patch (series) obsoletes 
id:1352303676-21920-1-git-send-email-tomi.ollila@iki.fi

The first patch is complete replacement of the above, the second
adds "robustness to fail" whenever icalendar-import-buffer fails
without signaling an error.

Tomi Ollila (2):
  notmuch-show.el: import calendar data with public function after CR
    removal
  notmuch-show.el: handle the case where icalendar-import-buffer
    returns nil

 emacs/notmuch-show.el | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
1.8.0

[PATCH v3 1/2] notmuch-show.el: import calendar data with public function after CR removal

[Tomi Ollila]

notmuch-get-bodypart-content provides raw data to its caller so
that it can be stored verbatim whenever needed. icalendar functions
expect Emacs to do EOL conversion for the data given to these. Therefore
it the CRLF -> LF conversion is now done explicitly.

The calls to private functions icalendar--convert-ical-to-diary and
icalendar--read-element are replaced with call to public function
icalendar-import-buffer.
---
 emacs/notmuch-show.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d061367..e3977cc 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -747,12 +747,14 @@ message at DEPTH in the current thread."
   (notmuch-show-insert-part-header nth declared-type content-type (plist-get part :filename))
   (insert (with-temp-buffer
 	    (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))
+	    ;; notmuch-get-bodypart-content provides "raw", non-converted
+	    ;; data. Replace CRLF with LF before icalendar can use it.
 	    (goto-char (point-min))
+	    (while (re-search-forward "\r\n" nil t)
+	      (replace-match "\n" nil nil))
 	    (let ((file (make-temp-file "notmuch-ical"))
 		  result)
-	      (icalendar--convert-ical-to-diary
-	       (icalendar--read-element nil nil)
-	       file t)
+	      (icalendar-import-buffer file t)
 	      (set-buffer (get-file-buffer file))
 	      (setq result (buffer-substring (point-min) (point-max)))
 	      (set-buffer-modified-p nil)
-- 
1.8.0

Re: [PATCH v3 1/2] notmuch-show.el: import calendar data with public function after CR removal

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> notmuch-get-bodypart-content provides raw data to its caller so
> that it can be stored verbatim whenever needed. icalendar functions
> expect Emacs to do EOL conversion for the data given to these. Therefore
> it the CRLF -> LF conversion is now done explicitly.
>
> The calls to private functions icalendar--convert-ical-to-diary and
> icalendar--read-element are replaced with call to public function
> icalendar-import-buffer.

This code looks straightforward to me. Any objections to merging?

Re: [PATCH v3 1/2] notmuch-show.el: import calendar data with public function after CR removal

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> notmuch-get-bodypart-content provides raw data to its caller so
> that it can be stored verbatim whenever needed. icalendar functions
> expect Emacs to do EOL conversion for the data given to these. Therefore
> it the CRLF -> LF conversion is now done explicitly.

pushed both

d

[PATCH v3 2/2] notmuch-show.el: handle the case where icalendar-import-buffer returns nil

[Tomi Ollila]

icalendar-import-buffer can fail by an error signal (which have been
witnessed) but according to its docstring it can also return nil
when failing (it returns t when succeeding).

Now that the error is caught by the caller of notmuch-show-inset-part-*
functions in case icalendar-import-buffer returns nil an explicit
error is signaled and unwind-protect takes care of deleting the
temporary file (just in case, it is usually not written to the fs yet).
---
 emacs/notmuch-show.el | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e3977cc..9fb85c5 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -754,12 +754,15 @@ message at DEPTH in the current thread."
 	      (replace-match "\n" nil nil))
 	    (let ((file (make-temp-file "notmuch-ical"))
 		  result)
-	      (icalendar-import-buffer file t)
-	      (set-buffer (get-file-buffer file))
-	      (setq result (buffer-substring (point-min) (point-max)))
-	      (set-buffer-modified-p nil)
-	      (kill-buffer (current-buffer))
-	      (delete-file file)
+	      (unwind-protect
+		  (progn
+		    (unless (icalendar-import-buffer file t)
+		      (error "Icalendar import error. See *icalendar-errors* for more information"))
+		    (set-buffer (get-file-buffer file))
+		    (setq result (buffer-substring (point-min) (point-max)))
+		    (set-buffer-modified-p nil)
+		    (kill-buffer (current-buffer)))
+		(delete-file file))
 	      result)))
   t)
 
-- 
1.8.0

Re: [PATCH v3 2/2] notmuch-show.el: handle the case where icalendar-import-buffer returns nil

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> icalendar-import-buffer can fail by an error signal (which have been
> witnessed) but according to its docstring it can also return nil
> when failing (it returns t when succeeding).
>
> Now that the error is caught by the caller of notmuch-show-inset-part-*
> functions in case icalendar-import-buffer returns nil an explicit
> error is signaled and unwind-protect takes care of deleting the
> temporary file (just in case, it is usually not written to the fs yet).

This one looks OK to me too, although the API that makes it necessary
out to be taken out back...

d

Re: [PATCH v3 2/2] notmuch-show.el: handle the case where icalendar-import-buffer returns nil

[Tomi Ollila]

On Thu, Nov 22 2012, David Bremner <david@tethera.net> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> icalendar-import-buffer can fail by an error signal (which have been
>> witnessed) but according to its docstring it can also return nil
>> when failing (it returns t when succeeding).
>>
>> Now that the error is caught by the caller of notmuch-show-inset-part-*
>> functions in case icalendar-import-buffer returns nil an explicit
>> error is signaled and unwind-protect takes care of deleting the
>> temporary file (just in case, it is usually not written to the fs yet).
>
> This one looks OK to me too, although the API that makes it necessary
> out to be taken out back...

I didn't quite understand the meaning of the last words...

Anyway, the point in this patch is that if icalendar-import-buffer (or
the older version using icalendar private functions) fail by returning
nil, the error case is not (and has not been) shown to the user (user
gets either empty or partially filled bodypart.

Now that we would have that condition-case in the caller this code can 
signal error message to the user and provide info where more information
can be obtained (*ical-errors* buffer).

More complex code could be used to make 
notmuch-show-insert-part-text/calendar return nil in case 
icalendar-import-buffer fails -- then 
notmuch-show-insert-bodypart-internal would attempt to use
next handler in chain to insert content (if any)

>
> d

Tomi


> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch