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

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

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

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

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

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

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.

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

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