From: Carl Worth <cworth@cworth.org>
To: Dirk Hohndel <hohndel@infradead.org>, notmuch <notmuch@notmuchmail.org>
Subject: Re: [PATCH] Reordering of thread authors to list matching authors
	first
In-Reply-To: <m38w8gyw7w.fsf@x200.gr8dns.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>
User-Agent: Notmuch/0.2-164-g57926bc (http://notmuchmail.org) Emacs/23.1.1
	(i486-pc-linux-gnu)
Date: Fri, 23 Apr 2010 17:21:53 -0700
Message-ID: <87hbn1og2m.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

On Wed, 21 Apr 2010 20:58:27 -0700, Dirk Hohndel <hohndel@infradead.org> wr=
ote:
> 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 fr=
om 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 =3D 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 *auth=
or);
> +
> +/* 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=20
> + * 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.

=2DCarl

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iD8DBQFL0jmh6JDdNq8qSWgRAhb6AKCijZXXjFjK0PlhlhzTMeyMUKZL+ACdH/Df
jkwAfLTN/kl3TnNA19WmZTU=
=Fi+A
-----END PGP SIGNATURE-----
--=-=-=--