new patch series for author reordering code

new patch series for author reordering code

[Dirk Hohndel]

I tried to break this out into logically independent pieces - but to connect
this as a series.  
First we add the authors member and accessors to message
Second the reordering of thread authors (still the original string based
algorithm that I used before - I couldn't quite make sense of cworth's proposed
algorithm
Third a NEWS blurb
Fourth test for the new functionality
Fifth a first simple routine to make author names easier to read - this one
undoes the typical Exchange ugliness

I think this could go into 0.3 as is. I've been using the mostly identical
previous version for about a week - the changes here are mostly cleanup based
on cworth's feedback.

I am planning to enhance the "display author names in a friendlier way" feature
in the future, capturing more cases.

/D

[PATCH 1/5] Add authors member to message

[Dirk Hohndel]

message->authors contains the author's name (as we want to print it)
get / set methods are declared in notmuch-private.h

Signed-off-by: Dirk Hohndel <hohndel@infradead.org>
---
 lib/message.cc        |   18 ++++++++++++++++++
 lib/notmuch-private.h |   10 ++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 721c9a6..4b2f98f 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -35,6 +35,7 @@ struct _notmuch_message {
     char *thread_id;
     char *in_reply_to;
     char *filename;
+    char *author;
     notmuch_message_file_t *message_file;
     notmuch_message_list_t *replies;
     unsigned long flags;
@@ -110,6 +111,7 @@ _notmuch_message_create (const void *talloc_owner,
     message->in_reply_to = NULL;
     message->filename = NULL;
     message->message_file = NULL;
+    message->author = NULL;
 
     message->replies = _notmuch_message_list_create (message);
     if (unlikely (message->replies == NULL)) {
@@ -533,6 +535,22 @@ notmuch_message_get_tags (notmuch_message_t *message)
     return _notmuch_convert_tags(message, i, end);
 }
 
+const char *
+notmuch_message_get_author (notmuch_message_t *message)
+{
+    return message->author;
+}
+
+void
+notmuch_message_set_author (notmuch_message_t *message,
+			    const char *author)
+{
+    if (message->author)
+	talloc_free(message->author);
+    message->author = talloc_strdup(message, author);
+    return;
+}
+
 void
 _notmuch_message_set_date (notmuch_message_t *message,
 			   const char *date)
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 94cce1b..6e83cc3 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -275,6 +275,16 @@ _notmuch_message_talloc_copy_data (notmuch_message_t *message);
 void
 _notmuch_message_clear_data (notmuch_message_t *message);
 
+/* Set the author member of 'message' - this is the representation used
+ * when displaying the message */
+void
+notmuch_message_set_author (notmuch_message_t *message, const char *author);
+
+/* Get the author member of 'message' */
+const char *
+notmuch_message_get_author (notmuch_message_t *message);
+
+
 /* index.cc */
 
 notmuch_status_t
-- 
1.6.6.1

[PATCH 2/5] Reorder displayed names of thread authors

[Dirk Hohndel]

When displaying threads as result of a search it makes sense to list those
authors first who match the search. The matching authors are separated from the
non-matching ones with a '|' instead of a ','

Imagine the default "+inbox" query. Those mails in the thread that
match the query are actually "new" (whatever that means). And some
people seem to think that it would be much better to see those author
names first. For example, imagine a long and drawn out thread that once
was started by me; you have long read the older part of the thread and
removed the inbox tag. Whenever a new email comes in on this thread,
prior to this patch the author column in the search display will first show
"Dirk Hohndel" - I think it should first show the actual author(s) of the new
mail(s).

Signed-off-by: Dirk Hohndel <hohndel@infradead.org>
---
 lib/thread.cc |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index e1da060..c80bb26 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -32,6 +32,7 @@ struct _notmuch_thread {
     char *subject;
     GHashTable *authors_hash;
     char *authors;
+    char *nonmatched_authors;
     GHashTable *tags;
 
     notmuch_message_list_t *message_list;
@@ -73,6 +74,79 @@ _thread_add_author (notmuch_thread_t *thread,
 	thread->authors = talloc_strdup (thread, author);
 }
 
+/*
+ * move authors of matched messages in the thread to
+ * the front of the authors list, but keep them in
+ * existing order within their group
+ */
+static void
+_thread_move_matched_author (notmuch_thread_t *thread,
+			     const char *author)
+{
+    char *authors_copy;
+    char *current_author;
+    char *last_pipe,*next_pipe;
+    int idx,nm_start,author_len,authors_len;
+
+    if (thread->authors == NULL || author == NULL)
+	return;
+    if (thread->nonmatched_authors == NULL)
+	thread->nonmatched_authors = thread->authors;
+    author_len = strlen(author);
+    authors_len = strlen(thread->authors);
+    if (author_len == authors_len) {
+	/* just one author */
+	thread->nonmatched_authors += author_len;
+	return;
+    }
+    current_author = strcasestr(thread->authors, author);
+    if (current_author == NULL)
+	return;
+    /* how far inside the nonmatched authors is our author? */
+    idx = current_author - thread->nonmatched_authors;
+    if (idx < 0) {
+	/* already among matched authors */
+	return;
+    }
+    /* are there any authors in the list after our author? */
+    if (thread->nonmatched_authors + author_len < thread->authors + authors_len) {
+	/* we have to make changes, so let's get a temp copy */
+	authors_copy = talloc_strdup(thread,thread->authors);
+	/* nm_start is the offset into where the non-matched authors start */
+	nm_start = thread->nonmatched_authors - thread->authors;
+	/* copy this author and add the "| " - the if clause above tells us there's more */
+	strncpy(thread->nonmatched_authors,author,author_len);
+	strncpy(thread->nonmatched_authors+author_len,"| ",2);
+	thread->nonmatched_authors += author_len+2;
+	if (idx > 0) {
+	  /* we are actually moving authors around, not just changing the separator
+	   * first copy the authors that came BEFORE our author */
+	  strncpy(thread->nonmatched_authors, authors_copy+nm_start, idx-2);
+	  /* finally, if there are authors AFTER our author, copy those */
+	  if(author_len+nm_start+idx < authors_len) {
+	    strncpy(thread->nonmatched_authors + idx - 2,", ",2);
+	    strncpy(thread->nonmatched_authors + idx, authors_copy+nm_start + idx + author_len + 2,
+		    authors_len - (nm_start + idx + author_len + 2));
+	  }
+	}
+	/* finally let's make sure there's just one '|' in the authors string */
+	last_pipe = strchr(thread->authors,'|');
+	while (last_pipe) {
+	    next_pipe = strchr(last_pipe+1,'|');
+	    if (next_pipe)
+		*last_pipe = ',';
+	    last_pipe = next_pipe;
+	}
+    } else {
+	thread->nonmatched_authors += author_len;
+	/* so now all authors are matched - let's remove the '|' */
+	last_pipe = strchr(thread->authors,'|');
+	if (last_pipe)
+	    *last_pipe = ',';
+    }
+    return;
+}
+
 /* Add 'message' as a message that belongs to 'thread'.
  *
  * The 'thread' will talloc_steal the 'message' and hold onto a
@@ -110,6 +184,7 @@ _thread_add_message (notmuch_thread_t *thread,
 		author = internet_address_mailbox_get_addr (mailbox);
 	    }
 	    _thread_add_author (thread, author);
+	    notmuch_message_set_author (message, author);
 	}
 	g_object_unref (G_OBJECT (list));
     }
@@ -182,6 +257,7 @@ _thread_add_matched_message (notmuch_thread_t *thread,
 	notmuch_message_set_flag (hashed_message,
 				  NOTMUCH_MESSAGE_FLAG_MATCH, 1);
     }
+    _thread_move_matched_author (thread,notmuch_message_get_author(hashed_message));
 
     if ((sort == NOTMUCH_SORT_OLDEST_FIRST && date <= thread->newest) ||
 	(sort != NOTMUCH_SORT_OLDEST_FIRST && date == thread->newest))
@@ -309,6 +385,7 @@ _notmuch_thread_create (void *ctx,
     thread->authors_hash = g_hash_table_new_full (g_str_hash, g_str_equal,
 						  free, NULL);
     thread->authors = NULL;
+    thread->nonmatched_authors = NULL;
     thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal,
 					  free, NULL);
 
-- 
1.6.6.1

[PATCH 3/5] Add NEWS section for author reordering

[Dirk Hohndel]

This should be required in all patches

Signed-off-by: Dirk Hohndel <hohndel@infradead.org>
---
 NEWS |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/NEWS b/NEWS
index eba0fd5..c2057c2 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,13 @@
+
+Visualization of author names that match a search
+
+  When notmuch displays threads as the result of a search, it now
+  lists the authors that match the search before listing the other
+  authors in the thread. It inserts a pipe '|' symbol between the last
+  matching and first non-matching author. This is especially useful in
+  a search that includes tag:unread. Now the authors of the unread
+  messages in the thread are listed first.
+
 Notmuch 0.2 (2010-04-16)
 ========================
 This is the second release of the notmuch mail system, with actual
-- 
1.6.6.1

[PATCH 4/5] Add tests for author name reordering in search results

[Dirk Hohndel]

This should be required for all patches :-)

Signed-off-by: Dirk Hohndel <hohndel@infradead.org>
---
 test/notmuch-test |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/test/notmuch-test b/test/notmuch-test
index 7082344..f455232 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -867,6 +867,33 @@ printf " Searching returns all three messages in one thread..."
 output=$($NOTMUCH search foo | notmuch_search_sanitize)
 pass_if_equal "$output" "thread:XXX   2000-01-01 [3/3] Notmuch Test Suite; brokenthreadtest (inbox unread)"
 
+printf "\nTesting author reordering;\n"
+printf " Adding parent message...\t\t\t"
+generate_message [body]=findme [id]=new-parent-id [subject]=author-reorder-threadtest '[from]="User <user@example.com>"' '[date]="Sat, 01 Jan 2000 12:00:00 -0000"'
+output=$(NOTMUCH_NEW)
+pass_if_equal "$output" "Added 1 new message to the database."
+printf " Adding initial child message...\t\t"
+generate_message [body]=findme '[in-reply-to]=\<new-parent-id\>' [subject]=author-reorder-threadtest '[from]="User1 <user1@example.com>"' '[date]="Sat, 01 Jan 2000 12:00:00 -0000"'
+output=$(NOTMUCH_NEW)
+pass_if_equal "$output" "Added 1 new message to the database."
+printf " Adding second child message...\t\t\t"
+generate_message [body]=findme '[in-reply-to]=\<new-parent-id\>' [subject]=author-reorder-threadtest '[from]="User2 <user2@example.com>"' '[date]="Sat, 01 Jan 2000 12:00:00 -0000"'
+output=$(NOTMUCH_NEW)
+pass_if_equal "$output" "Added 1 new message to the database."
+printf " Searching when all three messages match...\t"
+output=$($NOTMUCH search findme | notmuch_search_sanitize)
+pass_if_equal "$output" "thread:XXX   2000-01-01 [3/3] User, User1, User2; author-reorder-threadtest (inbox unread)"
+printf " Searching when two messages match...\t\t"
+output=$($NOTMUCH search User1 or User2 | notmuch_search_sanitize)
+pass_if_equal "$output" "thread:XXX   2000-01-01 [2/3] User1, User2| User; author-reorder-threadtest (inbox unread)"
+printf " Searching when only one message matches...\t"
+output=$($NOTMUCH search User2 | notmuch_search_sanitize)
+pass_if_equal "$output" "thread:XXX   2000-01-01 [1/3] User2| User, User1; author-reorder-threadtest (inbox unread)"
+printf " Searching when only first message matches...\t"
+output=$($NOTMUCH search User | notmuch_search_sanitize)
+pass_if_equal "$output" "thread:XXX   2000-01-01 [1/3] User| User1, User2; author-reorder-threadtest (inbox unread)"
+
+printf "\nTesting From line heuristics;\n"
 printf " Magic from guessing (nothing to go on)...\t"
 add_message '[from]="Sender <sender@example.com>"' \
              [to]=mailinglist@notmuchmail.org \
@@ -965,7 +992,6 @@ References: <${gen_msg_id}>
 On Tue, 05 Jan 2010 15:43:56 -0800, Sender <sender@example.com> wrote:
 > from guessing test"
 
-
 echo ""
 echo "Notmuch test suite complete."
 
-- 
1.6.6.1

[PATCH 5/5] Simple attempt to display author names in a friendlier way

[Dirk Hohndel]

This patch only addresses the typical Outlook/Exchange case
where we have "Last, First" <first.last@company.com> or
"Last, First MI" <first.mi.last@company.com>.

In the future we should be more fexible as to the formats
we recognize, but for now we address this one as it is the
Exchange default setting and therefore the most common one.

Signed-off-by: Dirk Hohndel <hohndel@infradead.org>
---
 lib/thread.cc |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/lib/thread.cc b/lib/thread.cc
index c80bb26..b8be3e1 100644
--- a/lib/thread.cc
+++ b/lib/thread.cc
@@ -147,6 +147,51 @@ _thread_move_matched_author (notmuch_thread_t *thread,
     return;
 }
 
+/* clean up the uggly "Lastname, Firstname" format that some mail systems
+ * (most notably, Exchange) are creating to be "Firstname Lastname"
+ * To make sure that we don't change other potential situations where a
+ * comma is in the name, we check that we match one of these patterns
+ * "Last, First" <first.last@company.com>
+ * "Last, First MI" <first.mi.last@company.com>
+ */
+char *
+_thread_cleanup_author (notmuch_thread_t *thread,
+			const char *author, const char *from)
+{
+    char *clean_author,*test_author;
+    const char *comma;
+    char *blank;
+    int fname,lname;
+
+    clean_author = talloc_strdup(thread, author);
+    if (clean_author == NULL)
+	return NULL;
+    comma = strchr(author,',');
+    if (comma) {
+	/* let's assemble what we think is the correct name */
+	lname = comma - author;
+	fname = strlen(author) - lname - 2;
+	strncpy(clean_author, comma + 2, fname);
+	*(clean_author+fname) = ' ';
+	strncpy(clean_author + fname + 1, author, lname);
+	*(clean_author+fname+1+lname) = '\0';
+	/* make a temporary copy and see if it matches the email */
+	test_author = talloc_strdup(thread,clean_author);
+
+	blank=strchr(test_author,' ');
+	while (blank != NULL) {
+	    *blank = '.';
+	    blank=strchr(test_author,' ');
+	}
+	if (strcasestr(from, test_author) == NULL)
+	    /* we didn't identify this as part of the email address
+	    * so let's punt and return the original author */
+	    strcpy (clean_author, author);
+
+    }
+    return clean_author;
+}
+
 /* Add 'message' as a message that belongs to 'thread'.
  *
  * The 'thread' will talloc_steal the 'message' and hold onto a
@@ -161,6 +206,7 @@ _thread_add_message (notmuch_thread_t *thread,
     InternetAddressList *list = NULL;
     InternetAddress *address;
     const char *from, *author;
+    char *clean_author;
 
     _notmuch_message_list_add_message (thread->message_list,
 				       talloc_steal (thread, message));
@@ -183,8 +229,9 @@ _thread_add_message (notmuch_thread_t *thread,
 		mailbox = INTERNET_ADDRESS_MAILBOX (address);
 		author = internet_address_mailbox_get_addr (mailbox);
 	    }
-	    _thread_add_author (thread, author);
-	    notmuch_message_set_author (message, author);
+	    clean_author = _thread_cleanup_author (thread, author, from);
+	    _thread_add_author (thread, clean_author);
+	    notmuch_message_set_author (message, clean_author);
 	}
 	g_object_unref (G_OBJECT (list));
     }
-- 
1.6.6.1

Re: new patch series for author reordering code

[Carl Worth]

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

On Sat, 24 Apr 2010 11:20:52 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> I think this could go into 0.3 as is. I've been using the mostly identical
> previous version for about a week - the changes here are mostly cleanup based
> on cworth's feedback.

Thanks, Dirk.

This made a very nice patch series, (particularly with the test case).

I've pushed this now.

-Carl

-- 
carl.d.worth@intel.com

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