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