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: <m3zl1cfsb4.fsf@x200.gr8dns.org>
References: <m31veru7vn.fsf@x200.gr8dns.org> <87zl1d5fc0.fsf@steelpick.2x.cz>
	<m3zl1cfsb4.fsf@x200.gr8dns.org>
Date: Sat, 10 Apr 2010 03:53:59 +0200
Message-ID: <87aatcysw8.fsf@wsheee.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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