[PATCH 0/2] re-enable line wrapping and add some header bling

[PATCH 0/2] re-enable line wrapping and add some header bling

[David Edmondson]

By default, re-enable `visual-line-mode' in `notmuch-show-mode'. Do it
via a hook so that purists (ahem) can turn it off.

Add some more processing of headers to make them look nice. Do it via
hooks so that unbelievers can turn it off.

David Edmondson (2):
  emacs: Re-enable line wrapping in `notmuch-show-mode'.
  emacs: Add more processing of displayed headers.

 emacs/notmuch-show.el |   50 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 42 insertions(+), 8 deletions(-)

-- 
1.7.8.3

[PATCH 1/2] emacs: Re-enable line wrapping in `notmuch-show-mode'.

[David Edmondson]

Turn on `visual-line-mode' via a hook, so that those who so choose can
avoid it.
---
 emacs/notmuch-show.el |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e6a5b31..effd2fd 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -71,9 +71,10 @@ any given message."
   "A list of functions called to decorate the headers listed in
 `notmuch-message-headers'.")
 
-(defcustom notmuch-show-hook nil
+(defcustom notmuch-show-hook '(notmuch-show-turn-on-visual-line-mode)
   "Functions called after populating a `notmuch-show' buffer."
   :type 'hook
+  :options '(notmuch-show-turn-on-visual-line-mode)
   :group 'notmuch-show
   :group 'notmuch-hooks)
 
@@ -133,6 +134,10 @@ indentation."
            ,@body)
 	 (kill-buffer buf)))))
 
+(defun notmuch-show-turn-on-visual-line-mode ()
+  "Enable Visual Line mode."
+  (visual-line-mode t))
+
 (defun notmuch-show-view-all-mime-parts ()
   "Use external viewers to view all attachments from the current message."
   (interactive)
-- 
1.7.8.3

Re: [PATCH 1/2] emacs: Re-enable line wrapping in `notmuch-show-mode'.

[Tomi Ollila]

On Thu, 26 Jan 2012 08:17:50 +0000, David Edmondson <dme@dme.org> wrote:
> Turn on `visual-line-mode' via a hook, so that those who so choose can
> avoid it.
> ---

+1

I think the docstring is good (or it could be
nonexistent like in 'turn-on-font-lock).


Tomi

Re: [PATCH 1/2] emacs: Re-enable line wrapping in `notmuch-show-mode'.

[David Bremner]

On Thu, 26 Jan 2012 08:17:50 +0000, David Edmondson <dme@dme.org> wrote:
> Turn on `visual-line-mode' via a hook, so that those who so choose can
> avoid it.

pushed,

d

[PATCH 2/2] emacs: Add more processing of displayed headers.

[David Edmondson]

Wrap headers to the width of the window and indent continuations.
---
 emacs/notmuch-show.el |   43 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index effd2fd..ad286d1 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -67,9 +67,16 @@ any given message."
   :type 'boolean
   :group 'notmuch-show)
 
-(defvar notmuch-show-markup-headers-hook '(notmuch-show-colour-headers)
+(defcustom notmuch-show-markup-headers-hook '(notmuch-show-colour-headers
+					      notmuch-show-fill-headers
+					      notmuch-show-indent-continuations)
   "A list of functions called to decorate the headers listed in
-`notmuch-message-headers'.")
+`notmuch-message-headers'."
+  :type 'hook
+  :options '(notmuch-show-colour-headers
+	     notmuch-show-fill-headers
+	     notmuch-show-indent-continuations)
+  :group 'notmuch-show)
 
 (defcustom notmuch-show-hook '(notmuch-show-turn-on-visual-line-mode)
   "Functions called after populating a `notmuch-show' buffer."
@@ -268,13 +275,35 @@ operation on the contents of the current buffer."
     (overlay-put (make-overlay (point) (re-search-forward ".*$"))
 		 'face face)))
 
-(defun notmuch-show-colour-headers ()
+(defun notmuch-show-colour-headers (depth)
   "Apply some colouring to the current headers."
   (goto-char (point-min))
   (while (looking-at "^[A-Za-z][-A-Za-z0-9]*:")
     (notmuch-show-fontify-header)
     (forward-line)))
 
+(defun notmuch-show-fill-headers (depth)
+  "Wrap the text of the current headers."
+
+  ;; '-5' to allow for the indentation code.
+  (let ((fill-column (- (window-width) depth 5)))
+    (goto-char (point-min))
+    (while (not (eobp))
+      (let ((start (point)))
+	(end-of-line)
+	;; We're left at the start of the next line, so there's no need
+	;; to move forward after filling.
+	(fill-region-as-paragraph start (point))))))
+
+(defun notmuch-show-indent-continuations (depth)
+  "Indent any continuation lines."
+  (goto-char (point-min))
+  (while (not (eobp))
+    (if (not (looking-at "^[A-Za-z][-A-Za-z0-9]*:"))
+	;; Four spaces tends to work well with 'To' and 'Cc' headers.
+	(insert "    "))
+    (forward-line)))
+
 (defun notmuch-show-spaces-n (n)
   "Return a string comprised of `n' spaces."
   (make-string n ? ))
@@ -329,7 +358,7 @@ message at DEPTH in the current thread."
   "Insert a single header."
   (insert header ": " header-value "\n"))
 
-(defun notmuch-show-insert-headers (headers)
+(defun notmuch-show-insert-headers (headers depth)
   "Insert the headers of the current message."
   (let ((start (point)))
     (mapc (lambda (header)
@@ -342,7 +371,7 @@ message at DEPTH in the current thread."
     (save-excursion
       (save-restriction
 	(narrow-to-region start (point-max))
-	(run-hooks 'notmuch-show-markup-headers-hook)))))
+	(run-hook-with-args 'notmuch-show-markup-headers-hook depth)))))
 
 (define-button-type 'notmuch-show-part-button-type
   'action 'notmuch-show-part-button-default
@@ -633,7 +662,7 @@ current buffer, if possible."
     ;; Override `notmuch-message-headers' to force `From' to be
     ;; displayed.
     (let ((notmuch-message-headers '("From" "Subject" "To" "Cc" "Date")))
-      (notmuch-show-insert-headers (plist-get message :headers)))
+      (notmuch-show-insert-headers (plist-get message :headers) 0))
 
     ;; Blank line after headers to be compatible with the normal
     ;; message display.
@@ -826,7 +855,7 @@ current buffer, if possible."
     ;; Set `headers-start' to point after the 'Subject:' header to be
     ;; compatible with the existing implementation. This just sets it
     ;; to after the first header.
-    (notmuch-show-insert-headers headers)
+    (notmuch-show-insert-headers headers depth)
     ;; Headers should include a blank line (backwards compatibility).
     (insert "\n")
     (save-excursion
-- 
1.7.8.3

Re: [PATCH 0/2] re-enable line wrapping and add some header bling

[Dmitry Kurochkin]

Hi David.

On Thu, 26 Jan 2012 08:17:49 +0000, David Edmondson <dme@dme.org> wrote:
> By default, re-enable `visual-line-mode' in `notmuch-show-mode'. Do it
> via a hook so that purists (ahem) can turn it off.
> 
> Add some more processing of headers to make them look nice. Do it via
> hooks so that unbelievers can turn it off.
> 

I did not review the code, but here is a general comment for both
patches (but especially for the first one).  It would be nice to have a
more detailed documentation for hooks.  Docstring like "Enable Visual
Line mode." for a function named `notmuch-show-turn-on-visual-line-mode'
is near useless.  It is quite obvious that the function enables
visual-line-mode from it's name.  And it does not give any information
on why would someone actually want to use it.  I do not remember what
visual-line-mode is exactly, so to understand whether this hook is
actually useful for me, I have to read visual-line-mode docs, think
about how it helps in notmuch-show, read some code, perhaps, etc.  I
would argue that since the hook itself is trivial, the main point in
having it is to provide a clearly documented solution for a common
problem for those who do not know how to solve this problem right away.
Currently, those who know what visual-line-mode is do not need this
hook, because they can easily write their own, and those who do not know
what visual-line-mode is can not use this hook, because it says nothing
about why it is actually useful.

Also, in addition to better docs, I would rename
`notmuch-show-turn-on-visual-line-mode' to something that reflects what
it does from user POV (like the other two hooks).

Though, the fact that the hook is enabled by default makes the above
arguments less important, I guess.

Regards,
  Dmitry

> David Edmondson (2):
>   emacs: Re-enable line wrapping in `notmuch-show-mode'.
>   emacs: Add more processing of displayed headers.
> 
>  emacs/notmuch-show.el |   50 +++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 42 insertions(+), 8 deletions(-)
> 
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 0/2] re-enable line wrapping and add some header bling

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

On Thu, 26 Jan 2012 20:17:49 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I did not review the code, but here is a general comment for both
> patches (but especially for the first one).  It would be nice to have a
> more detailed documentation for hooks.  Docstring like "Enable Visual
> Line mode." for a function named `notmuch-show-turn-on-visual-line-mode'
> is near useless.  It is quite obvious that the function enables
> visual-line-mode from it's name.  And it does not give any information
> on why would someone actually want to use it.  I do not remember what
> visual-line-mode is exactly, so to understand whether this hook is
> actually useful for me, I have to read visual-line-mode docs, think
> about how it helps in notmuch-show, read some code, perhaps, etc.  I
> would argue that since the hook itself is trivial, the main point in
> having it is to provide a clearly documented solution for a common
> problem for those who do not know how to solve this problem right away.
> Currently, those who know what visual-line-mode is do not need this
> hook, because they can easily write their own, and those who do not know
> what visual-line-mode is can not use this hook, because it says nothing
> about why it is actually useful.
> 
> Also, in addition to better docs, I would rename
> `notmuch-show-turn-on-visual-line-mode' to something that reflects what
> it does from user POV (like the other two hooks).
> 
> Though, the fact that the hook is enabled by default makes the above
> arguments less important, I guess.

I have a bunch of somewhat conflicting thoughts about this:
  - Being able to configure the behaviour without having to change the
    core code is good, so implementing behaviour using hook functions is
    useful.
  - Things should behave well in the default configuration, so most of
    the hook functions are enabled by default.
  - Everything can't be hook functions, so there's a balance to be made
    between implementing things as in-line code and via hook functions.
  - Most users shouldn't need to modify any of the hooks.
  - Documentation that explains what a hook function is about is
    obviously good.
  - Documenting something that is external to notmuch can be both
    wasteful and risky.

    Wasteful because such documentation typically already exists and
    risky because the precise behaviour of external components is not
    under our control.

    For example, the documentation for `visual-line-mode' is both
    concise and good, so there's little point in repeating it and, of
    course, the exact details of what `visual-line-mode' does can be
    changed by the Emacs developers. One would expect that they would
    update the documentation for `visual-line-mode' in such situations,
    which would leave any cloned documentation in an incorrect state.

Hence, I would probably take issue with your statement:

> I would argue that since the hook itself is trivial, the main point in
> having it is to provide a clearly documented solution for a common
> problem for those who do not know how to solve this problem right
> away.

That's not the intention of the hook functions under discussion
here. They are hook functions so that a curious and interested user can
make a change based on some aesthetic preference. The are not about
solving problems with the default configuration.

I think that `turn-on-visual-line-mode' (or the package local derivative
of it that was chosen) is precisely the right name for a function that
enables `visual-line-mode'. It describes perfectly and succinctly what
the function does and provides a pointer to follow to find out more (the
documentation for `visual-line-mode') without a user even having to
examine the documentation of the function itself.

All of that said, I'd agree that the documentation of
`notmuch-show-indent-continuations' could have been better. How about:
  "Indent any continuation lines in the current headers."
?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/2] re-enable line wrapping and add some header bling

[Jani Nikula]

On Fri, 27 Jan 2012 06:52:46 +0000, David Edmondson <dme@dme.org> wrote:
> On Thu, 26 Jan 2012 20:17:49 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > I did not review the code, but here is a general comment for both
> > patches (but especially for the first one).  It would be nice to have a
> > more detailed documentation for hooks.  Docstring like "Enable Visual
> > Line mode." for a function named `notmuch-show-turn-on-visual-line-mode'
> > is near useless.  It is quite obvious that the function enables
> > visual-line-mode from it's name.  And it does not give any information
> > on why would someone actually want to use it.  I do not remember what
> > visual-line-mode is exactly, so to understand whether this hook is
> > actually useful for me, I have to read visual-line-mode docs, think
> > about how it helps in notmuch-show, read some code, perhaps, etc.  I
> > would argue that since the hook itself is trivial, the main point in
> > having it is to provide a clearly documented solution for a common
> > problem for those who do not know how to solve this problem right away.
> > Currently, those who know what visual-line-mode is do not need this
> > hook, because they can easily write their own, and those who do not know
> > what visual-line-mode is can not use this hook, because it says nothing
> > about why it is actually useful.
> > 
> > Also, in addition to better docs, I would rename
> > `notmuch-show-turn-on-visual-line-mode' to something that reflects what
> > it does from user POV (like the other two hooks).
> > 
> > Though, the fact that the hook is enabled by default makes the above
> > arguments less important, I guess.
> 
> I have a bunch of somewhat conflicting thoughts about this:
>   - Being able to configure the behaviour without having to change the
>     core code is good, so implementing behaviour using hook functions is
>     useful.
>   - Things should behave well in the default configuration, so most of
>     the hook functions are enabled by default.
>   - Everything can't be hook functions, so there's a balance to be made
>     between implementing things as in-line code and via hook functions.
>   - Most users shouldn't need to modify any of the hooks.
>   - Documentation that explains what a hook function is about is
>     obviously good.
>   - Documenting something that is external to notmuch can be both
>     wasteful and risky.
> 
>     Wasteful because such documentation typically already exists and
>     risky because the precise behaviour of external components is not
>     under our control.
> 
>     For example, the documentation for `visual-line-mode' is both
>     concise and good, so there's little point in repeating it and, of
>     course, the exact details of what `visual-line-mode' does can be
>     changed by the Emacs developers. One would expect that they would
>     update the documentation for `visual-line-mode' in such situations,
>     which would leave any cloned documentation in an incorrect state.
> 
> Hence, I would probably take issue with your statement:
> 
> > I would argue that since the hook itself is trivial, the main point in
> > having it is to provide a clearly documented solution for a common
> > problem for those who do not know how to solve this problem right
> > away.
> 
> That's not the intention of the hook functions under discussion
> here. They are hook functions so that a curious and interested user can
> make a change based on some aesthetic preference. The are not about
> solving problems with the default configuration.
> 
> I think that `turn-on-visual-line-mode' (or the package local derivative
> of it that was chosen) is precisely the right name for a function that
> enables `visual-line-mode'. It describes perfectly and succinctly what
> the function does and provides a pointer to follow to find out more (the
> documentation for `visual-line-mode') without a user even having to
> examine the documentation of the function itself.

Agree completely.

Concrete suggestion:

+(defun notmuch-show-turn-on-visual-line-mode ()
+  "Enable `visual-line-mode'."
+  (visual-line-mode t))

To provide a link to the visual-line-mode documentation.

> All of that said, I'd agree that the documentation of
> `notmuch-show-indent-continuations' could have been better. How about:
>   "Indent any continuation lines in the current headers."
> ?

Sounds good.


BR,
Jani.

Re: [PATCH 0/2] re-enable line wrapping and add some header bling

[Jani Nikula]

On Thu, 26 Jan 2012 08:17:49 +0000, David Edmondson <dme@dme.org> wrote:
> By default, re-enable `visual-line-mode' in `notmuch-show-mode'. Do it
> via a hook so that purists (ahem) can turn it off.
> 
> Add some more processing of headers to make them look nice. Do it via
> hooks so that unbelievers can turn it off.

Tried them all, I like them all. I think it's good to have them enabled
by default.

Had to play with the options a bit to realize what the difference
between notmuch-show-fill-headers and
notmuch-show-turn-on-visual-line-mode is, especially when it comes to
headers and the "title line" for each message, and why I'd want to have
them both enabled (I do). So perhaps you could improve the documentation
as Dmitry suggested.

Thanks for these patches.

BR,
Jani.

> 
> David Edmondson (2):
>   emacs: Re-enable line wrapping in `notmuch-show-mode'.
>   emacs: Add more processing of displayed headers.
> 
>  emacs/notmuch-show.el |   50 +++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 42 insertions(+), 8 deletions(-)
> 
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v2] Wrap and indent headers in show mode

[David Edmondson]

v2:
- Simple rebase.

David Edmondson (1):
  emacs: Add more processing of displayed headers.

 emacs/notmuch-show.el |   43 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 7 deletions(-)

-- 
1.7.8.3

[PATCH v2] emacs: Add more processing of displayed headers.

[David Edmondson]

Wrap headers to the width of the window and indent continuations.
---
 emacs/notmuch-show.el |   43 ++++++++++++++++++++++++++++++++++++-------
 1 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 7469e2e..a589d37 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -68,9 +68,16 @@ any given message."
   :type 'boolean
   :group 'notmuch-show)
 
-(defvar notmuch-show-markup-headers-hook '(notmuch-show-colour-headers)
+(defcustom notmuch-show-markup-headers-hook '(notmuch-show-colour-headers
+					      notmuch-show-fill-headers
+					      notmuch-show-indent-continuations)
   "A list of functions called to decorate the headers listed in
-`notmuch-message-headers'.")
+`notmuch-message-headers'."
+  :type 'hook
+  :options '(notmuch-show-colour-headers
+	     notmuch-show-fill-headers
+	     notmuch-show-indent-continuations)
+  :group 'notmuch-show)
 
 (defcustom notmuch-show-hook '(notmuch-show-turn-on-visual-line-mode)
   "Functions called after populating a `notmuch-show' buffer."
@@ -269,13 +276,35 @@ operation on the contents of the current buffer."
     (overlay-put (make-overlay (point) (re-search-forward ".*$"))
 		 'face face)))
 
-(defun notmuch-show-colour-headers ()
+(defun notmuch-show-colour-headers (depth)
   "Apply some colouring to the current headers."
   (goto-char (point-min))
   (while (looking-at "^[A-Za-z][-A-Za-z0-9]*:")
     (notmuch-show-fontify-header)
     (forward-line)))
 
+(defun notmuch-show-fill-headers (depth)
+  "Wrap the text of the current headers."
+
+  ;; '-5' to allow for the indentation code.
+  (let ((fill-column (- (window-width) depth 5)))
+    (goto-char (point-min))
+    (while (not (eobp))
+      (let ((start (point)))
+	(end-of-line)
+	;; We're left at the start of the next line, so there's no need
+	;; to move forward after filling.
+	(fill-region-as-paragraph start (point))))))
+
+(defun notmuch-show-indent-continuations (depth)
+  "Indent any continuation lines."
+  (goto-char (point-min))
+  (while (not (eobp))
+    (if (not (looking-at "^[A-Za-z][-A-Za-z0-9]*:"))
+	;; Four spaces tends to work well with 'To' and 'Cc' headers.
+	(insert "    "))
+    (forward-line)))
+
 (defun notmuch-show-spaces-n (n)
   "Return a string comprised of `n' spaces."
   (make-string n ? ))
@@ -366,7 +395,7 @@ message at DEPTH in the current thread."
   "Insert a single header."
   (insert header ": " header-value "\n"))
 
-(defun notmuch-show-insert-headers (headers)
+(defun notmuch-show-insert-headers (headers depth)
   "Insert the headers of the current message."
   (let ((start (point)))
     (mapc (lambda (header)
@@ -379,7 +408,7 @@ message at DEPTH in the current thread."
     (save-excursion
       (save-restriction
 	(narrow-to-region start (point-max))
-	(run-hooks 'notmuch-show-markup-headers-hook)))))
+	(run-hook-with-args 'notmuch-show-markup-headers-hook depth)))))
 
 (define-button-type 'notmuch-show-part-button-type
   'action 'notmuch-show-part-button-default
@@ -671,7 +700,7 @@ current buffer, if possible."
     ;; Override `notmuch-message-headers' to force `From' to be
     ;; displayed.
     (let ((notmuch-message-headers '("From" "Subject" "To" "Cc" "Date")))
-      (notmuch-show-insert-headers (plist-get message :headers)))
+      (notmuch-show-insert-headers (plist-get message :headers) 0))
 
     ;; Blank line after headers to be compatible with the normal
     ;; message display.
@@ -864,7 +893,7 @@ current buffer, if possible."
     ;; Set `headers-start' to point after the 'Subject:' header to be
     ;; compatible with the existing implementation. This just sets it
     ;; to after the first header.
-    (notmuch-show-insert-headers headers)
+    (notmuch-show-insert-headers headers depth)
     (save-excursion
       (goto-char content-start)
       ;; If the subject of this message is the same as that of the
-- 
1.7.8.3