From: Dirk Hohndel <hohndel@infradead.org>
To: Carl Worth <cworth@cworth.org>, notmuch <notmuch@notmuchmail.org>
Subject: Re: [PATCH] Reordering of thread authors to list matching authors
	first
In-Reply-To: <87hbn1og2m.fsf@yoom.home.cworth.org>
References: <m31veru7vn.fsf@x200.gr8dns.org> <87zl1d5fc0.fsf@steelpick.2x.cz>
	<m3zl1cfsb4.fsf@x200.gr8dns.org>
	<87aatcysw8.fsf@wsheee.localdomain>
	<m3wrwg6n9y.fsf@x200.gr8dns.org> <m3aaswyw9t.fsf@x200.gr8dns.org>
	<m38w8gyw7w.fsf@x200.gr8dns.org>
	<87hbn1og2m.fsf@yoom.home.cworth.org>
User-Agent: Notmuch/0.2-187-g7c5f017 (http://notmuchmail.org) Emacs/23.1.1
	(i386-redhat-linux-gnu)
Date: Sat, 24 Apr 2010 10:35:05 -0700
Message-ID: <m3633g3gae.fsf@x200.gr8dns.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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