Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
Mark Walters markwalters1...@gmail.com writes: Some messages are sent as multipart/alternative but the alternatives contain different information. This allows the user to cycle which part to view. By default this is bound to 'W'. --- This version at least uses the notmuch escaping for message-id which makes me a bit happier: it probably doesn't have any nasty security flaws. I do still feel that the lisp is a bit ugly though. For what it's worth, I don't feel that this code is horrible. It seems like there remain design decisions to be made about how notmuch show ought to handle multipart/alternatives, but I can at least comment on this code. First, the use of a plist looks fine to me because most of the time it's going to have length 0. At most it will have one entry per message -- a few hundred. So I'm not worried about efficiency concerns. (defcustom notmuch-show-stash-mlarchive-link-alist '((Gmane . http://mid.gmane.org/;) (MARC . http://marc.info/?i=;) @@ -536,9 +540,19 @@ message at DEPTH in the current thread. (defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type) (notmuch-show-insert-part-header nth declared-type content-type nil) - (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part - (inner-parts (plist-get part :content)) - (start (point))) + (let* ((chosen-nth (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part + (notmuch-id-to-query (plist-get msg :id))) 0)) + (chosen-type (nth chosen-nth + (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part + (inner-parts (plist-get part :content)) + (start (point))) Changing let to let* makes me the slightest bit uneasy, although I'm not sure I could explain why. It would be nice if you could wrap the manipulation of notmuch-show-message-multipart/alternative-display-part in functions, ideally with names that are shorter than the variable they manipulate. Specifically, I think the definition of chosen-nth (which is almost repeated below) could be its own function, something like (notmuch-show-message-current-multipart msg), which could take a msg-id or a plist and do the plist-get and id-to-query that you do here. +;; If we have run out of possible content-types restart from the beginning +(unless chosen-type + (setq chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part + (setq notmuch-show-message-multipart/alternative-display-part + (lax-plist-put notmuch-show-message-multipart/alternative-display-part +(notmuch-id-to-query (plist-get msg :id)) 0))) + ;; This inserts all parts of the chosen type rather than just one, ;; but it's not clear that this is the wrong thing to do - which ;; should be chosen if there are more than one that match? @@ -942,6 +956,16 @@ message at DEPTH in the current thread. Not processing cryptographic MIME parts.)) (notmuch-show-refresh-view)) +(defun notmuch-show-cycle-message-multipart () + Cycle which part to display of a multipart messageToggle the display of non-matching messages. This docstring is broken. + (interactive) + (let* ((msg-id (notmuch-show-get-message-id)) + (next-part (1+ (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part msg-id) 0 +(setq notmuch-show-message-multipart/alternative-display-part + (lax-plist-put notmuch-show-message-multipart/alternative-display-part msg-id next-part)) +(message Cycling multipart/alternative for current message) +(notmuch-show-refresh-view))) Maybe move the reset-and-go-back-to-zero behavior to this function instead of in the display function. This opens you up to weird situations if one of the multipart/alternatives should disappear from a message or if some other function should change the alternative on display for a given message, but both of these seem unlikely to me.. Ethan ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Fri, Aug 10, 2012 at 1:10 AM, Mark Walters markwalters1...@gmail.com wrote: Some messages are sent as multipart/alternative but the alternatives contain different information. This allows the user to cycle which part to view. By default this is bound to 'W'. I've started using this, and quite like it. Only thing missing I can see would be a customizable way to say which types should be preferred. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Fri, 10 Aug 2012, Mark Walters markwalters1...@gmail.com wrote: Some messages are sent as multipart/alternative but the alternatives contain different information. This allows the user to cycle which part to view. By default this is bound to 'W'. --- On Thu, 09 Aug 2012, Jameson Graef Rollins jroll...@finestructure.net wrote: On Mon, Jun 18 2012, Mark Walters markwalters1...@gmail.com wrote: I agree with this cycling approach but I think it needs to be per message rather than per buffer. I attach a rather hacky attempt at this below: on minimal testing it seems to work. But the lisp is really a bit gross. In particular I have no idea if I should be escaping the message ids (so this could break in unfortunate/insecure ways) Thanks to broken Apple mail clients, I'm getting more and more messages that have attachments hidden in multipart/alternatives to text/plain parts. So I would really like to revive this patch. I just tested it and it still applies to current master, and actually seems to work great. 'W' cycles through which part is displayed in the current message. Pretty much exactly what I want. Mark seems to think this patch is less than ideal. One issue is that it's trying to store a setting for a single displayed message in a variable of full buffer scope. So he's storing a list of message ids there: +(defvar notmuch-show-message-multipart/alternative nil) +(make-variable-buffer-local 'notmuch-show-message-multipart/alternative) +(put 'notmuch-show-message-multipart/alternative 'permanent-local t) ... + (lax-plist-put notmuch-show-message-multipart/alternative (plist-get msg :id) 0))) I can see that might get a little hairy. Can any elisp experts out there think of a better way to do this? (actually, this is making me again want a show mode that only displays one message at a time (which I guess means I need to try pick again)). This version at least uses the notmuch escaping for message-id which makes me a bit happier: it probably doesn't have any nasty security flaws. I do still feel that the lisp is a bit ugly though. Incidentally, Austin suggested I might be able to use text-properties rather than this big list. Unfortunately, I use notmuch-show-refresh-view to do the redisplay and that deletes all text-properties. Note this is not very well tested as I have very few multipart/alternative messages. How would this work together with something like [1] (rationale in [2])? [1] id:ab777cf0fa83778d3399ac52094df9230738819d.1328798471.git.j...@nikula.org [2] id:cover.1328719309.git.j...@nikula.org If you introduce a mechanism to store the state, could it be extended to store the state of each individual part? That, in turn, could be used to add support for expanding/collapsing each alternative part through the buttons (e.g. [ text/html (not shown) ]). Each button could toggle the state of the part, and refresh buffer. I guess basically the above are related. If you stored a list of parts to display per each message id, the initial list could be created based on customized regexps, the buttons could be used for toggling each individual part (adding/removing the type from the list), and you could have a function that would cycle the list to your heart's content. BR, Jani. Best wishes Mark emacs/notmuch-show.el | 31 --- 1 files changed, 28 insertions(+), 3 deletions(-) diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el index dcfc190..dee6b85 100644 --- a/emacs/notmuch-show.el +++ b/emacs/notmuch-show.el @@ -154,6 +154,10 @@ indentation. (make-variable-buffer-local 'notmuch-show-indent-content) (put 'notmuch-show-indent-content 'permanent-local t) +(defvar notmuch-show-message-multipart/alternative-display-part nil) +(make-variable-buffer-local 'notmuch-show-message-multipart/alternative-display-part) +(put 'notmuch-show-message-multipart/alternative-display-part 'permanent-local t) + (defcustom notmuch-show-stash-mlarchive-link-alist '((Gmane . http://mid.gmane.org/;) (MARC . http://marc.info/?i=;) @@ -536,9 +540,19 @@ message at DEPTH in the current thread. (defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type) (notmuch-show-insert-part-header nth declared-type content-type nil) - (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part - (inner-parts (plist-get part :content)) - (start (point))) + (let* ((chosen-nth (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part + (notmuch-id-to-query (plist-get msg :id))) 0)) + (chosen-type (nth chosen-nth + (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part + (inner-parts (plist-get part :content)) + (start (point))) +;; If we have run out of
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Fri, Aug 10 2012, Jani Nikula j...@nikula.org wrote: How would this work together with something like [1] (rationale in [2])? [1] id:ab777cf0fa83778d3399ac52094df9230738819d.1328798471.git.j...@nikula.org [2] id:cover.1328719309.git.j...@nikula.org If you introduce a mechanism to store the state, could it be extended to store the state of each individual part? That, in turn, could be used to add support for expanding/collapsing each alternative part through the buttons (e.g. [ text/html (not shown) ]). Each button could toggle the state of the part, and refresh buffer. Hey, Jani. Are these patches needed if we have Mark's patch? I would prefer to see Mark's solution. Since alternative parts are supposed to be just that, alternative, it seems to me that a solution that would cycle through display of these parts is really what we want. Is there a strong need to show multiple alternative parts at the exact same time? jamie. pgpOyTnqDQsSx.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Aug 10, 2012 7:18 PM, Jameson Graef Rollins jroll...@finestructure.net wrote: On Fri, Aug 10 2012, Jani Nikula j...@nikula.org wrote: How would this work together with something like [1] (rationale in [2])? [1] id: ab777cf0fa83778d3399ac52094df9230738819d.1328798471.git.j...@nikula.org [2] id:cover.1328719309.git.j...@nikula.org If you introduce a mechanism to store the state, could it be extended to store the state of each individual part? That, in turn, could be used to add support for expanding/collapsing each alternative part through the buttons (e.g. [ text/html (not shown) ]). Each button could toggle the state of the part, and refresh buffer. Hey, Jani. Are these patches needed if we have Mark's patch? I would prefer to see Mark's solution. Since alternative parts are supposed to be just that, alternative, it seems to me that a solution that would cycle through display of these parts is really what we want. Is there a strong need to show multiple alternative parts at the exact same time? Thanks to broken Microsoft mail clients, I get plenty of invitations that have text/plain and text/calendar alternative parts with information complimenting each other. I usually need to see both (luckily the included html part I can ignore) and it's helpful if I can see them at the same time. In a perfect world neither you or me would need any of this functionality... I suppose cycling through the alternative parts is, in a sense, correct for the reasons you state, we have the code here to do just that, and I can always cook up something for myself. Let's go with this, then, to move forward. BR, Jani. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Fri, 10 Aug 2012, Jani Nikula j...@nikula.org wrote: On Aug 10, 2012 7:18 PM, Jameson Graef Rollins jroll...@finestructure.net wrote: On Fri, Aug 10 2012, Jani Nikula j...@nikula.org wrote: How would this work together with something like [1] (rationale in [2])? [1] id: ab777cf0fa83778d3399ac52094df9230738819d.1328798471.git.j...@nikula.org [2] id:cover.1328719309.git.j...@nikula.org If you introduce a mechanism to store the state, could it be extended to store the state of each individual part? That, in turn, could be used to add support for expanding/collapsing each alternative part through the buttons (e.g. [ text/html (not shown) ]). Each button could toggle the state of the part, and refresh buffer. Hey, Jani. Are these patches needed if we have Mark's patch? I would prefer to see Mark's solution. Since alternative parts are supposed to be just that, alternative, it seems to me that a solution that would cycle through display of these parts is really what we want. Is there a strong need to show multiple alternative parts at the exact same time? Thanks to broken Microsoft mail clients, I get plenty of invitations that have text/plain and text/calendar alternative parts with information complimenting each other. I usually need to see both (luckily the included html part I can ignore) and it's helpful if I can see them at the same time. In a perfect world neither you or me would need any of this functionality... I suppose cycling through the alternative parts is, in a sense, correct for the reasons you state, we have the code here to do just that, and I can always cook up something for myself. Let's go with this, then, to move forward. Hi I am not sure I agree: I think maybe toggling parts is better. Either the parts contain the same information and then the current behaviour is probably fine, or they are not in which case we might want to see both at once Best wishes Mark ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Fri, Aug 10 2012, Mark Walters markwalters1...@gmail.com wrote: I am not sure I agree: I think maybe toggling parts is better. Either the parts contain the same information and then the current behaviour is probably fine, or they are not in which case we might want to see both at once If the case we're talking about is the alternative part having additional information, then why is cycling not sufficient? Shouldn't I be able to cycle to a part that shows all the information I'm interested in? I assume that even in the most broken mailers the alternatives still show mostly redundant information. I really like the cycling behavior, but If this solution is really not enough then how about we just start off with the alternate parts collapsed, and you can click on them to open them. I don't really like the idea of having special code to handle specific alternative parts. That seems overly complicated to me. jamie. pgpgOWCUbJplF.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Jun 23, 2012 1:34 AM, David Bremner da...@tethera.net wrote: Jani Nikula j...@nikula.org writes: A small wrinkle is that in order to make the variable survive notmuch-show-refresh-view (which is required for expanding/collapsing the parts, but ends up calling kill-all-local-variables through notmuch-show-mode) it is necessary to give it the permanent-local property. The code looks simple enough; should we apply this patch while we wait for something fancier? Applying this doesn't make the fancy stuff harder to do at all. And this doesn't have a key binding, on purpose, so we're not committed to that either. But then I'm biased... I don't really know how to evaluate the permanent-local bit. All the other show mode toggles use that too. It's probably less surprising like this than the permanently local only as needed approach in v1. d ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Tue, 19 Jun 2012, Jani Nikula j...@nikula.org wrote: Add function notmuch-show-toggle-multipart-alternative to toggle the value of notmuch-show-all-multipart/alternative-parts variable in the buffer, and redisplay the thread with either all or preferred alternative parts expanded. A small wrinkle is that in order to make the variable survive notmuch-show-refresh-view (which is required for expanding/collapsing the parts, but ends up calling kill-all-local-variables through notmuch-show-mode) it is necessary to give it the permanent-local property. Hi This patch looks good to me with one small concern: I set the variable notmuch-show-all-multipart/alternative-parts in my an emacs file loaded from my .emacs file rather than through customize using (setq notmuch-show-all-multipart/alternative-parts nil) This no longer works because of the buffer local property (ie I see all parts): I need to use (setq-default notmuch-show-all-multipart/alternative-parts nil) This change might be worth flagging up in the news file. I do not think the existence of my semi-patch id:87pq8vokmp@qmul.ac.uk should hold up this patch. My patch is only a draft and I definitely do not know enough lisp to be able to write a correct version (with correct quoting of message-ids etc). Best wishes Mark --- emacs/notmuch-show.el | 12 1 file changed, 12 insertions(+) diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el index 36cad93..4d3f03f 100644 --- a/emacs/notmuch-show.el +++ b/emacs/notmuch-show.el @@ -98,6 +98,18 @@ any given message. Should all parts of multipart/alternative parts be shown? :type 'boolean :group 'notmuch-show) +(make-variable-buffer-local 'notmuch-show-all-multipart/alternative-parts) +(put 'notmuch-show-all-multipart/alternative-parts 'permanent-local t) + +(defun notmuch-show-toggle-multipart-alternative () + Toggle the display of all multipart/alternative parts. + (interactive) + (setq notmuch-show-all-multipart/alternative-parts + (not notmuch-show-all-multipart/alternative-parts)) + (message (if notmuch-show-all-multipart/alternative-parts +Showing all multipart/alternative parts. + Showing preferred multipart/alternative part.)) + (notmuch-show-refresh-view)) (defcustom notmuch-show-indent-messages-width 1 Width of message indentation in threads. -- 1.7.9.5 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
Jani Nikula j...@nikula.org writes: A small wrinkle is that in order to make the variable survive notmuch-show-refresh-view (which is required for expanding/collapsing the parts, but ends up calling kill-all-local-variables through notmuch-show-mode) it is necessary to give it the permanent-local property. The code looks simple enough; should we apply this patch while we wait for something fancier? I don't really know how to evaluate the permanent-local bit. d ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: add function to toggle display of all multipart/alternative parts
On Tue, 19 Jun 2012, Jani Nikula j...@nikula.org wrote: Add function notmuch-show-toggle-multipart-alternative to toggle the value of notmuch-show-all-multipart/alternative-parts variable in the buffer, and redisplay the thread with either all or preferred alternative parts expanded. I see that Mark has some draft stuff for a more desirable approach, but decided to send this anyway, as this is fairly small and ready. The v2 is now in line with the existing show mode toggles. BR, Jani. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch