[PATCH 2/3] emacs: show: add overlays for each part
Mark Walters
markwalters1009 at gmail.com
Thu Dec 13 00:54:22 PST 2012
Hi
Many thanks for the review: I will post a new version shortly. Some
comments inline below:
On Tue, 11 Dec 2012, Austin Clements <aclements at csail.mit.edu> wrote:
> On Tue, 04 Dec 2012, Mark Walters <markwalters1009 at gmail.com> wrote:
>> This make notmuch-show-insert-bodypart add an overlay for any
>
> s/make/makes/
>
>> non-trivial part with a button header (currently the first text/plain
>> part does not have a button). At this point the overlay is available
>> to the button but there is no action using it yet.
>>
>> In addition a not-shown variable which is used to request the part be
>
> not-shown is really an argument (I found this confusing).
>
>> hidden by default down to the overlay but this is not acted on yet.
>> ---
>> emacs/notmuch-show.el | 62 +++++++++++++++++++++++++++++++++++++-----------
>> 1 files changed, 48 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index f8ce037..3215ebc 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -569,10 +569,9 @@ message at DEPTH in the current thread."
>> ;; should be chosen if there are more than one that match?
>> (mapc (lambda (inner-part)
>> (let ((inner-type (plist-get inner-part :content-type)))
>> - (if (or notmuch-show-all-multipart/alternative-parts
>> - (string= chosen-type inner-type))
>> - (notmuch-show-insert-bodypart msg inner-part depth)
>> - (notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))
>> + (notmuch-show-insert-bodypart msg inner-part depth
>> + (not (or notmuch-show-all-multipart/alternative-parts
>
> Since notmuch-show-all-multipart/alternative-parts was basically a hack
> around our poor multipart/alternative support, I think this series (or a
> follow up patch) should change its default to nil or even eliminate it
> entirely.
I have added a patch at the end setting this to nil by default.
>> + (string= chosen-type inner-type))))))
>
> You could let-bind the (not (or ..)) to some variable ("hide" perhaps)
> in the let above to avoid this crazy line length.
>
>> inner-parts)
>>
>> (when notmuch-show-indent-multipart
>> @@ -840,17 +839,52 @@ message at DEPTH in the current thread."
>> (setq handlers (cdr handlers))))
>> t)
>>
>> -(defun notmuch-show-insert-bodypart (msg part depth)
>> - "Insert the body part PART at depth DEPTH in the current thread."
>> +(defun notmuch-show-insert-part-overlays (msg beg end not-shown)
>
> s/not-shown/hide/? Or hidden?
>
>> + "Add an overlay to the part between BEG and END"
>> + (let* ((button (button-at beg))
>> + (part-beg (and button (1+ (button-end button)))))
>> +
>> + ;; If the part contains no text we do not make it toggleable.
>> + (unless (or (not button) (eq part-beg end))
>
> (when (and button (/= part-beg end)) ...) ?
>
>> + (let ((base-label (button-get button :base-label))
>> + (overlay (make-overlay part-beg end))
>> + (message-invis-spec (plist-get msg :message-invis-spec))
>> + (invis-spec (make-symbol "notmuch-part-region")))
>> +
>> + (overlay-put overlay 'invisible (list invis-spec message-invis-spec))
>
> Non-trivial buffer invisibility specs are really bad for performance
> (Emacs' renderer does the obvious O(n^2) thing when rendering a buffer
> with an invisibility spec). Unfortunately, because of notmuch-wash and
> the way overlays with trivial 'invisible properties combine with
> overlays with list-type 'invisible properties combine, I don't think it
> can be avoided. But if we get rid of buffer invisibility specs from
> notmuch-wash, this code can also get much simpler.
How do you plan to get rid of the invisibility properties from notmuch
wash?
>> + (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
>
> This will leave the "(not shown)" in the part header if isearch unfolds
> the part.
>
> Do we even want isearch to unfold parts? It's not clear we do for
> multipart/alternative. If we do, probably the right thing is something
> like
I don't think we want to search hidden bodyparts so I just deleted this
line.
>
> (overlay-put overlay 'notmuch-show-part-button button)
> (overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)
>
> (defun notmuch-show-part-isearch-open (overlay)
> (notmuch-show-toggle-invisible-part-action
> (overlay-get overlay 'notmuch-show-part-button)))
>
>> + (overlay-put overlay 'priority 10)
>> + (overlay-put overlay 'type "part")
>> + ;; Now we have to add invis-spec to every overlay this
>> + ;; overlay contains, otherwise these inner overlays will
>> + ;; override this one.
>
> Interesting. In the simple case of using nil or t for 'invisible, the
> specs do combine as one would expect, but you're right that, with a
> non-trivial invisibility-spec, the highest priority overlay wins. It's
> too bad we don't know the depth of the part or we could just set the
> overlay priority. This is another thing that would go away if we didn't
> use buffer invisibility-specs.
I don't think priorities get it right. As far as I could tell if you
have two overlays at different priorities both with invisibility
properties then the higher priority overlay decides visibility by
itself: that is if it says invisible then the text is invisible, and if
it says visible then the text is visible.
>
>> + (mapc (lambda (inner)
>
> I would use a (dolist (inner (overlays-in part-beg end)) ...) here.
> Seems a little more readable.
>
>> + (when (and (>= (overlay-start inner) part-beg)
>> + (<= (overlay-end inner) end))
>> + (overlay-put inner 'invisible
>> + (cons invis-spec (overlay-get inner 'invisible)))))
>> + (overlays-in part-beg end))
>> +
>> + (button-put button 'invisibility-spec invis-spec)
>> + (button-put button 'overlay overlay))
>> + (goto-char (point-max)))))
>
> This goto-char seems oddly out of place, since it has nothing to do with
> overlay creation. Was it supposed to be here instead of in
> notmuch-show-insert-bodypart? Is it even necessary?
It was suppose to be in notmuch-show-insert-body-part. It was necessary
but I have wrapped the toggle-button call in the third patch and now it
does not seem necessary.
It is possible that the toggle-button function should not move point: if
that got changed the save-excursion here could be removed. (I will
discuss this in my reply to your comments on the next patch).
Best wishes
Mark
>
>> +
>> +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
>
> Same comment about not-shown. (Also in the commit message.)
>
>> + "Insert the body part PART at depth DEPTH in the current thread.
>> +
>> +If not-shown is TRUE then initially hide this part."
>
> s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/
>
>> (let ((content-type (downcase (plist-get part :content-type)))
>> - (nth (plist-get part :id)))
>> - (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type))
>> - ;; Some of the body part handlers leave point somewhere up in the
>> - ;; part, so we make sure that we're down at the end.
>> - (goto-char (point-max))
>> - ;; Ensure that the part ends with a carriage return.
>> - (unless (bolp)
>> - (insert "\n")))
>> + (nth (plist-get part :id))
>> + (beg (point)))
>> +
>> + (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type)
>> + ;; Some of the body part handlers leave point somewhere up in the
>> + ;; part, so we make sure that we're down at the end.
>> + (goto-char (point-max))
>> + ;; Ensure that the part ends with a carriage return.
>> + (unless (bolp)
>> + (insert "\n"))
>> + (notmuch-show-insert-part-overlays msg beg (point) not-shown)))
>>
>> (defun notmuch-show-insert-body (msg body depth)
>> "Insert the body BODY at depth DEPTH in the current thread."
>> --
>> 1.7.9.1
More information about the notmuch
mailing list