Code review comment for lp:~akwm/vm/abbreviate-headers

Hi Arik,

having a bit of a look at your suggested additions/extensions. Just a few
comments.

1. Your defcustom for vm-fill-headers-column has a 'mismatch' error (I think
you need to define the value as being either an integrer or nil. If defined
as just an integer, it sees the default of nil as a mismatch)

2. Do you think your variables and possibly function names should have
something like vm-presentation-* as a prefix. While I realise this does tend
to make identifiers quite long, it does seem to fit with a lot of the VM
coding style. It also helps make it clear where these functions/variables
operate i.e. the presentaiton as apart from composition or summary etc.

3. I'm a little confused as to how your extensions fit in with the rfaddons
shrunken headers features. Is it meant as a general replacement or as an
extension? Is there any dependency between the two?

It is interesting that I've noticed (prior to trying out your patch) that
the From: header of 'some' messages was being 'shrunken' - I suspect it is
something to do with enabling shrunken headers. The weird thing is that it
does it on some quite short from lines and not on others that are longer. I
was going to look into this when time permitted.

Just to clarify how your extension is meant to work.

You set a header 'fill column' to enable this feature. This column will
cause headers to be broken into multiple lines at that column. If the header
wraps into more lines than specified in
vm-abbreviate-headers, the header is truncated and a button added at the end
for when you want to show the full header.

I will continue to play with the extension for the next few days and see
what results I get.

Tim

On Wed, Apr 13, 2011 at 4:19 PM, Arik <email address hidden> wrote:

> Arik has proposed merging lp:~akwm/vm/abbreviate-headers into lp:vm.
>
> Requested reviews:
> Tim Cross (tcross)
>
> For more details, see:
> https://code.launchpad.net/~akwm/vm/abbreviate-headers/+merge/57440
>
> Change adds to core VM two functions for handling the display of headers.
> One will abbreviate the number of lines in any header to an amount
> particular to that header as given by the variable vm-abbreviate-headers.
> The other fills headers to a particular column (at the same time uniformly
> making a single space at each new line) controlled by
> vm-fill-headers-column. Both these variables, when set to nil, will cause VM
> to do nothing. Both likewise act only in the presentation buffer so will not
> make changes to the message itself.
>
> Should make sure that it is indeed always the presentation buffer. Should
> also check for corner cases perhaps with conflicting user config etc.
> --
> https://code.launchpad.net/~akwm/vm/abbreviate-headers/+merge/57440
> You are requested to review the proposed merge of
> lp:~akwm/vm/abbreviate-headers into lp:vm.
>
> === modified file 'lisp/vm-page.el'
> --- lisp/vm-page.el 2011-03-30 20:33:50 +0000
> +++ lisp/vm-page.el 2011-04-13 06:19:23 +0000
> @@ -873,6 +873,11 @@
> (vm-energize-urls-in-message-region)
> (vm-highlight-headers-maybe)
> (vm-energize-headers-and-xfaces))
> +
> + ;; 5.5 prettify the headers of the message while still
> + ;; in the presentation buffer
> + (when vm-fill-headers-column (vm-fill-headers))
> + (when vm-abbreviate-headers (vm-abbreviate-headers))
>
> ;; 6. Go to the text of message
> (if (and vm-honor-page-delimiters need-preview)
> @@ -894,6 +899,76 @@
>
> (defalias 'vm-preview-current-message 'vm-present-current-message)
>
> +(defun vm-fill-headers ()
> + "Fill headers of the message in the current buffer using
> +`vm-fill-headers-column' to determine the column to fill to.
> +This function will be run (given that `vm-fill-headers-column' is
> +non-nil) in the presentation buffer when viewing a message."
> + (let ((buffer-read-only nil)
> + (fill-column vm-fill-headers-column)
> + done start )
> + (save-excursion
> + (goto-char (point-min))
> + (when (search-forward-regexp "^\\w+: " nil t)
> + (while (not done)
> + (setq start (point))
> + (goto-char (point-at-bol))
> + (insert " ")
> + (forward-line 1)
> + (when (looking-at "^[ \t]+")
> + (search-forward-regexp "^[ \t]+")
> + (delete-region (point-at-bol) (point))
> + (insert " "))
> + (when (or (not (search-forward-regexp "^\\w+: \\|^$" nil
> t))
> + (looking-at "^$")
> + (= (point) (buffer-size)))
> + (setq done t))
> + (save-excursion (fill-region start (point-at-eol 0))
> + (goto-char start)
> + (goto-char
> (point-at-bol))
> + (delete-char 1)))))))
> +
> +(defun vm-abbreviate-headers ()
> + "Abbreviate the headers of the current message (is run in the
> +presentation buffer) to the number of lines as indicated by the
> +variable `vm-abbreviate-headers' (when non-nil). Headers in this
> +list with more lines than specified will have those lines hidden
> +and place a button of the form [...] allowing their
> +expansion. This can make viewing significantly more pleasant for
> +messages sent to (or CC'ed to) many recipients"
> + (let ((headers vm-abbreviate-headers)
> + s start end
> + (buffer-read-only nil))
> + (save-excursion
> + (while headers
> + (goto-char (point-min))
> + (when (search-forward-regexp
> + (concat "^" (car (car headers)) ": ") nil t)
> + (setq s (point))
> + (search-forward-regexp "^\\w+: \\|^$")
> + (setq end (- (point-at-bol) 1))
> + (goto-char s)
> + (end-of-line (cdr (car headers)))
> + (when (and (< (point) end) (not (invisible-p (point))))
> + (setq start (point))
> + (put-text-property start end
> + 'invisible t)
> + (goto-char end)
> + (insert-button "[...]" 'action
> 'vm-show-abbreviated-headers
> + 'vm-shrunk-header-start
> start
> + 'vm-shrunk-header-end
> end)))
> + (setq headers (cdr headers))))))
> +
> +(defun vm-show-abbreviated-headers (button)
> + (let ((buffer-read-only nil)
> + (inv t))
> + (when (invisible-p
> + (button-get button 'vm-shrunk-header-start))
> + (setq inv nil))
> + (put-text-property (button-get button 'vm-shrunk-header-start)
> + (button-get button
> 'vm-shrunk-header-end)
> + 'invisible inv)))
> +
> (defun vm-show-current-message ()
> "Show the current message in the Presentation Buffer. MIME decoding
> is done if necessary. (USR, 2010-01-14)"
>
> === modified file 'lisp/vm-vars.el'
> --- lisp/vm-vars.el 2011-04-06 22:41:57 +0000
> +++ lisp/vm-vars.el 2011-04-13 06:19:23 +0000
> @@ -1017,6 +1017,24 @@
> :group 'vm-presentation
> :type '(choice (const nil) regexp))
>
> +(defcustom vm-fill-headers-column nil
> + "*Non-nil value tells VM to fill the headers in the
> +presentation copy of the current message to this column. This
> +will fill all headers visible in the presentation, also ensuring
> +that subsequent lines are offset uniformly by a single space."
> + :group 'vm-presentation
> + :type 'integer)
> +
> +(defcustom vm-abbreviate-headers '(("To" . 3)("Subject" . 2)("Cc" . 3))
> + "*Non-nil value should be a list of cons cells each having as
> +its car the name of the header (e.g. 'To') where the cdr is the
> +number of lines to be displayed for this header, after which VM
> +will insert a button in the presentation that, when pushed, will
> +make visible temporarily the remaining lines of that header"
> + :group 'vm-presentation
> + :type '(alist :key-type (string :tag "Header name")
> + :value-type (integer :tag "Number of
> lines")))
> +
> (defcustom vm-highlighted-header-regexp nil
> "*Value specifies which headers to highlight.
> This is a regular expression that matches the names of headers that should
>
>
>