Re: [PATCH] emacs: tree: support fold/unfold thread

["Masson\, Julien" <massonju.eseo@gmail.com>]

Hello

Thanks for the reviews, I appreciate.

On Sat 23 Mar 2019 at 08:07, Mark Walters <markwalters1009@gmail.com> wrote:

> Hello
>
>> This patch allow the user to fold/unfold a thread in the current tree
>> buffer by pressing "t" key.
>
> This looks like a really nice feature! I will try and review the code
> properly soon, but have some preliminary comments.
>
>> By default a string is displayed at the beginning of the overlay to
>> indicate that this thread is folded.
>
> I wonder if it would make sense to make the [....] replace the "tree
> symbol" (-> etc)? This would mean that it was always on the screen and the
> tree symbol looks a little odd with a collapsed thread.

The range of the overlay is from the end of the line of the subject to
end of the line of the last message.
So on the screen the string "[...]" is placed at the end of the Subject.

If I change the range from the "tree symbol" the subject will be
invisible.

I chose to placed the overlay like this to make sure I still see the
subject of the folded thread.

But if you prefer I can place the overlay from the "tree symbol" position.

>
> Secondly, I wonder whether making it collapse just the subthread below
> the current message (ie a subtree) would be nice? To me that feels more
> generic, but might be more effort than it's worth as the code would need
> to deal with nested folds. Then C-u t could do exactly the current
> folding (ie the whole thread). But this is just a thought.

yes sure I'll send a v2 which handle sub-thread folding.

>
> Finally a comment on the code
>
>> +The overlay found is located between START and END position in BUFFER."
>> +  (seq-find (lambda (ov)
>
>> +(defun notmuch-tree-clean-up-overlays ()
>> +  "Remove overlays not referenced to any buffer"
>> +  (setq notmuch-tree-overlays (seq-filter #'overlay-buffer notmuch-tree-overlays)))
>
> seq-find and seq-filter are emacs 25+ only I think; at least they don't
> seem to be in emacs24 which I think we still support. Perhaps some cl
> functions can be used (eg remove-if-not) instead?

Ah yes sorry, I'll change this in the v2.

>
> Best wishes
>
> Mark

Thanks


Julien Masson