Merge lp:~akwm/vm/abbreviate-headers into lp:vm

[Arik]

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.

[Tim Cross (tcross)]

OK, will try to find time to play with it over the weekend.
Brief scan of the code looks good. However, your right about possible
conflicts with other configuraiton settings.

Will see if I can make it blow up! :)

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 "^$")
> + ...

[Arik (akwm)]

Thanks Tim. Messing around, I did find a case that behaves
undesirably, this occurs if one sets a field to 0 in
vm-abbreviate-headers. Doesn't seem to break and zero is probably a
silly value for anyone to try, so maybe highlighting this in the doc
is sufficient...

Similarly, for the fill headers, anything less than 2 (equally as
silly) causes odd display.

Anyway, these are probably extreme cases of protecting users from
themselves!

Thanks,
-Arik

Tim Cross writes:
 > OK, will try to find time to play with it over the weekend.
 > Brief scan of the code looks good. However, your right about possible
 > conflicts with other configuraiton settings.
 >
 > Will see if I can make it blow up! :)
 >
 > 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
 > > ...

[Tim Cross (tcross)]

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 m...

[Uday Reddy (reddyuday)]

Hi all,

I just got back from a week's trip and looking into this branch now.

> 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)

Tim, can you suggest the right type definition for this?

> 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.

No, we don't really have vm-presentation- as a package name. I think the
current names are fine as they stand.

> 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?

I have this question too.

Cheers,
Uday

[Uday Reddy (reddyuday)]

Hi Arik, here are my comments on the abbreviate-headers branch. (sorry for
the delay in reviewing this, due to some pre-arranged travel.)

- vm-fill-headers-column - the defcustom :type should be something like
  '(choice (const nil) integer)

- The regexp used for searching for header fields doesn't work for headers
  that have hyphens, e.g., X-Mailer. It must conform to the RFC
  definition of header fields. Check the function vm-match-header in
  vm-folder.el.

- Add a space between the header text and the "[...]" button to make it look
  cleaner. Can the button use the 'vm-mime-button-face? If so, you can
  shorten the button-text as well.

- The "[...]" button is a bit confusing because it doesn't indicate the
  current state of the abbreviation. It could be something like "[>>>]" for
  an abbreviated header and "[<<<]" for an expanded header.

- If this is going to replace rfaddons' shrunken-headers feature, then it is
  best to use the same naming, i.e., "shrunken" headers instead of "shrunk"
  headers or "abbreviated" headers. Differences in terminology can cause
  confusion down the road.

- The vm-fill-headers function seems a bit fiddly in engineering the
  correct indentation for the continuation lines. Can't the fill-prefix be
  used for this purpose, more cleanly?

- VM source files use a tab-width of 8 columns. So, please set your
  tab-width to 8 or turn off tabs.

Cheers,
Uday

[Arik (akwm)]

Thanks for the comments, I was away on travel as well and so couldn't
respond to Tim's thoughts, sorry. I was intending this to be a VM
native replacement for shrunken headers, along with the additional
fill-headers function which operates along the same lines.

I think I can address all of these, with the exception of using
fill-prefix for the fill-headers function, because it can accept a
value of nil, or characters other than a space and would not be
allowed as a header. I agree it is a bit clunky, but I'm not sure
another way to get the desired behaviour while using fill-region in
another way.

Thanks,
-Arik

Uday Reddy writes:
 > Hi Arik, here are my comments on the abbreviate-headers branch. (sorry for
 > the delay in reviewing this, due to some pre-arranged travel.)
 >
 > - vm-fill-headers-column - the defcustom :type should be something like
 > '(choice (const nil) integer)
 >
 > - The regexp used for searching for header fields doesn't work for headers
 > that have hyphens, e.g., X-Mailer. It must conform to the RFC
 > definition of header fields. Check the function vm-match-header in
 > vm-folder.el.
 >
 > - Add a space between the header text and the "[...]" button to make it look
 > cleaner. Can the button use the 'vm-mime-button-face? If so, you can
 > shorten the button-text as well.
 >
 > - The "[...]" button is a bit confusing because it doesn't indicate the
 > current state of the abbreviation. It could be something like "[>>>]" for
 > an abbreviated header and "[<<<]" for an expanded header.
 >
 > - If this is going to replace rfaddons' shrunken-headers feature, then it is
 > best to use the same naming, i.e., "shrunken" headers instead of "shrunk"
 > headers or "abbreviated" headers. Differences in terminology can cause
 > confusion down the road.
 >
 > - The vm-fill-headers function seems a bit fiddly in engineering the
 > correct indentation for the continuation lines. Can't the fill-prefix be
 > used for this purpose, more cleanly?
 >
 > - VM source files use a tab-width of 8 columns. So, please set your
 > tab-width to 8 or turn off tabs.
 >
 > Cheers,
 > Uday
 >
 > --
 > https://code.launchpad.net/~akwm/vm/abbreviate-headers/+merge/57440
 > You are the owner of lp:~akwm/vm/abbreviate-headers.

[Uday Reddy (reddyuday)]

Arik writes:

> Thanks for the comments, I was away on travel as well and so couldn't
> respond to Tim's thoughts, sorry. I was intending this to be a VM
> native replacement for shrunken headers, along with the additional
> fill-headers function which operates along the same lines.

Right. So, we will advise the users to stop using the rfaddons option for
shrunken headers, or even delete it from the distribution. If you used any
code from the rfaddons version, you should add a comment in the source file
and attribute the authorship to Rob F.

> I think I can address all of these, with the exception of using
> fill-prefix for the fill-headers function, because it can accept a
> value of nil, or characters other than a space and would not be
> allowed as a header. I agree it is a bit clunky, but I'm not sure
> another way to get the desired behaviour while using fill-region in
> another way.

I guess I can live with that. I am beginning to admire your earlier
approach for the thread-folding with an apparently fiddly basis on the
contents of the Summary buffer. Redoing it based on the internal thread
information costed me countless hours in debugging the threading code!

But add a comment saying how it is working, so that if somebody has to fix
it in future due to changes in the Emacs filling code, they will know what
is happening.

Cheers,
Uday

[lp:~akwm/vm/abbreviate-headers]

1177. By Arik on 2011-04-29

changed "abbreviate" to "shrunken/shrink", the expander button now
uses >>> and <<< to indicate the state of header shrinkage and also
uses the vm-mime-button face, altered the search for headers to use
vm-match-header functionality.

1178. By Arik on 2011-04-29

fixed a bug involving identifying header boundaries, cleaned up the
code as well to be more clear

[Arik (akwm)]

I think I have addressed all the issues brought up so far in the
updated branch. This includes using vm-match-header (which did not
have the most obvious doc-string, I didn't realize at first that it
matches the header at point and doesn't search...), improvements to
the indicator of shrunken headers using vm-mime-button face and the
">>>/<<<" indicators - though, Uday, I'm not sure what you meant about
it being shorter if the face could be used?, and added some comments
for the fill code along with some changes that make it smaller and
clearer.

Please let me know what you think about the changes. Then the next
thing is to add some info file documentation, I will try and find a
good entry point unless someone already has a good spot in mind.

Thanks,
-Arik

[lp:~akwm/vm/abbreviate-headers]

1179. By Arik on 2012-01-06

Added documentation to the new variables for automatically filling and
shrinking headers.

Unmerged revisions

1179. By Arik on 2012-01-06

Added documentation to the new variables for automatically filling and
shrinking headers.

1178. By Arik on 2011-04-29

fixed a bug involving identifying header boundaries, cleaned up the
code as well to be more clear

1177. By Arik on 2011-04-29

changed "abbreviate" to "shrunken/shrink", the expander button now
uses >>> and <<< to indicate the state of header shrinkage and also
uses the vm-mime-button face, altered the search for headers to use
vm-match-header functionality.

1176. By Arik on 2011-04-07

Added two functions to prettify the presentation of headers when
viewing a message. vm-abbreviate-headers will truncate headers in a
fasion quite similar to shrunken headers of rf-addons but is meant to
replace it, and vm-fill-headers attempts to uniformly display headers
that may have erratic layouts. Both are controlled by variables. Still
need info docs

=== modified file 'info/vm.texinfo'
--- info/vm.texinfo	2012-01-05 15:18:34 +0000
+++ info/vm.texinfo	2012-01-06 02:05:30 +0000
@@ -1753,6 +1753,29 @@
 specified by @code{vm-highlighted-header-face}, which defaults to
 'bold.
 
+@vindex vm-shrunken-headers
+@vindex vm-fill-headers-column
+Finally, some messages may have very long subject lines or a large
+list of recipients, which gets in the way of easily identifying the
+message or reading the message body. Two variables that allow the user
+to automatically manage this problem are @code{vm-shrunken-headers}
+and @code{vm-fill-headers-column}. The first,
+@code{vm-shrunken-headers}, takes a list of cons cells describing the
+header and the number of lines that header should show (e.g. (`''To''
+. 3), meaning the show three lines of the ``To:'' header). Undesirable
+results will happen if you select 0 lines; if you wish to not display
+the header, please instead modify the variable
+@code{vm-visible-headers}. @code{vm-fill-headers-column} will, prior
+to shrinking the headers via @code{vm-shrunken-headers}, fill all
+headers to that particular character width. In doing so, VM will first
+ensure there is only a single space in front of multi-line headers in
+order to maximize the use of the fill-column specified. All header
+modification is done in the presentation buffer and will not edit a
+real message. Both of these variables, if set to nil do nothing.
+@emph{note: vm-shrunken-headers is intended as a VM native replacement
+to the extension of the same name in vm-rfaddons.}
+
+
 @vindex vm-preview-read-messages
 By default, VM will not preview messages that are flagged as read.  To
 have VM preview all messages, set the value of
 
=== modified file 'lisp/vm-page.el'
--- lisp/vm-page.el	2012-01-03 16:49:23 +0000
+++ lisp/vm-page.el	2012-01-06 02:05:30 +0000
@@ -884,6 +884,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-shrunken-headers (vm-shrink-headers))
 
      ;; 6. Go to the text of message
      (if (and vm-honor-page-delimiters need-preview)
@@ -905,6 +910,91 @@
 
 (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)
+	start end)
+    (save-excursion
+      (goto-char (point-min))
+      (while (vm-match-header)
+	;; The following is to ensure that the filling is done
+	;; unanimously with one space to ensure the header is still a
+	;; valid multi-line header. for filling to applying the same
+	;; initial indent it requires 2 lines to have uniform indent
+	;; at the beginning, the following code is doing this
+	(setq start (point))
+	;; before doing any insertion of filling, which may add/remove
+	;; newlines save the current end of the header as a marker
+	(goto-char (- (vm-matched-header-end) 1))
+	(setq end (point-marker))
+	(goto-char start)
+	(insert " ")
+	(forward-line 1)
+	(when (looking-at "^[ \t]+")
+	  (search-forward-regexp "^[ \t]+")
+	  (delete-region (point-at-bol) (point))
+	  (insert " "))
+	(fill-region start end)
+	(goto-char start)
+	(delete-char 1)
+	(goto-char end)
+	(forward-line 1)))))
+
+(defun vm-shrink-headers ()
+  "Shrink the headers of the current message (is run in the
+presentation buffer) to the number of lines as indicated by the
+variable `vm-shrunken-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
+
+This functionality was entirely inspired and partially taken from
+Rob F's rfaddons feature shrunken headers"
+  (let ((headers vm-shrunken-headers) start end
+	(buffer-read-only nil))
+    (save-excursion
+      (while headers
+	(goto-char (point-min))
+	(when (search-forward-regexp 
+	       (concat "^" (car (car headers)) ": ") nil t)
+	  (goto-char (point-at-bol))
+	  (vm-match-header)
+	  (setq end (- (vm-matched-header-end) 1))
+	  (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-shrunken-headers
+			   'face 'vm-mime-button
+			   'vm-shrunken-header-start start
+			   'vm-shrunken-header-end end)))
+	(setq headers (cdr headers))))))
+
+(defun vm-show-shrunken-headers (button)
+  (let ((buffer-read-only nil)
+	(inv t))
+    (when (invisible-p
+	   (button-get button 'vm-shrunken-header-start))
+      (setq inv nil))
+    (put-text-property (button-get button 'vm-shrunken-header-start)
+		       (button-get button 'vm-shrunken-header-end)
+		       'invisible inv)
+    (save-excursion
+      (goto-char (button-start button))
+      (forward-char 2)
+      (delete-char 3)
+      (if inv
+	  (insert ">>>")
+	(insert "<<<")))))
+
 (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	2012-01-03 16:49:23 +0000
+++ lisp/vm-vars.el	2012-01-06 02:05:30 +0000
@@ -1053,6 +1053,25 @@
   :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 '(choice (const nil) integer))
+
+(defcustom vm-shrunken-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 '(choice (const nil)
+		 (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