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