From: Michal Sojka <sojkam1@fel.cvut.cz>
To: Dirk Hohndel <hohndel@infradead.org>, notmuch <notmuch@notmuchmail.org>
Subject: Re: [RFC] reordering and cleanup of thread authors
In-Reply-To: <m31veru7vn.fsf@x200.gr8dns.org>
References: <m31veru7vn.fsf@x200.gr8dns.org>
Date: Fri, 09 Apr 2010 08:07:27 +0200
Message-ID: <87zl1d5fc0.fsf@steelpick.2x.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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;
> +}