[RFC] reordering and cleanup of thread authors

[RFC] reordering and cleanup of thread authors

[Dirk Hohndel]

This is based in part on some discussion on IRC today.
When a thread is displayed in the search results, previously the authors
were always displayed in chronological order. But if only part of the
thread matches the query, that may or may not be the intuitive thing to
do.
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, 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).

The second cleanup that I've done in this context is to convert all
author names into the much easier to parse "Firstname Lastname"
format. Some broken mail systems (cough - Exchange - Cough) seem to
prefer "Lastname, Firstname" - which makes the comma separated list of
authors really hard to parse at one glance. Worse, it creates douplicate
entries if a person happens to show in both orders. So this does a
rather simplistic "look for a comma, if it's there, reorder" approach to
cleaning up the author name.

I don't think this is ready to be pulled. There was a strong request on
IRC to make the re-ordering of the author names configurable. I think
the cleanup of the names (First Last) should be configurable as well.
And of course I wonder about my implementation, given that I'm still
trying to wrap my mind around the current code base.

Thanks

/D

diff --git a/lib/message.cc b/lib/message.cc
index 721c9a6..ac0afd9 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,20 @@ 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)
+{
+    message->author = talloc_strdup(message, author);
+    return;
+}
+
 void
 _notmuch_message_set_date (notmuch_message_t *message,
 			   const char *date)
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 88da078..79638bb 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -771,6 +771,17 @@ notmuch_message_set_flag (notmuch_message_t *message,
 time_t
 notmuch_message_get_date  (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);
+
 /* Get the value of the specified header from 'message'.
  *
  * The value will be read from the actual message file, not from the
diff --git a/lib/thread.cc b/lib/thread.cc
index 48c070e..c3c83a3 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;
@@ -69,8 +70,95 @@ _thread_add_author (notmuch_thread_t *thread,
 	thread->authors = talloc_asprintf (thread, "%s, %s",
 					   thread->authors,
 					   author);
-    else
+    else {
 	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
+ * oldest first order within their group
+ */
+static void
+_thread_move_matched_author (notmuch_thread_t *thread,
+			     const char *author)
+{
+    char *authorscopy;
+    char *currentauthor;
+    int idx,nmstart,author_len,authors_len;
+
+    if (thread->authors == 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;
+    }
+    currentauthor = strcasestr(thread->authors, author);
+    if (currentauthor == NULL)
+	return;
+    idx = currentauthor - thread->nonmatched_authors;
+    if (idx < 0) {
+	/* already among matched authors */
+	return;
+    }
+    if (thread->nonmatched_authors + author_len < thread->authors + authors_len) {
+	/* we have to make changes, so let's get a temp copy */
+	authorscopy = strdup(thread->authors);
+	if (authorscopy == NULL)
+	    return;
+	/* nmstart is the offset into where the non-matched authors start */
+	nmstart = 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, authorscopy+nmstart, idx-2);
+	  /* finally, if there are authors AFTER our author, copy those */
+	  if(author_len+nmstart+idx < authors_len) {
+	    strncpy(thread->nonmatched_authors + idx - 2,", ",2);
+	    strncpy(thread->nonmatched_authors + idx, authorscopy+nmstart + idx + author_len + 2, 
+		    authors_len - (nmstart + idx + author_len + 2));
+	  }
+	}
+	free(authorscopy);
+    } else {
+	thread->nonmatched_authors += author_len;
+    }
+    return;
+}
+
+/* clean up the uggly "Lastname, Firstname" format to be "Firstname Lastname" 
+ */
+char *
+_thread_cleanup_author (notmuch_thread_t *thread,
+			const char *author)
+{
+  char *cleanauthor;
+  const char *comma;
+  int fname,lname;
+
+  cleanauthor = talloc_strdup(thread, author);
+  if (cleanauthor == NULL)
+    return NULL;
+  comma = strchr(author,',');
+  if (comma) {
+    lname = comma - author;
+    fname = strlen(author) - lname - 2;
+    strncpy(cleanauthor, comma + 2, fname);
+    *(cleanauthor+fname) = ' ';
+    strncpy(cleanauthor + fname + 1, author, lname);
+    *(cleanauthor+fname+1+lname) = '\0';
+  }
+  return cleanauthor;
 }
 
 /* Add 'message' as a message that belongs to 'thread'.
@@ -87,6 +175,7 @@ _thread_add_message (notmuch_thread_t *thread,
     InternetAddressList *list;
     InternetAddress *address;
     const char *from, *author;
+    char *cleanauthor;
 
     _notmuch_message_list_add_message (thread->message_list,
 				       talloc_steal (thread, message));
@@ -107,7 +196,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);
+	    cleanauthor = _thread_cleanup_author (thread, author);
+	    _thread_add_author (thread, cleanauthor );
+	    notmuch_message_set_author (message, cleanauthor);
 	}
 	g_object_unref (G_OBJECT (list));
     }
@@ -150,6 +241,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));
 }
 
 static void
@@ -251,6 +343,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);
 


-- 
Dirk Hohndel
Intel Open Source Technology Center

Re: [RFC] reordering and cleanup of thread authors

[Michal Sojka]

On Wed, 07 Apr 2010, Dirk Hohndel wrote:
> 
> This is based in part on some discussion on IRC today.
> When a thread is displayed in the search results, previously the authors
> were always displayed in chronological order. But if only part of the
> thread matches the query, that may or may not be the intuitive thing to
> do.

Hi Dirk,

thanks for the patch. It is a very interesting feature. See my comments
below.

>
> [...]
>
> +/*
> + * move authors of matched messages in the thread to 
> + * the front of the authors list, but keep them in
> + * oldest first order within their group
> + */
> +static void
> +_thread_move_matched_author (notmuch_thread_t *thread,
> +			     const char *author)
> +{
> +    char *authorscopy;
> +    char *currentauthor;
> +    int idx,nmstart,author_len,authors_len;
> +
> +    if (thread->authors == 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;
> +    }
> +    currentauthor = strcasestr(thread->authors, author);
> +    if (currentauthor == NULL)
> +	return;
> +    idx = currentauthor - thread->nonmatched_authors;
> +    if (idx < 0) {
> +	/* already among matched authors */
> +	return;
> +    }
> +    if (thread->nonmatched_authors + author_len < thread->authors + authors_len) {

What does the above condition exactly mean? I was not able to decode it
and it seems to be wrong. I expect that the "|" is used to delimit
matched and non-matched authors. If I run notmuch search
thread:XXXXXXXXXXXXXXXX, which matches all messages in the thread, I see
all authors delimited by "|", which I consider wrong.

-Michal

> +	/* we have to make changes, so let's get a temp copy */
> +	authorscopy = strdup(thread->authors);
> +	if (authorscopy == NULL)
> +	    return;
> +	/* nmstart is the offset into where the non-matched authors start */
> +	nmstart = 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, authorscopy+nmstart, idx-2);
> +	  /* finally, if there are authors AFTER our author, copy those */
> +	  if(author_len+nmstart+idx < authors_len) {
> +	    strncpy(thread->nonmatched_authors + idx - 2,", ",2);
> +	    strncpy(thread->nonmatched_authors + idx, authorscopy+nmstart + idx + author_len + 2, 
> +		    authors_len - (nmstart + idx + author_len + 2));
> +	  }
> +	}
> +	free(authorscopy);
> +    } else {
> +	thread->nonmatched_authors += author_len;
> +    }
> +    return;
> +}

Re: [RFC] reordering and cleanup of thread authors

[Dirk Hohndel]

On Fri, 09 Apr 2010 08:07:27 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> On Wed, 07 Apr 2010, Dirk Hohndel wrote:
> > 
> > This is based in part on some discussion on IRC today.
> > When a thread is displayed in the search results, previously the authors
> > were always displayed in chronological order. But if only part of the
> > thread matches the query, that may or may not be the intuitive thing to
> > do.
> 
> thanks for the patch. It is a very interesting feature.

Thanks - I've been using it for a few days now and am fairly happy with
it.

> >
> > +/*
> > + * move authors of matched messages in the thread to 
> > + * the front of the authors list, but keep them in
> > + * oldest first order within their group
> > + */
> > +static void
> > +_thread_move_matched_author (notmuch_thread_t *thread,
> > +			     const char *author)
> > +{
> > +    char *authorscopy;
> > +    char *currentauthor;
> > +    int idx,nmstart,author_len,authors_len;
> > +
> > +    if (thread->authors == 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;
> > +    }
> > +    currentauthor = strcasestr(thread->authors, author);
> > +    if (currentauthor == NULL)
> > +	return;
> > +    idx = currentauthor - thread->nonmatched_authors;
> > +    if (idx < 0) {
> > +	/* already among matched authors */
> > +	return;
> > +    }
> > +    if (thread->nonmatched_authors + author_len < thread->authors + authors_len) {
> 
> What does the above condition exactly mean? 

Eh - it's ugly. 

thread->authors + authors_len points to the trailing '\0' of the list of
all my authors. At this point in the code we know that the current
position of this author is at or after nonmatched_authors. So if
nonmatched_authors + author_len is less than the end of the all authors
that means that there was another author in the list behind this one -
and we need to move things around. 

In the else clause we just move the pointer to nonmatched_authors to the
end of this author.

So yeah, ugly, should be rewritten to make it easier to understand (or
at least commented better).

> I was not able to decode it
> and it seems to be wrong. I expect that the "|" is used to delimit
> matched and non-matched authors. If I run notmuch search
> thread:XXXXXXXXXXXXXXXX, which matches all messages in the thread, I see
> all authors delimited by "|", which I consider wrong.

Why do you think it's wrong? It's consistent. The thing that I actually
DISlike about the current solution is that the last author has no
delimiter (since there's no trailing ',' in the list and I didn't want
to realloc the space for the authors string). So you can't tell with one
glance if all authors match or all but the last one match. I haven't
come up with a better visual way to indicate this... 

Suggestions?

/D

-- 
Dirk Hohndel
Intel Open Source Technology Center

Re: [RFC] reordering and cleanup of thread authors

[Michal Sojka]

On Fri, 09 Apr 2010, Dirk Hohndel wrote:
> On Fri, 09 Apr 2010 08:07:27 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > On Wed, 07 Apr 2010, Dirk Hohndel wrote:
> > > 
> > > This is based in part on some discussion on IRC today.
> > > When a thread is displayed in the search results, previously the authors
> > > were always displayed in chronological order. But if only part of the
> > > thread matches the query, that may or may not be the intuitive thing to
> > > do.
> > 
> > thanks for the patch. It is a very interesting feature.
> 
> Thanks - I've been using it for a few days now and am fairly happy with
> it.
> 
> > >
> > > +/*
> > > + * move authors of matched messages in the thread to 
> > > + * the front of the authors list, but keep them in
> > > + * oldest first order within their group
> > > + */
> > > +static void
> > > +_thread_move_matched_author (notmuch_thread_t *thread,
> > > +			     const char *author)
> > > +{
> > > +    char *authorscopy;
> > > +    char *currentauthor;
> > > +    int idx,nmstart,author_len,authors_len;
> > > +
> > > +    if (thread->authors == 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;
> > > +    }
> > > +    currentauthor = strcasestr(thread->authors, author);
> > > +    if (currentauthor == NULL)
> > > +	return;
> > > +    idx = currentauthor - thread->nonmatched_authors;
> > > +    if (idx < 0) {
> > > +	/* already among matched authors */
> > > +	return;
> > > +    }
> > > +    if (thread->nonmatched_authors + author_len < thread->authors + authors_len) {
> > 
> > What does the above condition exactly mean? 
> 
> Eh - it's ugly. 
> 
> thread->authors + authors_len points to the trailing '\0' of the list of
> all my authors. At this point in the code we know that the current
> position of this author is at or after nonmatched_authors. So if
> nonmatched_authors + author_len is less than the end of the all authors
> that means that there was another author in the list behind this one -
> and we need to move things around. 
> 
> In the else clause we just move the pointer to nonmatched_authors to the
> end of this author.
> 
> So yeah, ugly, should be rewritten to make it easier to understand (or
> at least commented better).
> 
> > I was not able to decode it
> > and it seems to be wrong. I expect that the "|" is used to delimit
> > matched and non-matched authors. If I run notmuch search
> > thread:XXXXXXXXXXXXXXXX, which matches all messages in the thread, I see
> > all authors delimited by "|", which I consider wrong.
> 
> Why do you think it's wrong? 

Because I thought, that | was used as a delimiter between the two parts
of the list and not as a marker of matched authors.

> It's consistent. The thing that I actually DISlike about the current
> solution is that the last author has no delimiter (since there's no
> trailing ',' in the list and I didn't want to realloc the space for
> the authors string). So you can't tell with one glance if all authors
> match or all but the last one match. I haven't come up with a better
> visual way to indicate this...

I think that using | as a separator would help here. Let's say that
initially we have "Matched Author, Non Matched, Matched Again" we can
tranform this to  "Matched Author, Matched Again| Non Matched". This way,
the length of the string remains the same. If there is no | after
transformation, we know that all authors matched because there is always
at least one mathed author in the search results.

-Michal

Re: [RFC] reordering and cleanup of thread authors

[Dirk Hohndel]

On Sat, 10 Apr 2010 03:53:59 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> I think that using | as a separator would help here. Let's say that
> initially we have "Matched Author, Non Matched, Matched Again" we can
> tranform this to  "Matched Author, Matched Again| Non Matched". This way,
> the length of the string remains the same. If there is no | after
> transformation, we know that all authors matched because there is always
> at least one mathed author in the search results.

That's a great idea. I'll update the patch to do that.

/D

-- 
Dirk Hohndel
Intel Open Source Technology Center

Re: [RFC] reordering and cleanup of thread authors

[Dirk Hohndel]

On Fri, 09 Apr 2010 19:42:49 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> On Sat, 10 Apr 2010 03:53:59 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > I think that using | as a separator would help here. Let's say that
> > initially we have "Matched Author, Non Matched, Matched Again" we can
> > tranform this to  "Matched Author, Matched Again| Non Matched". This way,
> > the length of the string remains the same. If there is no | after
> > transformation, we know that all authors matched because there is always
> > at least one mathed author in the search results.
> 
> That's a great idea. I'll update the patch to do that.

Since Carl just prompted me, I wrote an updated patch (will post in a
separate message replying to this one). 

I made the change suggested by Michal, fixed a bug or two and removed
the part of the patch that was trying to cleanup author names in "Last,
First" format - on IRC it was pointed out to me that I was overlooking
another use of the ',' in email addresses: accounts that are shared by
multiple people. And it makes no sense to reorder email addresses of the
form "Wife, Husband and child" <familyaccount@add.res>

I haven't given up on this, though. Since Exchange and Outlook have this
nasty habit of creating these "Last, First" <first.last@company.com> or
"Last, First MI" <first.mi.last@company.com> from headers, I really want
to add the option to clean those up. So I'll submit a separate patch
that checks if we have exactly one of these two pattern shown here - and
that then reorders things

/D

-- 
Dirk Hohndel
Intel Open Source Technology Center

[PATCH] Reordering of thread authors to list matching authors first

[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/message.cc |   16 ++++++++++++
 lib/notmuch.h  |   11 ++++++++
 lib/thread.cc  |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 0 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 721c9a6..ac0afd9 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,20 @@ 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)
+{
+    message->author = talloc_strdup(message, author);
+    return;
+}
+
 void
 _notmuch_message_set_date (notmuch_message_t *message,
 			   const char *date)
diff --git a/lib/notmuch.h b/lib/notmuch.h
index bae48a6..769f747 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -773,6 +773,17 @@ notmuch_message_set_flag (notmuch_message_t *message,
 time_t
 notmuch_message_get_date  (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);
+
 /* Get the value of the specified header from 'message'.
  *
  * The value will be read from the actual message file, not from the
diff --git a/lib/thread.cc b/lib/thread.cc
index e514bf8..baa0d7f 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,76 @@ _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 *authorscopy;
+    char *currentauthor;
+    char *lastpipe,*nextpipe;
+    int idx,nmstart,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;
+    }
+    currentauthor = strcasestr(thread->authors, author);
+    if (currentauthor == NULL)
+	return;
+    /* how far inside the nonmatched authors is our author? */
+    idx = currentauthor - 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 */
+	authorscopy = xstrdup(thread->authors);
+	/* nmstart is the offset into where the non-matched authors start */
+	nmstart = 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, authorscopy+nmstart, idx-2);
+	  /* finally, if there are authors AFTER our author, copy those */
+	  if(author_len+nmstart+idx < authors_len) {
+	    strncpy(thread->nonmatched_authors + idx - 2,", ",2);
+	    strncpy(thread->nonmatched_authors + idx, authorscopy+nmstart + idx + author_len + 2, 
+		    authors_len - (nmstart + idx + author_len + 2));
+	  }
+	}
+	free(authorscopy);
+	/* finally let's make sure there's just one '|' in the authors string */
+	lastpipe = strchr(thread->authors,'|');
+	while (lastpipe) {
+	    nextpipe = strchr(lastpipe+1,'|');
+	    if (nextpipe)
+		*lastpipe = ',';
+	    lastpipe = nextpipe;
+	}
+    } else {
+	thread->nonmatched_authors += author_len;
+    }
+    return;
+}
+
 /* Add 'message' as a message that belongs to 'thread'.
  *
  * The 'thread' will talloc_steal the 'message' and hold onto a
@@ -108,6 +179,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));
     }
@@ -162,6 +234,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));
 }
 
 static void
@@ -283,6 +356,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


-- 
Dirk Hohndel
Intel Open Source Technology Center

Re: [PATCH] Reordering of thread authors to list matching authors first

[Carl Worth]

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

On Wed, 21 Apr 2010 20:58:27 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> 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 ','

It seems a reasonable feature to me.

Some notes on the patch:

> +void
> +notmuch_message_set_author (notmuch_message_t *message,
> +			    const char *author)
> +{
> +    message->author = talloc_strdup(message, author);
> +    return;
> +}

This is leaking any previously set author value, (admittedly, it's only
a "talloc leak" so it will still get cleaned up when the message is
cleaned up, but still.

> +/* 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);

The notmuch.h file is our publicly installed header file for the library
interface. I don't think the feature here requires any new library
interface. Even if it did, we wouldn't want a public function like
set_author that could simply scramble internal state and change the
result of future calls to get_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)

The implementation here seems a bit fiddly.

We already have two passes over the messages, (first all messages, and
then all matched messages). And we're currently calling add_author from
the first pass.

How about simply calling a new add_matched_author from the second pass
which would look very much like the existing add_author. Then we could
change add_author to accumulate authors into an array rather than a
string. Then, finally, we would append any of these authors not already
in the matched_authors hash tabled onto the final string.

That should be less code and easier to understand I think.

I can take a whack at that later if you don't beat me to it.

-Carl

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

Re: [PATCH] Reordering of thread authors to list matching authors first

[Dirk Hohndel]

On Fri, 23 Apr 2010 17:21:53 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Wed, 21 Apr 2010 20:58:27 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> > 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 ','
> 
> It seems a reasonable feature to me.

I switched back to origin/master to help get ready for 0.3 and
tremendously miss this feature :-) 
 
> Some notes on the patch:
> 
> > +void
> > +notmuch_message_set_author (notmuch_message_t *message,
> > +			    const char *author)
> > +{
> > +    message->author = talloc_strdup(message, author);
> > +    return;
> > +}
> 
> This is leaking any previously set author value, (admittedly, it's only
> a "talloc leak" so it will still get cleaned up when the message is
> cleaned up, but still.

Fixed in forthcoming revision of this patch

> 
> > +/* 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);
> 
> The notmuch.h file is our publicly installed header file for the library
> interface. I don't think the feature here requires any new library
> interface. Even if it did, we wouldn't want a public function like
> set_author that could simply scramble internal state and change the
> result of future calls to get_author.

My mistake - moved them to notmuch-private.h

> > +/*
> > + * 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)
> 
> The implementation here seems a bit fiddly.
> 
> We already have two passes over the messages, (first all messages, and
> then all matched messages). And we're currently calling add_author from
> the first pass.
> 
> How about simply calling a new add_matched_author from the second pass
> which would look very much like the existing add_author. Then we could
> change add_author to accumulate authors into an array rather than a
> string. Then, finally, we would append any of these authors not already
> in the matched_authors hash tabled onto the final string.
> 
> That should be less code and easier to understand I think.
> 
> I can take a whack at that later if you don't beat me to it.

Maybe I'm misunderstanding your proposed algorithm - but it seems quite
a bit more complicated than what I'm doing today...

/D