VM

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

abbreviate-headers
Merge into trunk

Revision history for this message

Arik (akwm) wrote on 2011-04-14:

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
> > + (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
> >
> >
> >
>
> --
> https://code.launchpad.net/~akwm/vm/abbreviate-headers/+merge/57440
> You are the owner of lp:~akwm/vm/abbreviate-headers.

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 <arik.mitschang@gmail.com> 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
 > >
 > >
 > >
 > 
 > -- 
 > https://code.launchpad.net/~akwm/vm/abbreviate-headers/+merge/57440
 > You are the owner of lp:~akwm/vm/abbreviate-headers.

« Back to merge proposal