[PATCH] emacs: Fix to unconditionally display subject changes in collapsed thread view

[PATCH] emacs: Fix to unconditionally display subject changes in collapsed thread view

[Carl Worth]

The feature to show subject changes in the collapsed thread view was
originally added (8ab433607) with an option
(notmuch-show-always-show-subject) to display the subject
for all messages, even when there was no change.

The subsequent commit (4f04d273) changed the sense of the test (or to
and) and the name of the controlling variable
(notmuch-show-elide-same-subject).

But this commit is broken in a few ways:

  1. The original definition of notmuch-show-always-show-subject was
     left around

     But the variable isn't actually used in the code at all, so it
     just adds clutter and confusion to the customization interface.

  2. The name and description of the controlling variable doesn't
     match the implementation

     The name suggests that setting the variable to t will cause
     repeated subjects to be elided, (suggesting that when it is nil
     all subjects will be shown).

     However, when the variable is nil, no subjects are shown. So a
     correct name for the variable in this sense would be
     notmuch-show-subject-changes.

Showing subject changes is a useful feature, and should be on by
default. (We don't want to bury generally useful features behind
customizations that users have to find).

Rather than fixing the name of the variable and changing its default
value, here we remove the condition entirely, such that the feature is
enabled unconditionally.

So both the currently-used variable and the stale definition of the
formerly-used are removed.

Also, the one relevant test-suite result is updated, (showing the
intial subject of a collapsed thread, and no subject display for later
messages that do not change the subject).
---
 emacs/notmuch-show.el                              |   16 ++--------------
 .../notmuch-show-thread-with-hidden-messages       |    1 +
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 6685717..f96743b 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -65,17 +65,6 @@ any given message."
   :group 'notmuch
   :type 'boolean)
 
-(defcustom notmuch-show-elide-same-subject nil
-  "Do not show the subject of a collapsed message if it is the
-same as that of the previous message."
-  :group 'notmuch
-  :type 'boolean)
-
-(defcustom notmuch-show-always-show-subject t
-  "Should a collapsed message show the `Subject:' line?"
-  :group 'notmuch
-  :type 'boolean)
-
 (defvar notmuch-show-markup-headers-hook '(notmuch-show-colour-headers)
   "A list of functions called to decorate the headers listed in
 `notmuch-message-headers'.")
@@ -727,9 +716,8 @@ current buffer, if possible."
       ;; If the subject of this message is the same as that of the
       ;; previous message, don't display it when this message is
       ;; collapsed.
-      (when (and notmuch-show-elide-same-subject
-		 (not (string= notmuch-show-previous-subject
-			       bare-subject)))
+      (when (not (string= notmuch-show-previous-subject
+			  bare-subject))
 	(forward-line 1))
       (setq headers-start (point-marker)))
     (setq headers-end (point-marker))
diff --git a/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages b/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages
index 5df6606..8a0660f 100644
--- a/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages
+++ b/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages
@@ -1,3 +1,4 @@
 Jan Janak <jan@ryngle.com> (2009-11-17) (inbox unread)
+Subject: [notmuch] What a great idea!
  Jan Janak <jan@ryngle.com> (2009-11-17) (inbox)
  Carl Worth <cworth@cworth.org> (2009-11-18) (inbox unread)
-- 
1.7.5.4

Re: [PATCH] emacs: Fix to unconditionally display subject changes in collapsed thread view

[Carl Worth]

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

On Fri,  1 Jul 2011 02:01:08 -0700, Carl Worth <cworth@cworth.org> wrote:
> Rather than fixing the name of the variable and changing its default
> value, here we remove the condition entirely, such that the feature is
> enabled unconditionally.

I've just pushed this change out.

I emailed it here just to be able to comment on it.

David, I implemented this fix while writing up the NEWS[*] notes for the
0.6 release (and noticing that this feature which was implemented wasn't
actually usable as documented so I couldn't really add a NEWS item for
it).

If you want to sneak it into 0.6 I think it's safe, (it even hits one
existing test case---though an additional test case to show an actual
subject change appearing would be even better).

It's your call of course, and if the change doesn't go, then that's fine
too. In that case, I'll just not comment on this feature in NEWS until
our next release.

-Carl

[*] Yes, I'm working away at this—almost done now...

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