* [PATCH 00/11] add recipients to search output @ 2012-08-20 1:52 Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins ` (3 more replies) 0 siblings, 4 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail This series is an attempt to add thread recipients to the search output. My personal overall goal of this series is to support the handling of drafts in the emacs ui. For drafts we want to see recipients, instead of authors, in the search output. I can imagine other uses for this series as well, though. The first four patches generalize the author list handling in thread objects to handle any address list. These patches could be applied regardless of if the rest of the series is accepted. After that we modify the thread constructor such that it can hold thread recipients as well. Since there is overhead in retrieving thread recipients from the message files (recipients are not stored in the database) this is handled with a switch. Further patches add the new switch to the search CLI that adds thread recipients to the structured output formats. I didn't modify the text output format, since there is no way to extend it. I can imagine tweaking the text output such that the author field is instead replaced by the recipients (as is done for the emacs UI at the end of the series), but that's not done here. In the emacs UI, I add a new toggle function that will toggle display of thread authors or recipients in the 'authors' field of the search output. It's unfortunate that this ambiguity in that field name remains, but I didn't know how to change that cleanly. I'm working on some tests for the new emacs functionality that I'll include in the inevitable v2 of this series. The last patch is mostly just a tickle to suggest adding the recipients to the database. It would make the --include-recipient searches much faster of course, but it might be overhead in the database that folks aren't interested in. As always, feedback, review, and comments are much appreciated. jamie. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 01/11] lib: new thread addresses structure 2012-08-20 1:52 [PATCH 00/11] add recipients to search output Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 02/11] lib: use new addresses structure for thread authors Jameson Graef Rollins ` (2 more replies) 2012-08-22 20:43 ` [PATCH 00/11] add recipients to search output Jameson Graef Rollins ` (2 subsequent siblings) 3 siblings, 3 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail This new structure holds addresses associated with a thread, both matched and unmatched. Initially this will be used to replace the existing infrastructure for storing the addresses of thread authors. Further patches will use it to store the addresses of threads recipients. Init and destructor functions are included, as well as a function to add addresses to a struct, either "matched" or not. --- lib/notmuch.h | 1 + lib/thread.cc | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/lib/notmuch.h b/lib/notmuch.h index 3633bed..6acd38d 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -118,6 +118,7 @@ typedef struct _notmuch_database notmuch_database_t; typedef struct _notmuch_query notmuch_query_t; typedef struct _notmuch_threads notmuch_threads_t; typedef struct _notmuch_thread notmuch_thread_t; +typedef struct _notmuch_thread_addresses notmuch_thread_addresses_t; typedef struct _notmuch_messages notmuch_messages_t; typedef struct _notmuch_message notmuch_message_t; typedef struct _notmuch_tags notmuch_tags_t; diff --git a/lib/thread.cc b/lib/thread.cc index e976d64..7af9eeb 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -24,6 +24,14 @@ #include <gmime/gmime.h> #include <glib.h> /* GHashTable */ +struct visible _notmuch_thread_addresses { + GHashTable *matched_hash; + GPtrArray *matched_array; + GHashTable *unmatched_hash; + GPtrArray *unmatched_array; + char *string; +}; + struct visible _notmuch_thread { notmuch_database_t *notmuch; char *thread_id; @@ -44,6 +52,18 @@ struct visible _notmuch_thread { }; static int +_notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses) +{ + g_hash_table_unref (addresses->matched_hash); + g_hash_table_unref (addresses->unmatched_hash); + g_ptr_array_free (addresses->matched_array, TRUE); + g_ptr_array_free (addresses->unmatched_array, TRUE); + addresses->matched_array = NULL; + addresses->unmatched_array = NULL; + return 0; +} + +static int _notmuch_thread_destructor (notmuch_thread_t *thread) { g_hash_table_unref (thread->authors_hash); @@ -64,6 +84,81 @@ _notmuch_thread_destructor (notmuch_thread_t *thread) return 0; } +/* Add address to a thread addresses struct. If matched is TRUE, then + * the address will be added to the matched list.*/ +static void +_thread_add_address (notmuch_thread_addresses_t *addresses, + const char *address, + notmuch_bool_t matched) +{ + char *address_copy; + GHashTable *hash; + GPtrArray *array; + + if (matched) { + hash = addresses->matched_hash; + array = addresses->matched_array; + } else { + hash = addresses->unmatched_hash; + array = addresses->unmatched_array; + } + + if (address == NULL) + return; + + if (g_hash_table_lookup_extended (hash, address, NULL, NULL)) + return; + + address_copy = talloc_strdup (addresses, address); + + g_hash_table_insert (hash, address_copy, NULL); + + g_ptr_array_add (array, address_copy); +} + +/* Construct an addresses string from matched and unmatched addresses + * in notmuch_thread_addresses_t. The string contains matched + * addresses first, then non-matched addresses (with the two groups + * separated by '|'). Within each group, addresses are listed in date + * order. */ +static void +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses) +{ + unsigned int i; + char *address; + int first_non_matched_address = 1; + + /* First, list all matched addressses in date order. */ + for (i = 0; i < addresses->matched_array->len; i++) { + address = (char *) g_ptr_array_index (addresses->matched_array, i); + if (addresses->string) + addresses->string = talloc_asprintf (addresses, "%s, %s", + addresses->string, + address); + else + addresses->string = address; + } + + /* Next, append any non-matched addresses that haven't already appeared. */ + for (i = 0; i < addresses->unmatched_array->len; i++) { + address = (char *) g_ptr_array_index (addresses->unmatched_array, i); + if (g_hash_table_lookup_extended (addresses->matched_hash, + address, NULL, NULL)) + continue; + if (first_non_matched_address) { + addresses->string = talloc_asprintf (addresses, "%s| %s", + addresses->string, + address); + } else { + addresses->string = talloc_asprintf (addresses, "%s, %s", + addresses->string, + address); + } + + first_non_matched_address = 0; + } +} + /* Add each author of the thread to the thread's authors_hash and to * the thread's authors_array. */ static void @@ -382,6 +477,27 @@ _resolve_thread_relationships (unused (notmuch_thread_t *thread)) */ } +/* Initialize a thread addresses struct. */ +notmuch_thread_addresses_t * +_thread_addresses_init (const void *ctx) +{ + notmuch_thread_addresses_t *addresses; + + addresses = talloc (ctx, notmuch_thread_addresses_t); + if (unlikely (addresses == NULL)) + return NULL; + + addresses->matched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, + NULL, NULL); + addresses->matched_array = g_ptr_array_new (); + addresses->unmatched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, + NULL, NULL); + addresses->unmatched_array = g_ptr_array_new (); + addresses->string = NULL; + + return addresses; +} + /* Create a new notmuch_thread_t object by finding the thread * containing the message with the given doc ID, treating any messages * contained in match_set as "matched". Remove all messages in the -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 02/11] lib: use new addresses structure for thread authors 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Jameson Graef Rollins 2012-09-08 17:24 ` [PATCH 02/11] lib: use new addresses structure for thread authors Austin Clements 2012-08-30 15:38 ` [PATCH 01/11] lib: new thread addresses structure Michal Sojka 2012-09-08 17:24 ` Austin Clements 2 siblings, 2 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail Now that we have the infrastructure in place, we modify the thread object and associated functions to use the new addresses structure for storing thread authors. --- lib/thread.cc | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/lib/thread.cc b/lib/thread.cc index 7af9eeb..9e0e5cb 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -36,11 +36,7 @@ struct visible _notmuch_thread { notmuch_database_t *notmuch; char *thread_id; char *subject; - GHashTable *authors_hash; - GPtrArray *authors_array; - GHashTable *matched_authors_hash; - GPtrArray *matched_authors_array; - char *authors; + notmuch_thread_addresses_t *authors; GHashTable *tags; notmuch_message_list_t *message_list; @@ -66,21 +62,9 @@ _notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses) static int _notmuch_thread_destructor (notmuch_thread_t *thread) { - g_hash_table_unref (thread->authors_hash); - g_hash_table_unref (thread->matched_authors_hash); + _notmuch_thread_addresses_destructor (thread->authors); g_hash_table_unref (thread->tags); g_hash_table_unref (thread->message_hash); - - if (thread->authors_array) { - g_ptr_array_free (thread->authors_array, TRUE); - thread->authors_array = NULL; - } - - if (thread->matched_authors_array) { - g_ptr_array_free (thread->matched_authors_array, TRUE); - thread->matched_authors_array = NULL; - } - return 0; } @@ -341,7 +325,7 @@ _thread_add_message (notmuch_thread_t *thread, author = internet_address_mailbox_get_addr (mailbox); } clean_author = _thread_cleanup_author (thread, author, from); - _thread_add_author (thread, clean_author); + _thread_add_address (thread->authors, clean_author, FALSE); notmuch_message_set_author (message, clean_author); } g_object_unref (G_OBJECT (list)); @@ -436,7 +420,7 @@ _thread_add_matched_message (notmuch_thread_t *thread, NOTMUCH_MESSAGE_FLAG_MATCH, 1); } - _thread_add_matched_author (thread, notmuch_message_get_author (hashed_message)); + _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE); } static void @@ -555,14 +539,11 @@ _notmuch_thread_create (void *ctx, thread->notmuch = notmuch; thread->thread_id = talloc_strdup (thread, thread_id); thread->subject = NULL; - thread->authors_hash = g_hash_table_new_full (g_str_hash, g_str_equal, - NULL, NULL); - thread->authors_array = g_ptr_array_new (); - thread->matched_authors_hash = g_hash_table_new_full (g_str_hash, - g_str_equal, - NULL, NULL); - thread->matched_authors_array = g_ptr_array_new (); - thread->authors = NULL; + + thread->authors = _thread_addresses_init (thread); + if (unlikely (thread->authors == NULL)) + return NULL; + thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal, free, NULL); @@ -607,7 +588,7 @@ _notmuch_thread_create (void *ctx, notmuch_query_destroy (thread_id_query); - _resolve_thread_authors_string (thread); + _resolve_thread_addresses_string (thread->authors); _resolve_thread_relationships (thread); @@ -641,7 +622,7 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread) const char * notmuch_thread_get_authors (notmuch_thread_t *thread) { - return thread->authors; + return thread->authors->string; } const char * -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 03/11] lib: give _thread_cleanup_author a more generic name 2012-08-20 1:52 ` [PATCH 02/11] lib: use new addresses structure for thread authors Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 04/11] lib: remove no longer needed author-specific thread functions Jameson Graef Rollins 2012-09-08 17:25 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Austin Clements 2012-09-08 17:24 ` [PATCH 02/11] lib: use new addresses structure for thread authors Austin Clements 1 sibling, 2 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail We will use this for cleaning non-author address fields, so we give it the more generic name _thread_cleanup_address. --- lib/thread.cc | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/lib/thread.cc b/lib/thread.cc index 9e0e5cb..b53ccb0 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -242,48 +242,49 @@ _resolve_thread_authors_string (notmuch_thread_t *thread) * "Last, First MI" <first.mi.last@company.com> */ static char * -_thread_cleanup_author (notmuch_thread_t *thread, - const char *author, const char *from) +_thread_cleanup_address (notmuch_thread_t *thread, + const char *address, + const char *original) { - char *clean_author,*test_author; + char *clean_address,*test_address; const char *comma; char *blank; int fname,lname; - if (author == NULL) + if (address == NULL) return NULL; - clean_author = talloc_strdup(thread, author); - if (clean_author == NULL) + clean_address = talloc_strdup(thread, address); + if (clean_address == NULL) return NULL; /* check if there's a comma in the name and that there's a * component of the name behind it (so the name doesn't end with * the comma - in which case the string that strchr finds is just * one character long ",\0"). - * Otherwise just return the copy of the original author name that + * Otherwise just return the copy of the original address name that * we just made*/ - comma = strchr(author,','); + comma = strchr(address,','); if (comma && strlen(comma) > 1) { /* let's assemble what we think is the correct name */ - lname = comma - author; - fname = strlen(author) - lname - 2; - strncpy(clean_author, comma + 2, fname); - *(clean_author+fname) = ' '; - strncpy(clean_author + fname + 1, author, lname); - *(clean_author+fname+1+lname) = '\0'; + lname = comma - address; + fname = strlen(address) - lname - 2; + strncpy(clean_address, comma + 2, fname); + *(clean_address+fname) = ' '; + strncpy(clean_address + fname + 1, address, lname); + *(clean_address+fname+1+lname) = '\0'; /* make a temporary copy and see if it matches the email */ - test_author = talloc_strdup(thread,clean_author); + test_address = talloc_strdup(thread,clean_address); - blank=strchr(test_author,' '); + blank=strchr(test_address,' '); while (blank != NULL) { *blank = '.'; - blank=strchr(test_author,' '); + blank=strchr(test_address,' '); } - if (strcasestr(from, test_author) == NULL) + if (strcasestr(original, test_address) == NULL) /* we didn't identify this as part of the email address - * so let's punt and return the original author */ - strcpy (clean_author, author); + * so let's punt and return the original address */ + strcpy (clean_address, address); } - return clean_author; + return clean_address; } /* Add 'message' as a message that belongs to 'thread'. -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 04/11] lib: remove no longer needed author-specific thread functions 2012-08-20 1:52 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 05/11] lib: add ability to store recipients in message structure Jameson Graef Rollins 2012-09-08 17:25 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Austin Clements 1 sibling, 1 reply; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail The _add_author, _add_matched_author, and _resolve_thread_authors_string functions have been replaced by the more generic _address functions that utilize the new notmuch_thread_addresses_t. --- lib/thread.cc | 91 --------------------------------------------------------- 1 file changed, 91 deletions(-) diff --git a/lib/thread.cc b/lib/thread.cc index b53ccb0..757e143 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -143,97 +143,6 @@ _resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses) } } -/* Add each author of the thread to the thread's authors_hash and to - * the thread's authors_array. */ -static void -_thread_add_author (notmuch_thread_t *thread, - const char *author) -{ - char *author_copy; - - if (author == NULL) - return; - - if (g_hash_table_lookup_extended (thread->authors_hash, - author, NULL, NULL)) - return; - - author_copy = talloc_strdup (thread, author); - - g_hash_table_insert (thread->authors_hash, author_copy, NULL); - - g_ptr_array_add (thread->authors_array, author_copy); -} - -/* Add each matched author of the thread to the thread's - * matched_authors_hash and to the thread's matched_authors_array. */ -static void -_thread_add_matched_author (notmuch_thread_t *thread, - const char *author) -{ - char *author_copy; - - if (author == NULL) - return; - - if (g_hash_table_lookup_extended (thread->matched_authors_hash, - author, NULL, NULL)) - return; - - author_copy = talloc_strdup (thread, author); - - g_hash_table_insert (thread->matched_authors_hash, author_copy, NULL); - - g_ptr_array_add (thread->matched_authors_array, author_copy); -} - -/* Construct an authors string from matched_authors_array and - * authors_array. The string contains matched authors first, then - * non-matched authors (with the two groups separated by '|'). Within - * each group, authors are listed in date order. */ -static void -_resolve_thread_authors_string (notmuch_thread_t *thread) -{ - unsigned int i; - char *author; - int first_non_matched_author = 1; - - /* First, list all matched authors in date order. */ - for (i = 0; i < thread->matched_authors_array->len; i++) { - author = (char *) g_ptr_array_index (thread->matched_authors_array, i); - if (thread->authors) - thread->authors = talloc_asprintf (thread, "%s, %s", - thread->authors, - author); - else - thread->authors = author; - } - - /* Next, append any non-matched authors that haven't already appeared. */ - for (i = 0; i < thread->authors_array->len; i++) { - author = (char *) g_ptr_array_index (thread->authors_array, i); - if (g_hash_table_lookup_extended (thread->matched_authors_hash, - author, NULL, NULL)) - continue; - if (first_non_matched_author) { - thread->authors = talloc_asprintf (thread, "%s| %s", - thread->authors, - author); - } else { - thread->authors = talloc_asprintf (thread, "%s, %s", - thread->authors, - author); - } - - first_non_matched_author = 0; - } - - g_ptr_array_free (thread->authors_array, TRUE); - thread->authors_array = NULL; - g_ptr_array_free (thread->matched_authors_array, TRUE); - thread->matched_authors_array = NULL; -} - /* clean up the ugly "Lastname, Firstname" format that some mail systems * (most notably, Exchange) are creating to be "Firstname Lastname" * To make sure that we don't change other potential situations where a -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 05/11] lib: add ability to store recipients in message structure 2012-08-20 1:52 ` [PATCH 04/11] lib: remove no longer needed author-specific thread functions Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail Added as a string, in parallel to the authors element. --- lib/message.cc | 18 ++++++++++++++++++ lib/notmuch-private.h | 9 +++++++++ 2 files changed, 27 insertions(+) diff --git a/lib/message.cc b/lib/message.cc index 978de06..fa28073 100644 --- a/lib/message.cc +++ b/lib/message.cc @@ -36,6 +36,7 @@ struct visible _notmuch_message { notmuch_string_list_t *filename_term_list; notmuch_string_list_t *filename_list; char *author; + char *recipients; notmuch_message_file_t *message_file; notmuch_message_list_t *replies; unsigned long flags; @@ -109,6 +110,7 @@ _notmuch_message_create_for_document (const void *talloc_owner, message->filename_list = NULL; message->message_file = NULL; message->author = NULL; + message->recipients = NULL; message->replies = _notmuch_message_list_create (message); if (unlikely (message->replies == NULL)) { @@ -808,6 +810,22 @@ notmuch_message_set_author (notmuch_message_t *message, return; } +const char * +notmuch_message_get_recipients (notmuch_message_t *message) +{ + return message->recipients; +} + +void +notmuch_message_set_recipients (notmuch_message_t *message, + const char *recipients) +{ + if (message->recipients) + talloc_free(message->recipients); + message->recipients = talloc_strdup(message, recipients); + return; +} + void _notmuch_message_set_header_values (notmuch_message_t *message, const char *date, diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h index bfb4111..27a41b6 100644 --- a/lib/notmuch-private.h +++ b/lib/notmuch-private.h @@ -325,6 +325,15 @@ notmuch_message_set_author (notmuch_message_t *message, const char *author); const char * notmuch_message_get_author (notmuch_message_t *message); +/* Set the recipients of 'message' - this is the representation used + * when displaying the message */ +void +notmuch_message_set_recipients (notmuch_message_t *message, const char *recipients); + +/* Get the authors of 'message' */ +const char * +notmuch_message_get_recipients (notmuch_message_t *message); + /* index.cc */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 06/11] lib: store thread recipients in thread structure 2012-08-20 1:52 ` [PATCH 05/11] lib: add ability to store recipients in message structure Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 07/11] test: search recipient output Jameson Graef Rollins ` (3 more replies) 2012-08-31 20:19 ` [PATCH 05/11] lib: add ability to store recipients in message structure Michal Sojka 2012-09-08 17:24 ` Austin Clements 2 siblings, 4 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail This utilizes the new thread addresses struct to store thread recipients, again in parallel to authors. Since message recipients are not stored in the database, including recipients in the thread structure exacts a significant overhead as the recipients are retrieved from the original message files. Because of this, a new boolean argument, include_recipients, is added to the necessary functions (_notmuch_thread_create, _thread_add_message and _thread_add_matched_message) that controls whether the recipients are fetched and included. If message recipients are ever stored in the database this new argument could probably be removed. --- lib/notmuch-private.h | 3 +- lib/notmuch.h | 14 +++++++++ lib/query.cc | 3 +- lib/thread.cc | 77 +++++++++++++++++++++++++++++++++++++------------ 4 files changed, 76 insertions(+), 21 deletions(-) diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h index 27a41b6..32d1523 100644 --- a/lib/notmuch-private.h +++ b/lib/notmuch-private.h @@ -232,7 +232,8 @@ _notmuch_thread_create (void *ctx, unsigned int seed_doc_id, notmuch_doc_id_set_t *match_set, notmuch_string_list_t *excluded_terms, - notmuch_sort_t sort); + notmuch_sort_t sort, + notmuch_bool_t include_recipients); /* message.cc */ diff --git a/lib/notmuch.h b/lib/notmuch.h index 6acd38d..f9e71c1 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -759,6 +759,20 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread); const char * notmuch_thread_get_authors (notmuch_thread_t *thread); +/* Get the recipients of 'thread' + * + * The returned string is a comma-separated list of the names of the + * recipients of mail messages in the query results that belong to this + * thread. + * + * The returned string belongs to 'thread' and as such, should not be + * modified by the caller and will only be valid for as long as the + * thread is valid, (which is until notmuch_thread_destroy or until + * the query from which it derived is destroyed). + */ +const char * +notmuch_thread_get_recipients (notmuch_thread_t *thread); + /* Get the subject of 'thread' * * The subject is taken from the first message (according to the query diff --git a/lib/query.cc b/lib/query.cc index e9c1a2d..54833a7 100644 --- a/lib/query.cc +++ b/lib/query.cc @@ -486,7 +486,8 @@ notmuch_threads_get (notmuch_threads_t *threads) doc_id, &threads->match_set, threads->query->exclude_terms, - threads->query->sort); + threads->query->sort, + FALSE); } void diff --git a/lib/thread.cc b/lib/thread.cc index 757e143..baf07c2 100644 --- a/lib/thread.cc +++ b/lib/thread.cc @@ -37,6 +37,7 @@ struct visible _notmuch_thread { char *thread_id; char *subject; notmuch_thread_addresses_t *authors; + notmuch_thread_addresses_t *recipients; GHashTable *tags; notmuch_message_list_t *message_list; @@ -63,6 +64,7 @@ static int _notmuch_thread_destructor (notmuch_thread_t *thread) { _notmuch_thread_addresses_destructor (thread->authors); + _notmuch_thread_addresses_destructor (thread->recipients); g_hash_table_unref (thread->tags); g_hash_table_unref (thread->message_hash); return 0; @@ -204,14 +206,17 @@ _thread_cleanup_address (notmuch_thread_t *thread, static void _thread_add_message (notmuch_thread_t *thread, notmuch_message_t *message, - notmuch_string_list_t *exclude_terms) + notmuch_string_list_t *exclude_terms, + notmuch_bool_t include_recipients) { notmuch_tags_t *tags; const char *tag; - InternetAddressList *list = NULL; + InternetAddressList *from_list = NULL; + InternetAddressList *to_list = NULL; InternetAddress *address; const char *from, *author; - char *clean_author; + const char *to, *recipient; + char *clean_address; _notmuch_message_list_add_message (thread->message_list, talloc_steal (thread, message)); @@ -223,10 +228,9 @@ _thread_add_message (notmuch_thread_t *thread, from = notmuch_message_get_header (message, "from"); if (from) - list = internet_address_list_parse_string (from); - - if (list) { - address = internet_address_list_get_address (list, 0); + from_list = internet_address_list_parse_string (from); + if (from_list) { + address = internet_address_list_get_address (from_list, 0); if (address) { author = internet_address_get_name (address); if (author == NULL) { @@ -234,11 +238,32 @@ _thread_add_message (notmuch_thread_t *thread, mailbox = INTERNET_ADDRESS_MAILBOX (address); author = internet_address_mailbox_get_addr (mailbox); } - clean_author = _thread_cleanup_author (thread, author, from); - _thread_add_address (thread->authors, clean_author, FALSE); - notmuch_message_set_author (message, clean_author); + clean_address = _thread_cleanup_address (thread, author, from); + _thread_add_address (thread->authors, clean_address, FALSE); + notmuch_message_set_author (message, clean_address); + } + g_object_unref (G_OBJECT (from_list)); + } + + if (include_recipients) { + to = notmuch_message_get_header (message, "to"); + if (to) + to_list = internet_address_list_parse_string (to); + if (to_list) { + address = internet_address_list_get_address (to_list, 0); + if (address) { + recipient = internet_address_get_name (address); + if (recipient == NULL) { + InternetAddressMailbox *mailbox; + mailbox = INTERNET_ADDRESS_MAILBOX (address); + recipient = internet_address_mailbox_get_addr (mailbox); + } + clean_address = _thread_cleanup_address (thread, recipient, to); + _thread_add_address (thread->recipients, clean_address, FALSE); + notmuch_message_set_recipients (message, clean_address); } - g_object_unref (G_OBJECT (list)); + g_object_unref (G_OBJECT (to_list)); + } } if (! thread->subject) { @@ -301,7 +326,8 @@ _thread_set_subject_from_message (notmuch_thread_t *thread, static void _thread_add_matched_message (notmuch_thread_t *thread, notmuch_message_t *message, - notmuch_sort_t sort) + notmuch_sort_t sort, + notmuch_bool_t include_recipients) { time_t date; notmuch_message_t *hashed_message; @@ -331,6 +357,8 @@ _thread_add_matched_message (notmuch_thread_t *thread, } _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE); + if (include_recipients) + _thread_add_address (thread->recipients, notmuch_message_get_recipients (hashed_message), TRUE); } static void @@ -399,10 +427,10 @@ _thread_addresses_init (const void *ctx) * * Creating the thread will perform a database search to get all * messages belonging to the thread and will get the first subject - * line, the total count of messages, and all authors in the thread. - * Each message in the thread is checked against match_set to allow - * for a separate count of matched messages, and to allow a viewer to - * display these messages differently. + * line, the total count of messages, and all authors and recipients + * of the thread. Each message in the thread is checked against + * match_set to allow for a separate count of matched messages, and to + * allow a viewer to display these messages differently. * * Here, 'ctx' is talloc context for the resulting thread object. * @@ -414,7 +442,8 @@ _notmuch_thread_create (void *ctx, unsigned int seed_doc_id, notmuch_doc_id_set_t *match_set, notmuch_string_list_t *exclude_terms, - notmuch_sort_t sort) + notmuch_sort_t sort, + notmuch_bool_t include_recipients) { notmuch_thread_t *thread; notmuch_message_t *seed_message; @@ -453,6 +482,9 @@ _notmuch_thread_create (void *ctx, thread->authors = _thread_addresses_init (thread); if (unlikely (thread->authors == NULL)) return NULL; + thread->recipients = _thread_addresses_init (thread); + if (unlikely (thread->recipients == NULL)) + return NULL; thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal, free, NULL); @@ -486,11 +518,11 @@ _notmuch_thread_create (void *ctx, if (doc_id == seed_doc_id) message = seed_message; - _thread_add_message (thread, message, exclude_terms); + _thread_add_message (thread, message, exclude_terms, include_recipients); if ( _notmuch_doc_id_set_contains (match_set, doc_id)) { _notmuch_doc_id_set_remove (match_set, doc_id); - _thread_add_matched_message (thread, message, sort); + _thread_add_matched_message (thread, message, sort, include_recipients); } _notmuch_message_close (message); @@ -499,6 +531,7 @@ _notmuch_thread_create (void *ctx, notmuch_query_destroy (thread_id_query); _resolve_thread_addresses_string (thread->authors); + _resolve_thread_addresses_string (thread->recipients); _resolve_thread_relationships (thread); @@ -536,6 +569,12 @@ notmuch_thread_get_authors (notmuch_thread_t *thread) } const char * +notmuch_thread_get_recipients (notmuch_thread_t *thread) +{ + return thread->recipients->string; +} + +const char * notmuch_thread_get_subject (notmuch_thread_t *thread) { return thread->subject; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 07/11] test: search recipient output 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 08/11] cli: add thread recipients to search output Jameson Graef Rollins 2012-08-31 20:44 ` [PATCH 06/11] lib: store thread recipients in thread structure Michal Sojka ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail Simple json test for a new additional recipient field. known_broken to be removed with next patch. --- test/json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/json b/test/json index ac8fa8e..ac423a8 100755 --- a/test/json +++ b/test/json @@ -60,4 +60,18 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\", \"tags\": [\"inbox\", \"unread\"]}]" +test_begin_subtest "Search message: include recipients" +test_subtest_known_broken +output=$(notmuch search --format=json --include-recipients "json-search-message" | notmuch_search_sanitize) +test_expect_equal_json "$output" "[{\"thread\": \"XXX\", + \"timestamp\": 946728000, + \"date_relative\": \"2000-01-01\", + \"matched\": 1, + \"total\": 1, + \"authors\": \"Notmuch Test Suite\", + \"recipients\": \"Notmuch Test Suite\", + \"subject\": \"json-search-subject\", + \"tags\": [\"inbox\", + \"unread\"]}]" + test_done -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 08/11] cli: add thread recipients to search output 2012-08-20 1:52 ` [PATCH 07/11] test: search recipient output Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 09/11] emacs: add ability to show recipients instead of author in search Jameson Graef Rollins 2012-08-31 21:00 ` [PATCH 08/11] cli: add thread recipients to search output Michal Sojka 0 siblings, 2 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail This adds a "--include-recipients" option to notmuch search. With structured output formats (e.g. json), a new recipients field will be included that holds recipients of the thread. Matched and non-matched recipients are delineated as with authors. As mentioned in the previous patch for the underlying lib functions, the need for the option is because message recipients are not stored in the database and therefore retrieving them adds a significant overhead. If they were included, this option would not be necessary. --- lib/notmuch.h | 6 +++++- lib/query.cc | 5 +++-- notmuch-search.c | 14 +++++++++++--- notmuch-show.c | 2 +- test/json | 1 - 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/notmuch.h b/lib/notmuch.h index f9e71c1..8eb455e 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -642,6 +642,9 @@ notmuch_threads_valid (notmuch_threads_t *threads); /* Get the current thread from 'threads' as a notmuch_thread_t. * + * If the include_recipients flag is TRUE, thread recipients will be + * included in the returned thread object. + * * Note: The returned thread belongs to 'threads' and has a lifetime * identical to it (and the query to which it belongs). * @@ -652,7 +655,8 @@ notmuch_threads_valid (notmuch_threads_t *threads); * NULL. */ notmuch_thread_t * -notmuch_threads_get (notmuch_threads_t *threads); +notmuch_threads_get (notmuch_threads_t *threads, + notmuch_bool_t include_recipients); /* Move the 'threads' iterator to the next thread. * diff --git a/lib/query.cc b/lib/query.cc index 54833a7..0a4f058 100644 --- a/lib/query.cc +++ b/lib/query.cc @@ -472,7 +472,8 @@ notmuch_threads_valid (notmuch_threads_t *threads) } notmuch_thread_t * -notmuch_threads_get (notmuch_threads_t *threads) +notmuch_threads_get (notmuch_threads_t *threads, + notmuch_bool_t include_recipients) { unsigned int doc_id; @@ -487,7 +488,7 @@ notmuch_threads_get (notmuch_threads_t *threads) &threads->match_set, threads->query->exclude_terms, threads->query->sort, - FALSE); + include_recipients); } void diff --git a/notmuch-search.c b/notmuch-search.c index 830c4e4..f610a84 100644 --- a/notmuch-search.c +++ b/notmuch-search.c @@ -52,7 +52,8 @@ do_search_threads (sprinter_t *format, notmuch_sort_t sort, output_t output, int offset, - int limit) + int limit, + notmuch_bool_t include_recipients) { notmuch_thread_t *thread; notmuch_threads_t *threads; @@ -76,7 +77,7 @@ do_search_threads (sprinter_t *format, notmuch_threads_valid (threads) && (limit < 0 || i < offset + limit); notmuch_threads_move_to_next (threads), i++) { - thread = notmuch_threads_get (threads); + thread = notmuch_threads_get (threads, include_recipients); if (i < offset) { notmuch_thread_destroy (thread); @@ -91,6 +92,7 @@ do_search_threads (sprinter_t *format, } else { /* output == OUTPUT_SUMMARY */ void *ctx_quote = talloc_new (thread); const char *authors = notmuch_thread_get_authors (thread); + const char *recipients = notmuch_thread_get_recipients (thread); const char *subject = notmuch_thread_get_subject (thread); const char *thread_id = notmuch_thread_get_thread_id (thread); int matched = notmuch_thread_get_matched_messages (thread); @@ -129,6 +131,10 @@ do_search_threads (sprinter_t *format, format->integer (format, total); format->map_key (format, "authors"); format->string (format, authors); + if (include_recipients) { + format->map_key (format, "recipients"); + format->string (format, recipients); + } format->map_key (format, "subject"); format->string (format, subject); } @@ -303,6 +309,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) int offset = 0; int limit = -1; /* unlimited */ int exclude = EXCLUDE_TRUE; + notmuch_bool_t include_recipients = FALSE; unsigned int i; enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT } @@ -331,6 +338,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) { 0, 0 } } }, { NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 }, { NOTMUCH_OPT_INT, &limit, "limit", 'L', 0 }, + { NOTMUCH_OPT_BOOLEAN, &include_recipients, "include-recipients", 'r', 0 }, { 0, 0, 0, 0, 0 } }; @@ -402,7 +410,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[]) default: case OUTPUT_SUMMARY: case OUTPUT_THREADS: - ret = do_search_threads (format, query, sort, output, offset, limit); + ret = do_search_threads (format, query, sort, output, offset, limit, include_recipients); break; case OUTPUT_MESSAGES: case OUTPUT_FILES: diff --git a/notmuch-show.c b/notmuch-show.c index 3556293..cc4b428 100644 --- a/notmuch-show.c +++ b/notmuch-show.c @@ -965,7 +965,7 @@ do_show (void *ctx, notmuch_threads_valid (threads); notmuch_threads_move_to_next (threads)) { - thread = notmuch_threads_get (threads); + thread = notmuch_threads_get (threads, FALSE); messages = notmuch_thread_get_toplevel_messages (thread); diff --git a/test/json b/test/json index ac423a8..f441b59 100755 --- a/test/json +++ b/test/json @@ -61,7 +61,6 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\", \"unread\"]}]" test_begin_subtest "Search message: include recipients" -test_subtest_known_broken output=$(notmuch search --format=json --include-recipients "json-search-message" | notmuch_search_sanitize) test_expect_equal_json "$output" "[{\"thread\": \"XXX\", \"timestamp\": 946728000, -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 09/11] emacs: add ability to show recipients instead of author in search 2012-08-20 1:52 ` [PATCH 08/11] cli: add thread recipients to search output Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 10/11] emacs: add function to toggle showing authors/recipients " Jameson Graef Rollins 2012-08-31 21:00 ` [PATCH 08/11] cli: add thread recipients to search output Michal Sojka 1 sibling, 1 reply; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail A new boolean argument is added to notmuch-search to control whether thread authors or recipients are shown in the 'authors' field. --- emacs/notmuch-hello.el | 6 +++--- emacs/notmuch.el | 40 +++++++++++++++++++++++++++++----------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el index 684bedc..4615cf6 100644 --- a/emacs/notmuch-hello.el +++ b/emacs/notmuch-hello.el @@ -26,7 +26,7 @@ (require 'notmuch-lib) (require 'notmuch-mua) -(declare-function notmuch-search "notmuch" (query &optional oldest-first target-thread target-line continuation)) +(declare-function notmuch-search "notmuch" (query &optional oldest-first show-recipients target-thread target-line continuation)) (declare-function notmuch-poll "notmuch" ()) (defcustom notmuch-hello-recent-searches-max 10 @@ -280,7 +280,7 @@ afterwards.") (setq search (notmuch-hello-trim search)) (let ((history-delete-duplicates t)) (add-to-history 'notmuch-search-history search))) - (notmuch-search search notmuch-search-oldest-first nil nil + (notmuch-search search notmuch-search-oldest-first nil nil nil #'notmuch-hello-search-continuation)) (defun notmuch-hello-add-saved-search (widget) @@ -331,7 +331,7 @@ diagonal." (notmuch-search (widget-get widget :notmuch-search-terms) notmuch-search-oldest-first - nil nil #'notmuch-hello-search-continuation)) + nil nil nil #'notmuch-hello-search-continuation)) (defun notmuch-saved-search-count (search) (car (process-lines notmuch-command "count" search))) diff --git a/emacs/notmuch.el b/emacs/notmuch.el index 7b61e9b..d05b1e8 100644 --- a/emacs/notmuch.el +++ b/emacs/notmuch.el @@ -254,6 +254,7 @@ For a mouse binding, return nil." (notmuch-common-do-stash (notmuch-search-find-thread-id))) (defvar notmuch-search-query-string) +(defvar notmuch-search-show-recipients) (defvar notmuch-search-target-thread) (defvar notmuch-search-target-line) (defvar notmuch-search-continuation) @@ -405,6 +406,7 @@ Complete list of currently available key bindings: (make-local-variable 'notmuch-search-oldest-first) (make-local-variable 'notmuch-search-target-thread) (make-local-variable 'notmuch-search-target-line) + (make-local-variable 'notmuch-search-show-recipients) (set (make-local-variable 'notmuch-search-continuation) nil) (set (make-local-variable 'scroll-preserve-screen-position) t) (add-to-invisibility-spec (cons 'ellipsis t)) @@ -781,7 +783,10 @@ non-authors is found, assume that all of the authors match." 'face 'notmuch-search-subject))) ((string-equal field "authors") - (notmuch-search-insert-authors format-string (plist-get result :authors))) + (notmuch-search-insert-authors format-string + (if notmuch-search-show-recipients + (plist-get result :recipients) + (plist-get result :authors)))) ((string-equal field "tags") (let ((tags-str (mapconcat 'identity (plist-get result :tags) " "))) @@ -931,13 +936,16 @@ PROMPT is the string to prompt with." 'notmuch-search-history nil nil))))) ;;;###autoload -(defun notmuch-search (&optional query oldest-first target-thread target-line continuation) +(defun notmuch-search (&optional query oldest-first show-recipients target-thread target-line continuation) "Run \"notmuch search\" with the given `query' and display results. If `query' is nil, it is read interactively from the minibuffer. Other optional parameters are used as follows: oldest-first: A Boolean controlling the sort order of returned threads + show-recipients: A Boolean controlling whether or not thread + recipients instead of authors are shown in the + 'authors' field. target-thread: A thread ID (without the thread: prefix) that will be made current if it appears in the search results. target-line: The line number to move to if the target thread does not @@ -952,25 +960,34 @@ Other optional parameters are used as follows: (set 'buffer-undo-list t) (set 'notmuch-search-query-string query) (set 'notmuch-search-oldest-first oldest-first) + (set 'notmuch-search-show-recipients show-recipients) (set 'notmuch-search-target-thread target-thread) (set 'notmuch-search-target-line target-line) (set 'notmuch-search-continuation continuation) (let ((proc (get-buffer-process (current-buffer))) + (proc-args (list + "notmuch-search" buffer + notmuch-command "search" + "--format=json" + (if oldest-first + "--sort=oldest-first" + "--sort=newest-first"))) (inhibit-read-only t)) + (if show-recipients + (setq proc-args (append proc-args '("--include-recipients")))) (if proc (error "notmuch search process already running for query `%s'" query) ) (erase-buffer) (goto-char (point-min)) (save-excursion - (let ((proc (start-process - "notmuch-search" buffer - notmuch-command "search" - "--format=json" - (if oldest-first - "--sort=oldest-first" - "--sort=newest-first") - query)) + (let ( + ;; start-process insists on non-nil string arguments. + ;; This is annoying for variable length argument lists. + ;; We use apply here so that we can construct the + ;; start-process argument list ahead of time (instead of + ;; at invocation) to avoid nils. + (proc (apply 'start-process (append proc-args (list query)))) ;; Use a scratch buffer to accumulate partial output. ;; This buffer will be killed by the sentinel, which ;; should be called no matter how the process dies. @@ -995,11 +1012,12 @@ same relative position within the new buffer." (interactive) (let ((target-line (line-number-at-pos)) (oldest-first notmuch-search-oldest-first) + (show-recipients notmuch-search-show-recipients) (target-thread (notmuch-search-find-thread-id 'bare)) (query notmuch-search-query-string) (continuation notmuch-search-continuation)) (notmuch-kill-this-buffer) - (notmuch-search query oldest-first target-thread target-line continuation) + (notmuch-search query oldest-first show-recipients target-thread target-line continuation) (goto-char (point-min)))) (defcustom notmuch-poll-script nil -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 10/11] emacs: add function to toggle showing authors/recipients in search 2012-08-20 1:52 ` [PATCH 09/11] emacs: add ability to show recipients instead of author in search Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 11/11] lib: add recipients to database Jameson Graef Rollins 0 siblings, 1 reply; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail New function, notmuch-search-toggle-tofrom, toggles whether thread authors or recipients are shown in the 'authors' fields in the search results. This is bound to the '/' key. --- emacs/notmuch.el | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/emacs/notmuch.el b/emacs/notmuch.el index d05b1e8..ca16583 100644 --- a/emacs/notmuch.el +++ b/emacs/notmuch.el @@ -226,6 +226,7 @@ For a mouse binding, return nil." (define-key map "m" 'notmuch-mua-new-mail) (define-key map "s" 'notmuch-search) (define-key map "o" 'notmuch-search-toggle-order) + (define-key map "/" 'notmuch-search-toggle-tofrom) (define-key map "c" 'notmuch-search-stash-map) (define-key map "=" 'notmuch-search-refresh-view) (define-key map "G" 'notmuch-search-poll-and-refresh-view) @@ -1082,6 +1083,16 @@ search." (set 'notmuch-search-oldest-first (not notmuch-search-oldest-first)) (notmuch-search-refresh-view)) +(defun notmuch-search-toggle-tofrom () + "Toggle showing thread authors or recipients. + +By default, search results display the authors of the threads in +the \"authors\" field. This command toggles the display of +thread recipients in that field instead." + (interactive) + (set 'notmuch-search-show-recipients (not notmuch-search-show-recipients)) + (notmuch-search-refresh-view)) + (defun notmuch-search-filter (query) "Filter the current search results based on an additional query string. -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 11/11] lib: add recipients to database 2012-08-20 1:52 ` [PATCH 10/11] emacs: add function to toggle showing authors/recipients " Jameson Graef Rollins @ 2012-08-20 1:52 ` Jameson Graef Rollins 2012-08-31 21:34 ` Michal Sojka 0 siblings, 1 reply; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-20 1:52 UTC (permalink / raw) To: Notmuch Mail This adds just the "to" recipients, but probably "cc"s should be included as well. --- lib/database.cc | 2 +- lib/message.cc | 4 ++++ lib/notmuch-private.h | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/database.cc b/lib/database.cc index 761dc1a..4c1d578 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -1814,7 +1814,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch, goto DONE; date = notmuch_message_file_get_header (message_file, "date"); - _notmuch_message_set_header_values (message, date, from, subject); + _notmuch_message_set_header_values (message, date, from, to, subject); _notmuch_message_index_file (message, filename); } else { diff --git a/lib/message.cc b/lib/message.cc index fa28073..cc5c8a0 100644 --- a/lib/message.cc +++ b/lib/message.cc @@ -420,6 +420,8 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header) * available */ if (strcasecmp (header, "from") == 0) value = message->doc.get_value (NOTMUCH_VALUE_FROM); + if (strcasecmp (header, "to") == 0) + value = message->doc.get_value (NOTMUCH_VALUE_TO); else if (strcasecmp (header, "subject") == 0) value = message->doc.get_value (NOTMUCH_VALUE_SUBJECT); else if (strcasecmp (header, "message-id") == 0) @@ -830,6 +832,7 @@ void _notmuch_message_set_header_values (notmuch_message_t *message, const char *date, const char *from, + const char *to, const char *subject) { time_t time_value; @@ -844,6 +847,7 @@ _notmuch_message_set_header_values (notmuch_message_t *message, message->doc.add_value (NOTMUCH_VALUE_TIMESTAMP, Xapian::sortable_serialise (time_value)); message->doc.add_value (NOTMUCH_VALUE_FROM, from); + message->doc.add_value (NOTMUCH_VALUE_TO, to); message->doc.add_value (NOTMUCH_VALUE_SUBJECT, subject); } diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h index 32d1523..f56c580 100644 --- a/lib/notmuch-private.h +++ b/lib/notmuch-private.h @@ -95,6 +95,7 @@ typedef enum { NOTMUCH_VALUE_TIMESTAMP = 0, NOTMUCH_VALUE_MESSAGE_ID, NOTMUCH_VALUE_FROM, + NOTMUCH_VALUE_TO, NOTMUCH_VALUE_SUBJECT } notmuch_value_t; @@ -291,6 +292,7 @@ void _notmuch_message_set_header_values (notmuch_message_t *message, const char *date, const char *from, + const char *to, const char *subject); void _notmuch_message_sync (notmuch_message_t *message); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 11/11] lib: add recipients to database 2012-08-20 1:52 ` [PATCH 11/11] lib: add recipients to database Jameson Graef Rollins @ 2012-08-31 21:34 ` Michal Sojka 0 siblings, 0 replies; 27+ messages in thread From: Michal Sojka @ 2012-08-31 21:34 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail On Mon, Aug 20 2012, Jameson Graef Rollins wrote: > This adds just the "to" recipients, but probably "cc"s should be > included as well. > --- > lib/database.cc | 2 +- > lib/message.cc | 4 ++++ > lib/notmuch-private.h | 2 ++ > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/lib/database.cc b/lib/database.cc > index 761dc1a..4c1d578 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -1814,7 +1814,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch, > goto DONE; > > date = notmuch_message_file_get_header (message_file, "date"); > - _notmuch_message_set_header_values (message, date, from, subject); > + _notmuch_message_set_header_values (message, date, from, to, subject); > > _notmuch_message_index_file (message, filename); > } else { > diff --git a/lib/message.cc b/lib/message.cc > index fa28073..cc5c8a0 100644 > --- a/lib/message.cc > +++ b/lib/message.cc > @@ -420,6 +420,8 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header) > * available */ > if (strcasecmp (header, "from") == 0) > value = message->doc.get_value (NOTMUCH_VALUE_FROM); > + if (strcasecmp (header, "to") == 0) > + value = message->doc.get_value (NOTMUCH_VALUE_TO); > else if (strcasecmp (header, "subject") == 0) > value = message->doc.get_value (NOTMUCH_VALUE_SUBJECT); > else if (strcasecmp (header, "message-id") == 0) > @@ -830,6 +832,7 @@ void > _notmuch_message_set_header_values (notmuch_message_t *message, > const char *date, > const char *from, > + const char *to, > const char *subject) > { > time_t time_value; > @@ -844,6 +847,7 @@ _notmuch_message_set_header_values (notmuch_message_t *message, > message->doc.add_value (NOTMUCH_VALUE_TIMESTAMP, > Xapian::sortable_serialise (time_value)); > message->doc.add_value (NOTMUCH_VALUE_FROM, from); > + message->doc.add_value (NOTMUCH_VALUE_TO, to); > message->doc.add_value (NOTMUCH_VALUE_SUBJECT, subject); > } > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h > index 32d1523..f56c580 100644 > --- a/lib/notmuch-private.h > +++ b/lib/notmuch-private.h > @@ -95,6 +95,7 @@ typedef enum { > NOTMUCH_VALUE_TIMESTAMP = 0, > NOTMUCH_VALUE_MESSAGE_ID, > NOTMUCH_VALUE_FROM, > + NOTMUCH_VALUE_TO, It would be definitely useful to add other headers to the database (and make them searchable). As far as I remember this is on the todo list for ages. I'm only not sure that the approach of adding every possible header manually (like in this patch) is a good approach. Emails can contain arbitrary headers so there would be always some header missing. I'm not that much familiar with Xapian to figure out how to implement this. Otherwise this series looks quite well. I'm only not sure whether to merge it now or after changing the storage of headers in the database. Probably, if emacs interface is also extended to use this feature as part of saved searches or hello-section in a way that I can see the list of drafts on one click from hello screen. I'd vote for merging this now. However, I've just looked how does my drafts folder (messages saved with C-x C-s) looks like and one problem I see there is that draft messages do not have message ids. Do you have an idea how to implement drafts in emacs UI? Thanks, -Michal ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 08/11] cli: add thread recipients to search output 2012-08-20 1:52 ` [PATCH 08/11] cli: add thread recipients to search output Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 09/11] emacs: add ability to show recipients instead of author in search Jameson Graef Rollins @ 2012-08-31 21:00 ` Michal Sojka 1 sibling, 0 replies; 27+ messages in thread From: Michal Sojka @ 2012-08-31 21:00 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail On Mon, Aug 20 2012, Jameson Graef Rollins wrote: > This adds a "--include-recipients" option to notmuch search. With > structured output formats (e.g. json), a new recipients field will be > included that holds recipients of the thread. Matched and non-matched > recipients are delineated as with authors. > > As mentioned in the previous patch for the underlying lib functions, > the need for the option is because message recipients are not stored > in the database and therefore retrieving them adds a significant > overhead. If they were included, this option would not be necessary. > --- > lib/notmuch.h | 6 +++++- > lib/query.cc | 5 +++-- > notmuch-search.c | 14 +++++++++++--- > notmuch-show.c | 2 +- > test/json | 1 - > 5 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/lib/notmuch.h b/lib/notmuch.h > index f9e71c1..8eb455e 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -642,6 +642,9 @@ notmuch_threads_valid (notmuch_threads_t *threads); > > /* Get the current thread from 'threads' as a notmuch_thread_t. > * > + * If the include_recipients flag is TRUE, thread recipients will be > + * included in the returned thread object. > + * > * Note: The returned thread belongs to 'threads' and has a lifetime > * identical to it (and the query to which it belongs). > * > @@ -652,7 +655,8 @@ notmuch_threads_valid (notmuch_threads_t *threads); > * NULL. > */ > notmuch_thread_t * > -notmuch_threads_get (notmuch_threads_t *threads); > +notmuch_threads_get (notmuch_threads_t *threads, > + notmuch_bool_t include_recipients); What about adding a new function notmuch_threds_get_with_recipients() to not break public API? It would also make the sources more readable. -M. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 06/11] lib: store thread recipients in thread structure 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 07/11] test: search recipient output Jameson Graef Rollins @ 2012-08-31 20:44 ` Michal Sojka 2012-09-02 7:52 ` Mark Walters 2012-09-08 17:25 ` Austin Clements 3 siblings, 0 replies; 27+ messages in thread From: Michal Sojka @ 2012-08-31 20:44 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail On Mon, Aug 20 2012, Jameson Graef Rollins wrote: > This utilizes the new thread addresses struct to store thread > recipients, again in parallel to authors. > > Since message recipients are not stored in the database, including > recipients in the thread structure exacts a significant overhead as > the recipients are retrieved from the original message files. Because > of this, a new boolean argument, include_recipients, is added to the > necessary functions (_notmuch_thread_create, _thread_add_message and > _thread_add_matched_message) that controls whether the recipients are > fetched and included. If message recipients are ever stored in the > database this new argument could probably be removed. > --- > lib/notmuch-private.h | 3 +- > lib/notmuch.h | 14 +++++++++ > lib/query.cc | 3 +- > lib/thread.cc | 77 +++++++++++++++++++++++++++++++++++++------------ > 4 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h > index 27a41b6..32d1523 100644 > --- a/lib/notmuch-private.h > +++ b/lib/notmuch-private.h > @@ -232,7 +232,8 @@ _notmuch_thread_create (void *ctx, > unsigned int seed_doc_id, > notmuch_doc_id_set_t *match_set, > notmuch_string_list_t *excluded_terms, > - notmuch_sort_t sort); > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients); > > /* message.cc */ > > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 6acd38d..f9e71c1 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -759,6 +759,20 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread); > const char * > notmuch_thread_get_authors (notmuch_thread_t *thread); > > +/* Get the recipients of 'thread' > + * > + * The returned string is a comma-separated list of the names of the > + * recipients of mail messages in the query results that belong to this > + * thread. > + * > + * The returned string belongs to 'thread' and as such, should not be > + * modified by the caller and will only be valid for as long as the > + * thread is valid, (which is until notmuch_thread_destroy or until > + * the query from which it derived is destroyed). > + */ > +const char * > +notmuch_thread_get_recipients (notmuch_thread_t *thread); > + > /* Get the subject of 'thread' > * > * The subject is taken from the first message (according to the query > diff --git a/lib/query.cc b/lib/query.cc > index e9c1a2d..54833a7 100644 > --- a/lib/query.cc > +++ b/lib/query.cc > @@ -486,7 +486,8 @@ notmuch_threads_get (notmuch_threads_t *threads) > doc_id, > &threads->match_set, > threads->query->exclude_terms, > - threads->query->sort); > + threads->query->sort, > + FALSE); > } > > void > diff --git a/lib/thread.cc b/lib/thread.cc > index 757e143..baf07c2 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -37,6 +37,7 @@ struct visible _notmuch_thread { > char *thread_id; > char *subject; > notmuch_thread_addresses_t *authors; > + notmuch_thread_addresses_t *recipients; > GHashTable *tags; > > notmuch_message_list_t *message_list; > @@ -63,6 +64,7 @@ static int > _notmuch_thread_destructor (notmuch_thread_t *thread) > { > _notmuch_thread_addresses_destructor (thread->authors); > + _notmuch_thread_addresses_destructor (thread->recipients); > g_hash_table_unref (thread->tags); > g_hash_table_unref (thread->message_hash); > return 0; > @@ -204,14 +206,17 @@ _thread_cleanup_address (notmuch_thread_t *thread, > static void > _thread_add_message (notmuch_thread_t *thread, > notmuch_message_t *message, > - notmuch_string_list_t *exclude_terms) > + notmuch_string_list_t *exclude_terms, > + notmuch_bool_t include_recipients) > { > notmuch_tags_t *tags; > const char *tag; > - InternetAddressList *list = NULL; > + InternetAddressList *from_list = NULL; > + InternetAddressList *to_list = NULL; > InternetAddress *address; > const char *from, *author; > - char *clean_author; > + const char *to, *recipient; > + char *clean_address; > > _notmuch_message_list_add_message (thread->message_list, > talloc_steal (thread, message)); > @@ -223,10 +228,9 @@ _thread_add_message (notmuch_thread_t *thread, > > from = notmuch_message_get_header (message, "from"); > if (from) > - list = internet_address_list_parse_string (from); > - > - if (list) { > - address = internet_address_list_get_address (list, 0); > + from_list = internet_address_list_parse_string (from); > + if (from_list) { > + address = internet_address_list_get_address (from_list, 0); > if (address) { > author = internet_address_get_name (address); > if (author == NULL) { > @@ -234,11 +238,32 @@ _thread_add_message (notmuch_thread_t *thread, > mailbox = INTERNET_ADDRESS_MAILBOX (address); > author = internet_address_mailbox_get_addr (mailbox); > } > - clean_author = _thread_cleanup_author (thread, author, from); > - _thread_add_address (thread->authors, clean_author, FALSE); > - notmuch_message_set_author (message, clean_author); > + clean_address = _thread_cleanup_address (thread, author, from); > + _thread_add_address (thread->authors, clean_address, FALSE); > + notmuch_message_set_author (message, clean_address); > + } > + g_object_unref (G_OBJECT (from_list)); > + } > + > + if (include_recipients) { > + to = notmuch_message_get_header (message, "to"); I think that recipients should be composed from all to, cc and bcc headers. I often write emails with only Bcc header. If such an email is saved as draft, I might be surprised by not seeing the "recipients". > + if (to) > + to_list = internet_address_list_parse_string (to); > + if (to_list) { > + address = internet_address_list_get_address (to_list, 0); > + if (address) { > + recipient = internet_address_get_name (address); > + if (recipient == NULL) { > + InternetAddressMailbox *mailbox; > + mailbox = INTERNET_ADDRESS_MAILBOX (address); > + recipient = internet_address_mailbox_get_addr (mailbox); > + } > + clean_address = _thread_cleanup_address (thread, recipient, to); > + _thread_add_address (thread->recipients, clean_address, FALSE); > + notmuch_message_set_recipients (message, clean_address); > } > - g_object_unref (G_OBJECT (list)); > + g_object_unref (G_OBJECT (to_list)); > + } > } > > if (! thread->subject) { > @@ -301,7 +326,8 @@ _thread_set_subject_from_message (notmuch_thread_t *thread, > static void > _thread_add_matched_message (notmuch_thread_t *thread, > notmuch_message_t *message, > - notmuch_sort_t sort) > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients) > { > time_t date; > notmuch_message_t *hashed_message; > @@ -331,6 +357,8 @@ _thread_add_matched_message (notmuch_thread_t *thread, > } > > _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE); > + if (include_recipients) > + _thread_add_address (thread->recipients, notmuch_message_get_recipients (hashed_message), TRUE); > } > > static void > @@ -399,10 +427,10 @@ _thread_addresses_init (const void *ctx) > * > * Creating the thread will perform a database search to get all > * messages belonging to the thread and will get the first subject > - * line, the total count of messages, and all authors in the thread. > - * Each message in the thread is checked against match_set to allow > - * for a separate count of matched messages, and to allow a viewer to > - * display these messages differently. > + * line, the total count of messages, and all authors and recipients ... all authors (and optionally recipients). > + * of the thread. Each message in the thread is checked against > + * match_set to allow for a separate count of matched messages, and to > + * allow a viewer to display these messages differently. > * > * Here, 'ctx' is talloc context for the resulting thread object. > * > @@ -414,7 +442,8 @@ _notmuch_thread_create (void *ctx, > unsigned int seed_doc_id, > notmuch_doc_id_set_t *match_set, > notmuch_string_list_t *exclude_terms, > - notmuch_sort_t sort) > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients) > { > notmuch_thread_t *thread; > notmuch_message_t *seed_message; > @@ -453,6 +482,9 @@ _notmuch_thread_create (void *ctx, > thread->authors = _thread_addresses_init (thread); > if (unlikely (thread->authors == NULL)) > return NULL; > + thread->recipients = _thread_addresses_init (thread); > + if (unlikely (thread->recipients == NULL)) > + return NULL; > > thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal, > free, NULL); > @@ -486,11 +518,11 @@ _notmuch_thread_create (void *ctx, > if (doc_id == seed_doc_id) > message = seed_message; > > - _thread_add_message (thread, message, exclude_terms); > + _thread_add_message (thread, message, exclude_terms, include_recipients); > > if ( _notmuch_doc_id_set_contains (match_set, doc_id)) { > _notmuch_doc_id_set_remove (match_set, doc_id); > - _thread_add_matched_message (thread, message, sort); > + _thread_add_matched_message (thread, message, sort, include_recipients); > } > > _notmuch_message_close (message); > @@ -499,6 +531,7 @@ _notmuch_thread_create (void *ctx, > notmuch_query_destroy (thread_id_query); > > _resolve_thread_addresses_string (thread->authors); > + _resolve_thread_addresses_string (thread->recipients); > > _resolve_thread_relationships (thread); > > @@ -536,6 +569,12 @@ notmuch_thread_get_authors (notmuch_thread_t *thread) > } > > const char * > +notmuch_thread_get_recipients (notmuch_thread_t *thread) > +{ > + return thread->recipients->string; > +} > + > +const char * > notmuch_thread_get_subject (notmuch_thread_t *thread) > { > return thread->subject; > -- > 1.7.10.4 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 06/11] lib: store thread recipients in thread structure 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 07/11] test: search recipient output Jameson Graef Rollins 2012-08-31 20:44 ` [PATCH 06/11] lib: store thread recipients in thread structure Michal Sojka @ 2012-09-02 7:52 ` Mark Walters 2012-09-08 17:25 ` Austin Clements 3 siblings, 0 replies; 27+ messages in thread From: Mark Walters @ 2012-09-02 7:52 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail On Mon, 20 Aug 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > This utilizes the new thread addresses struct to store thread > recipients, again in parallel to authors. > > Since message recipients are not stored in the database, including > recipients in the thread structure exacts a significant overhead as > the recipients are retrieved from the original message files. Because > of this, a new boolean argument, include_recipients, is added to the > necessary functions (_notmuch_thread_create, _thread_add_message and > _thread_add_matched_message) that controls whether the recipients are > fetched and included. If message recipients are ever stored in the > database this new argument could probably be removed. > --- Hi I have briefly looked through the series and overall it looks good. > lib/notmuch-private.h | 3 +- > lib/notmuch.h | 14 +++++++++ > lib/query.cc | 3 +- > lib/thread.cc | 77 +++++++++++++++++++++++++++++++++++++------------ > 4 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h > index 27a41b6..32d1523 100644 > --- a/lib/notmuch-private.h > +++ b/lib/notmuch-private.h > @@ -232,7 +232,8 @@ _notmuch_thread_create (void *ctx, > unsigned int seed_doc_id, > notmuch_doc_id_set_t *match_set, > notmuch_string_list_t *excluded_terms, > - notmuch_sort_t sort); > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients); > > /* message.cc */ > > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 6acd38d..f9e71c1 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -759,6 +759,20 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread); > const char * > notmuch_thread_get_authors (notmuch_thread_t *thread); > > +/* Get the recipients of 'thread' > + * > + * The returned string is a comma-separated list of the names of the > + * recipients of mail messages in the query results that belong to this > + * thread. > + * > + * The returned string belongs to 'thread' and as such, should not be > + * modified by the caller and will only be valid for as long as the > + * thread is valid, (which is until notmuch_thread_destroy or until > + * the query from which it derived is destroyed). > + */ > +const char * > +notmuch_thread_get_recipients (notmuch_thread_t *thread); > + > /* Get the subject of 'thread' > * > * The subject is taken from the first message (according to the query > diff --git a/lib/query.cc b/lib/query.cc > index e9c1a2d..54833a7 100644 > --- a/lib/query.cc > +++ b/lib/query.cc > @@ -486,7 +486,8 @@ notmuch_threads_get (notmuch_threads_t *threads) > doc_id, > &threads->match_set, > threads->query->exclude_terms, > - threads->query->sort); > + threads->query->sort, > + FALSE); > } > > void > diff --git a/lib/thread.cc b/lib/thread.cc > index 757e143..baf07c2 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -37,6 +37,7 @@ struct visible _notmuch_thread { > char *thread_id; > char *subject; > notmuch_thread_addresses_t *authors; > + notmuch_thread_addresses_t *recipients; > GHashTable *tags; > > notmuch_message_list_t *message_list; > @@ -63,6 +64,7 @@ static int > _notmuch_thread_destructor (notmuch_thread_t *thread) > { > _notmuch_thread_addresses_destructor (thread->authors); > + _notmuch_thread_addresses_destructor (thread->recipients); > g_hash_table_unref (thread->tags); > g_hash_table_unref (thread->message_hash); > return 0; > @@ -204,14 +206,17 @@ _thread_cleanup_address (notmuch_thread_t *thread, > static void > _thread_add_message (notmuch_thread_t *thread, > notmuch_message_t *message, > - notmuch_string_list_t *exclude_terms) > + notmuch_string_list_t *exclude_terms, > + notmuch_bool_t include_recipients) > { > notmuch_tags_t *tags; > const char *tag; > - InternetAddressList *list = NULL; > + InternetAddressList *from_list = NULL; > + InternetAddressList *to_list = NULL; > InternetAddress *address; > const char *from, *author; > - char *clean_author; > + const char *to, *recipient; > + char *clean_address; > > _notmuch_message_list_add_message (thread->message_list, > talloc_steal (thread, message)); > @@ -223,10 +228,9 @@ _thread_add_message (notmuch_thread_t *thread, > > from = notmuch_message_get_header (message, "from"); > if (from) > - list = internet_address_list_parse_string (from); > - > - if (list) { > - address = internet_address_list_get_address (list, 0); > + from_list = internet_address_list_parse_string (from); > + if (from_list) { > + address = internet_address_list_get_address (from_list, 0); > if (address) { > author = internet_address_get_name (address); > if (author == NULL) { > @@ -234,11 +238,32 @@ _thread_add_message (notmuch_thread_t *thread, > mailbox = INTERNET_ADDRESS_MAILBOX (address); > author = internet_address_mailbox_get_addr (mailbox); > } > - clean_author = _thread_cleanup_author (thread, author, from); > - _thread_add_address (thread->authors, clean_author, FALSE); > - notmuch_message_set_author (message, clean_author); > + clean_address = _thread_cleanup_address (thread, author, from); > + _thread_add_address (thread->authors, clean_address, FALSE); > + notmuch_message_set_author (message, clean_address); > + } > + g_object_unref (G_OBJECT (from_list)); > + } > + > + if (include_recipients) { > + to = notmuch_message_get_header (message, "to"); > + if (to) > + to_list = internet_address_list_parse_string (to); > + if (to_list) { > + address = internet_address_list_get_address (to_list, 0); I think you are just adding the first address on the to: line. Is that deliberate? The comment before notmuch_thread_get_recipients (above) suggests all recipients are being added, but doing so may make the string too large to be useful. Best wishes Mark > + if (address) { > + recipient = internet_address_get_name (address); > + if (recipient == NULL) { > + InternetAddressMailbox *mailbox; > + mailbox = INTERNET_ADDRESS_MAILBOX (address); > + recipient = internet_address_mailbox_get_addr (mailbox); > + } > + clean_address = _thread_cleanup_address (thread, recipient, to); > + _thread_add_address (thread->recipients, clean_address, FALSE); > + notmuch_message_set_recipients (message, clean_address); > } > - g_object_unref (G_OBJECT (list)); > + g_object_unref (G_OBJECT (to_list)); > + } > } > > if (! thread->subject) { > @@ -301,7 +326,8 @@ _thread_set_subject_from_message (notmuch_thread_t *thread, > static void > _thread_add_matched_message (notmuch_thread_t *thread, > notmuch_message_t *message, > - notmuch_sort_t sort) > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients) > { > time_t date; > notmuch_message_t *hashed_message; > @@ -331,6 +357,8 @@ _thread_add_matched_message (notmuch_thread_t *thread, > } > > _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE); > + if (include_recipients) > + _thread_add_address (thread->recipients, notmuch_message_get_recipients (hashed_message), TRUE); > } > > static void > @@ -399,10 +427,10 @@ _thread_addresses_init (const void *ctx) > * > * Creating the thread will perform a database search to get all > * messages belonging to the thread and will get the first subject > - * line, the total count of messages, and all authors in the thread. > - * Each message in the thread is checked against match_set to allow > - * for a separate count of matched messages, and to allow a viewer to > - * display these messages differently. > + * line, the total count of messages, and all authors and recipients > + * of the thread. Each message in the thread is checked against > + * match_set to allow for a separate count of matched messages, and to > + * allow a viewer to display these messages differently. > * > * Here, 'ctx' is talloc context for the resulting thread object. > * > @@ -414,7 +442,8 @@ _notmuch_thread_create (void *ctx, > unsigned int seed_doc_id, > notmuch_doc_id_set_t *match_set, > notmuch_string_list_t *exclude_terms, > - notmuch_sort_t sort) > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients) > { > notmuch_thread_t *thread; > notmuch_message_t *seed_message; > @@ -453,6 +482,9 @@ _notmuch_thread_create (void *ctx, > thread->authors = _thread_addresses_init (thread); > if (unlikely (thread->authors == NULL)) > return NULL; > + thread->recipients = _thread_addresses_init (thread); > + if (unlikely (thread->recipients == NULL)) > + return NULL; > > thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal, > free, NULL); > @@ -486,11 +518,11 @@ _notmuch_thread_create (void *ctx, > if (doc_id == seed_doc_id) > message = seed_message; > > - _thread_add_message (thread, message, exclude_terms); > + _thread_add_message (thread, message, exclude_terms, include_recipients); > > if ( _notmuch_doc_id_set_contains (match_set, doc_id)) { > _notmuch_doc_id_set_remove (match_set, doc_id); > - _thread_add_matched_message (thread, message, sort); > + _thread_add_matched_message (thread, message, sort, include_recipients); > } > > _notmuch_message_close (message); > @@ -499,6 +531,7 @@ _notmuch_thread_create (void *ctx, > notmuch_query_destroy (thread_id_query); > > _resolve_thread_addresses_string (thread->authors); > + _resolve_thread_addresses_string (thread->recipients); > > _resolve_thread_relationships (thread); > > @@ -536,6 +569,12 @@ notmuch_thread_get_authors (notmuch_thread_t *thread) > } > > const char * > +notmuch_thread_get_recipients (notmuch_thread_t *thread) > +{ > + return thread->recipients->string; > +} > + > +const char * > notmuch_thread_get_subject (notmuch_thread_t *thread) > { > return thread->subject; > -- > 1.7.10.4 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 06/11] lib: store thread recipients in thread structure 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins ` (2 preceding siblings ...) 2012-09-02 7:52 ` Mark Walters @ 2012-09-08 17:25 ` Austin Clements 3 siblings, 0 replies; 27+ messages in thread From: Austin Clements @ 2012-09-08 17:25 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm: > This utilizes the new thread addresses struct to store thread > recipients, again in parallel to authors. > > Since message recipients are not stored in the database, including > recipients in the thread structure exacts a significant overhead as > the recipients are retrieved from the original message files. Because > of this, a new boolean argument, include_recipients, is added to the > necessary functions (_notmuch_thread_create, _thread_add_message and > _thread_add_matched_message) that controls whether the recipients are > fetched and included. If message recipients are ever stored in the > database this new argument could probably be removed. > --- > lib/notmuch-private.h | 3 +- > lib/notmuch.h | 14 +++++++++ > lib/query.cc | 3 +- > lib/thread.cc | 77 +++++++++++++++++++++++++++++++++++++------------ > 4 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h > index 27a41b6..32d1523 100644 > --- a/lib/notmuch-private.h > +++ b/lib/notmuch-private.h > @@ -232,7 +232,8 @@ _notmuch_thread_create (void *ctx, > unsigned int seed_doc_id, > notmuch_doc_id_set_t *match_set, > notmuch_string_list_t *excluded_terms, > - notmuch_sort_t sort); > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients); > > /* message.cc */ > > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 6acd38d..f9e71c1 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -759,6 +759,20 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread); > const char * > notmuch_thread_get_authors (notmuch_thread_t *thread); > > +/* Get the recipients of 'thread' > + * > + * The returned string is a comma-separated list of the names of the > + * recipients of mail messages in the query results that belong to this > + * thread. > + * > + * The returned string belongs to 'thread' and as such, should not be > + * modified by the caller and will only be valid for as long as the > + * thread is valid, (which is until notmuch_thread_destroy or until > + * the query from which it derived is destroyed). > + */ > +const char * > +notmuch_thread_get_recipients (notmuch_thread_t *thread); > + > /* Get the subject of 'thread' > * > * The subject is taken from the first message (according to the query > diff --git a/lib/query.cc b/lib/query.cc > index e9c1a2d..54833a7 100644 > --- a/lib/query.cc > +++ b/lib/query.cc > @@ -486,7 +486,8 @@ notmuch_threads_get (notmuch_threads_t *threads) > doc_id, > &threads->match_set, > threads->query->exclude_terms, > - threads->query->sort); > + threads->query->sort, > + FALSE); > } > > void > diff --git a/lib/thread.cc b/lib/thread.cc > index 757e143..baf07c2 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -37,6 +37,7 @@ struct visible _notmuch_thread { > char *thread_id; > char *subject; > notmuch_thread_addresses_t *authors; > + notmuch_thread_addresses_t *recipients; > GHashTable *tags; > > notmuch_message_list_t *message_list; > @@ -63,6 +64,7 @@ static int > _notmuch_thread_destructor (notmuch_thread_t *thread) > { > _notmuch_thread_addresses_destructor (thread->authors); > + _notmuch_thread_addresses_destructor (thread->recipients); Same thing about not calling this explicitly. > g_hash_table_unref (thread->tags); > g_hash_table_unref (thread->message_hash); > return 0; > @@ -204,14 +206,17 @@ _thread_cleanup_address (notmuch_thread_t *thread, > static void > _thread_add_message (notmuch_thread_t *thread, > notmuch_message_t *message, > - notmuch_string_list_t *exclude_terms) > + notmuch_string_list_t *exclude_terms, > + notmuch_bool_t include_recipients) Could we instead omit this argument and construct the recipients list when notmuch_thread_get_recipients is called for the first time? That makes things a little harder here, because you'll have to keep a list of messages in the order they were added and scan it on demand (and use the matched flag), but it will avoid the public API major version bump as well as generally minimizing interface complexity. Also, I think that list will prove valuable for other things in the future, since currently there's no way to iterate over the messages in a thread in date order; the best you can do is recursive walk the thread structure. Conveniently, we already have a message list abstraction. > { > notmuch_tags_t *tags; > const char *tag; > - InternetAddressList *list = NULL; > + InternetAddressList *from_list = NULL; > + InternetAddressList *to_list = NULL; > InternetAddress *address; > const char *from, *author; > - char *clean_author; > + const char *to, *recipient; > + char *clean_address; > > _notmuch_message_list_add_message (thread->message_list, > talloc_steal (thread, message)); > @@ -223,10 +228,9 @@ _thread_add_message (notmuch_thread_t *thread, > > from = notmuch_message_get_header (message, "from"); > if (from) > - list = internet_address_list_parse_string (from); > - > - if (list) { > - address = internet_address_list_get_address (list, 0); > + from_list = internet_address_list_parse_string (from); > + if (from_list) { > + address = internet_address_list_get_address (from_list, 0); > if (address) { > author = internet_address_get_name (address); > if (author == NULL) { > @@ -234,11 +238,32 @@ _thread_add_message (notmuch_thread_t *thread, > mailbox = INTERNET_ADDRESS_MAILBOX (address); > author = internet_address_mailbox_get_addr (mailbox); > } > - clean_author = _thread_cleanup_author (thread, author, from); > - _thread_add_address (thread->authors, clean_author, FALSE); > - notmuch_message_set_author (message, clean_author); > + clean_address = _thread_cleanup_address (thread, author, from); > + _thread_add_address (thread->authors, clean_address, FALSE); > + notmuch_message_set_author (message, clean_address); > + } > + g_object_unref (G_OBJECT (from_list)); > + } > + > + if (include_recipients) { > + to = notmuch_message_get_header (message, "to"); > + if (to) > + to_list = internet_address_list_parse_string (to); > + if (to_list) { > + address = internet_address_list_get_address (to_list, 0); > + if (address) { > + recipient = internet_address_get_name (address); > + if (recipient == NULL) { > + InternetAddressMailbox *mailbox; > + mailbox = INTERNET_ADDRESS_MAILBOX (address); > + recipient = internet_address_mailbox_get_addr (mailbox); > + } > + clean_address = _thread_cleanup_address (thread, recipient, to); > + _thread_add_address (thread->recipients, clean_address, FALSE); > + notmuch_message_set_recipients (message, clean_address); > } > - g_object_unref (G_OBJECT (list)); > + g_object_unref (G_OBJECT (to_list)); > + } > } > > if (! thread->subject) { > @@ -301,7 +326,8 @@ _thread_set_subject_from_message (notmuch_thread_t *thread, > static void > _thread_add_matched_message (notmuch_thread_t *thread, > notmuch_message_t *message, > - notmuch_sort_t sort) > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients) > { > time_t date; > notmuch_message_t *hashed_message; > @@ -331,6 +357,8 @@ _thread_add_matched_message (notmuch_thread_t *thread, > } > > _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE); > + if (include_recipients) > + _thread_add_address (thread->recipients, notmuch_message_get_recipients (hashed_message), TRUE); > } > > static void > @@ -399,10 +427,10 @@ _thread_addresses_init (const void *ctx) > * > * Creating the thread will perform a database search to get all > * messages belonging to the thread and will get the first subject > - * line, the total count of messages, and all authors in the thread. > - * Each message in the thread is checked against match_set to allow > - * for a separate count of matched messages, and to allow a viewer to > - * display these messages differently. > + * line, the total count of messages, and all authors and recipients > + * of the thread. Each message in the thread is checked against > + * match_set to allow for a separate count of matched messages, and to > + * allow a viewer to display these messages differently. > * > * Here, 'ctx' is talloc context for the resulting thread object. > * > @@ -414,7 +442,8 @@ _notmuch_thread_create (void *ctx, > unsigned int seed_doc_id, > notmuch_doc_id_set_t *match_set, > notmuch_string_list_t *exclude_terms, > - notmuch_sort_t sort) > + notmuch_sort_t sort, > + notmuch_bool_t include_recipients) > { > notmuch_thread_t *thread; > notmuch_message_t *seed_message; > @@ -453,6 +482,9 @@ _notmuch_thread_create (void *ctx, > thread->authors = _thread_addresses_init (thread); > if (unlikely (thread->authors == NULL)) > return NULL; > + thread->recipients = _thread_addresses_init (thread); > + if (unlikely (thread->recipients == NULL)) > + return NULL; > > thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal, > free, NULL); > @@ -486,11 +518,11 @@ _notmuch_thread_create (void *ctx, > if (doc_id == seed_doc_id) > message = seed_message; > > - _thread_add_message (thread, message, exclude_terms); > + _thread_add_message (thread, message, exclude_terms, include_recipients); > > if ( _notmuch_doc_id_set_contains (match_set, doc_id)) { > _notmuch_doc_id_set_remove (match_set, doc_id); > - _thread_add_matched_message (thread, message, sort); > + _thread_add_matched_message (thread, message, sort, include_recipients); > } > > _notmuch_message_close (message); > @@ -499,6 +531,7 @@ _notmuch_thread_create (void *ctx, > notmuch_query_destroy (thread_id_query); > > _resolve_thread_addresses_string (thread->authors); > + _resolve_thread_addresses_string (thread->recipients); > > _resolve_thread_relationships (thread); > > @@ -536,6 +569,12 @@ notmuch_thread_get_authors (notmuch_thread_t *thread) > } > > const char * > +notmuch_thread_get_recipients (notmuch_thread_t *thread) > +{ > + return thread->recipients->string; > +} > + > +const char * > notmuch_thread_get_subject (notmuch_thread_t *thread) > { > return thread->subject; -- Austin Clements MIT/'06/PhD/CSAIL amdragon@mit.edu http://web.mit.edu/amdragon Somewhere in the dream we call reality you will find me, searching for the reality we call dreams. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 05/11] lib: add ability to store recipients in message structure 2012-08-20 1:52 ` [PATCH 05/11] lib: add ability to store recipients in message structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins @ 2012-08-31 20:19 ` Michal Sojka 2012-09-08 17:24 ` Austin Clements 2 siblings, 0 replies; 27+ messages in thread From: Michal Sojka @ 2012-08-31 20:19 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail Hi Jamie, continuing in the review from yesterday... On Mon, Aug 20 2012, Jameson Graef Rollins wrote: [...] > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h > index bfb4111..27a41b6 100644 > --- a/lib/notmuch-private.h > +++ b/lib/notmuch-private.h > @@ -325,6 +325,15 @@ notmuch_message_set_author (notmuch_message_t *message, const char *author); > const char * > notmuch_message_get_author (notmuch_message_t *message); > > +/* Set the recipients of 'message' - this is the representation used > + * when displaying the message */ > +void > +notmuch_message_set_recipients (notmuch_message_t *message, const char *recipients); > + > +/* Get the authors of 'message' */ s/authors/recipients > +const char * > +notmuch_message_get_recipients (notmuch_message_t *message); > + > > /* index.cc */ > > -- > 1.7.10.4 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 05/11] lib: add ability to store recipients in message structure 2012-08-20 1:52 ` [PATCH 05/11] lib: add ability to store recipients in message structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins 2012-08-31 20:19 ` [PATCH 05/11] lib: add ability to store recipients in message structure Michal Sojka @ 2012-09-08 17:24 ` Austin Clements 2 siblings, 0 replies; 27+ messages in thread From: Austin Clements @ 2012-09-08 17:24 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm: > Added as a string, in parallel to the authors element. > --- > lib/message.cc | 18 ++++++++++++++++++ > lib/notmuch-private.h | 9 +++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/lib/message.cc b/lib/message.cc > index 978de06..fa28073 100644 > --- a/lib/message.cc > +++ b/lib/message.cc > @@ -36,6 +36,7 @@ struct visible _notmuch_message { > notmuch_string_list_t *filename_term_list; > notmuch_string_list_t *filename_list; > char *author; > + char *recipients; > notmuch_message_file_t *message_file; > notmuch_message_list_t *replies; > unsigned long flags; > @@ -109,6 +110,7 @@ _notmuch_message_create_for_document (const void *talloc_owner, > message->filename_list = NULL; > message->message_file = NULL; > message->author = NULL; > + message->recipients = NULL; > > message->replies = _notmuch_message_list_create (message); > if (unlikely (message->replies == NULL)) { > @@ -808,6 +810,22 @@ notmuch_message_set_author (notmuch_message_t *message, > return; > } > > +const char * > +notmuch_message_get_recipients (notmuch_message_t *message) > +{ > + return message->recipients; > +} > + > +void > +notmuch_message_set_recipients (notmuch_message_t *message, > + const char *recipients) > +{ > + if (message->recipients) > + talloc_free(message->recipients); > + message->recipients = talloc_strdup(message, recipients); Missing spaces before parameter lists. > + return; > +} > + > void > _notmuch_message_set_header_values (notmuch_message_t *message, > const char *date, > diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h > index bfb4111..27a41b6 100644 > --- a/lib/notmuch-private.h > +++ b/lib/notmuch-private.h > @@ -325,6 +325,15 @@ notmuch_message_set_author (notmuch_message_t *message, const char *author); > const char * > notmuch_message_get_author (notmuch_message_t *message); > > +/* Set the recipients of 'message' - this is the representation used > + * when displaying the message */ > +void > +notmuch_message_set_recipients (notmuch_message_t *message, const char *recipients); > + > +/* Get the authors of 'message' */ > +const char * > +notmuch_message_get_recipients (notmuch_message_t *message); > + > > /* index.cc */ > -- Austin Clements MIT/'06/PhD/CSAIL amdragon@mit.edu http://web.mit.edu/amdragon Somewhere in the dream we call reality you will find me, searching for the reality we call dreams. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 03/11] lib: give _thread_cleanup_author a more generic name 2012-08-20 1:52 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 04/11] lib: remove no longer needed author-specific thread functions Jameson Graef Rollins @ 2012-09-08 17:25 ` Austin Clements 1 sibling, 0 replies; 27+ messages in thread From: Austin Clements @ 2012-09-08 17:25 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Should this patch also rename calls to this function? Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm: > We will use this for cleaning non-author address fields, so we give it > the more generic name _thread_cleanup_address. > --- > lib/thread.cc | 43 ++++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/lib/thread.cc b/lib/thread.cc > index 9e0e5cb..b53ccb0 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -242,48 +242,49 @@ _resolve_thread_authors_string (notmuch_thread_t *thread) > * "Last, First MI" <first.mi.last@company.com> > */ > static char * > -_thread_cleanup_author (notmuch_thread_t *thread, > - const char *author, const char *from) > +_thread_cleanup_address (notmuch_thread_t *thread, > + const char *address, > + const char *original) > { > - char *clean_author,*test_author; > + char *clean_address,*test_address; > const char *comma; > char *blank; > int fname,lname; > > - if (author == NULL) > + if (address == NULL) > return NULL; > - clean_author = talloc_strdup(thread, author); > - if (clean_author == NULL) > + clean_address = talloc_strdup(thread, address); > + if (clean_address == NULL) > return NULL; > /* check if there's a comma in the name and that there's a > * component of the name behind it (so the name doesn't end with > * the comma - in which case the string that strchr finds is just > * one character long ",\0"). > - * Otherwise just return the copy of the original author name that > + * Otherwise just return the copy of the original address name that > * we just made*/ > - comma = strchr(author,','); > + comma = strchr(address,','); > if (comma && strlen(comma) > 1) { > /* let's assemble what we think is the correct name */ > - lname = comma - author; > - fname = strlen(author) - lname - 2; > - strncpy(clean_author, comma + 2, fname); > - *(clean_author+fname) = ' '; > - strncpy(clean_author + fname + 1, author, lname); > - *(clean_author+fname+1+lname) = '\0'; > + lname = comma - address; > + fname = strlen(address) - lname - 2; > + strncpy(clean_address, comma + 2, fname); > + *(clean_address+fname) = ' '; > + strncpy(clean_address + fname + 1, address, lname); > + *(clean_address+fname+1+lname) = '\0'; > /* make a temporary copy and see if it matches the email */ > - test_author = talloc_strdup(thread,clean_author); > + test_address = talloc_strdup(thread,clean_address); > > - blank=strchr(test_author,' '); > + blank=strchr(test_address,' '); > while (blank != NULL) { > *blank = '.'; > - blank=strchr(test_author,' '); > + blank=strchr(test_address,' '); > } > - if (strcasestr(from, test_author) == NULL) > + if (strcasestr(original, test_address) == NULL) > /* we didn't identify this as part of the email address > - * so let's punt and return the original author */ > - strcpy (clean_author, author); > + * so let's punt and return the original address */ > + strcpy (clean_address, address); > } > - return clean_author; > + return clean_address; > } > > /* Add 'message' as a message that belongs to 'thread'. -- Austin Clements MIT/'06/PhD/CSAIL amdragon@mit.edu http://web.mit.edu/amdragon Somewhere in the dream we call reality you will find me, searching for the reality we call dreams. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 02/11] lib: use new addresses structure for thread authors 2012-08-20 1:52 ` [PATCH 02/11] lib: use new addresses structure for thread authors Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Jameson Graef Rollins @ 2012-09-08 17:24 ` Austin Clements 1 sibling, 0 replies; 27+ messages in thread From: Austin Clements @ 2012-09-08 17:24 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm: > Now that we have the infrastructure in place, we modify the thread > object and associated functions to use the new addresses structure for > storing thread authors. > --- > lib/thread.cc | 41 +++++++++++------------------------------ > 1 file changed, 11 insertions(+), 30 deletions(-) > > diff --git a/lib/thread.cc b/lib/thread.cc > index 7af9eeb..9e0e5cb 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -36,11 +36,7 @@ struct visible _notmuch_thread { > notmuch_database_t *notmuch; > char *thread_id; > char *subject; > - GHashTable *authors_hash; > - GPtrArray *authors_array; > - GHashTable *matched_authors_hash; > - GPtrArray *matched_authors_array; > - char *authors; > + notmuch_thread_addresses_t *authors; > GHashTable *tags; > > notmuch_message_list_t *message_list; > @@ -66,21 +62,9 @@ _notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses) > static int > _notmuch_thread_destructor (notmuch_thread_t *thread) > { > - g_hash_table_unref (thread->authors_hash); > - g_hash_table_unref (thread->matched_authors_hash); > + _notmuch_thread_addresses_destructor (thread->authors); This is the explicit call I mentioned in my talloc destructor comment on patch 1. > g_hash_table_unref (thread->tags); > g_hash_table_unref (thread->message_hash); > - > - if (thread->authors_array) { > - g_ptr_array_free (thread->authors_array, TRUE); > - thread->authors_array = NULL; > - } > - > - if (thread->matched_authors_array) { > - g_ptr_array_free (thread->matched_authors_array, TRUE); > - thread->matched_authors_array = NULL; > - } > - > return 0; > } > > @@ -341,7 +325,7 @@ _thread_add_message (notmuch_thread_t *thread, > author = internet_address_mailbox_get_addr (mailbox); > } > clean_author = _thread_cleanup_author (thread, author, from); > - _thread_add_author (thread, clean_author); > + _thread_add_address (thread->authors, clean_author, FALSE); > notmuch_message_set_author (message, clean_author); > } > g_object_unref (G_OBJECT (list)); > @@ -436,7 +420,7 @@ _thread_add_matched_message (notmuch_thread_t *thread, > NOTMUCH_MESSAGE_FLAG_MATCH, 1); > } > > - _thread_add_matched_author (thread, notmuch_message_get_author (hashed_message)); > + _thread_add_address (thread->authors, notmuch_message_get_author (hashed_message), TRUE); > } > > static void > @@ -555,14 +539,11 @@ _notmuch_thread_create (void *ctx, > thread->notmuch = notmuch; > thread->thread_id = talloc_strdup (thread, thread_id); > thread->subject = NULL; > - thread->authors_hash = g_hash_table_new_full (g_str_hash, g_str_equal, > - NULL, NULL); > - thread->authors_array = g_ptr_array_new (); > - thread->matched_authors_hash = g_hash_table_new_full (g_str_hash, > - g_str_equal, > - NULL, NULL); > - thread->matched_authors_array = g_ptr_array_new (); > - thread->authors = NULL; > + > + thread->authors = _thread_addresses_init (thread); > + if (unlikely (thread->authors == NULL)) > + return NULL; > + > thread->tags = g_hash_table_new_full (g_str_hash, g_str_equal, > free, NULL); > > @@ -607,7 +588,7 @@ _notmuch_thread_create (void *ctx, > > notmuch_query_destroy (thread_id_query); > > - _resolve_thread_authors_string (thread); > + _resolve_thread_addresses_string (thread->authors); If you make my suggested change to the _resolve_thread_addresses_string API, this call would simply go away... > > _resolve_thread_relationships (thread); > > @@ -641,7 +622,7 @@ notmuch_thread_get_matched_messages (notmuch_thread_t *thread) > const char * > notmuch_thread_get_authors (notmuch_thread_t *thread) > { > - return thread->authors; > + return thread->authors->string; ... and this would be return _thread_addresses_to_string (or whatever). > } > > const char * -- Austin Clements MIT/'06/PhD/CSAIL amdragon@mit.edu http://web.mit.edu/amdragon Somewhere in the dream we call reality you will find me, searching for the reality we call dreams. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 01/11] lib: new thread addresses structure 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 02/11] lib: use new addresses structure for thread authors Jameson Graef Rollins @ 2012-08-30 15:38 ` Michal Sojka 2012-08-30 16:33 ` Jameson Graef Rollins 2012-09-08 17:24 ` Austin Clements 2 siblings, 1 reply; 27+ messages in thread From: Michal Sojka @ 2012-08-30 15:38 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail Hi Jameson, some comments below. On Mon, Aug 20 2012, Jameson Graef Rollins wrote: > This new structure holds addresses associated with a thread, both > matched and unmatched. Initially this will be used to replace the > existing infrastructure for storing the addresses of thread authors. > Further patches will use it to store the addresses of threads > recipients. > > Init and destructor functions are included, as well as a function to > add addresses to a struct, either "matched" or not. > --- > lib/notmuch.h | 1 + > lib/thread.cc | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 117 insertions(+) > > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 3633bed..6acd38d 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -118,6 +118,7 @@ typedef struct _notmuch_database notmuch_database_t; > typedef struct _notmuch_query notmuch_query_t; > typedef struct _notmuch_threads notmuch_threads_t; > typedef struct _notmuch_thread notmuch_thread_t; > +typedef struct _notmuch_thread_addresses notmuch_thread_addresses_t; > typedef struct _notmuch_messages notmuch_messages_t; > typedef struct _notmuch_message notmuch_message_t; > typedef struct _notmuch_tags notmuch_tags_t; > diff --git a/lib/thread.cc b/lib/thread.cc > index e976d64..7af9eeb 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -24,6 +24,14 @@ > #include <gmime/gmime.h> > #include <glib.h> /* GHashTable */ > > +struct visible _notmuch_thread_addresses { > + GHashTable *matched_hash; > + GPtrArray *matched_array; > + GHashTable *unmatched_hash; > + GPtrArray *unmatched_array; > + char *string; > +}; > + > struct visible _notmuch_thread { > notmuch_database_t *notmuch; > char *thread_id; > @@ -44,6 +52,18 @@ struct visible _notmuch_thread { > }; > > static int > +_notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses) > +{ > + g_hash_table_unref (addresses->matched_hash); > + g_hash_table_unref (addresses->unmatched_hash); > + g_ptr_array_free (addresses->matched_array, TRUE); > + g_ptr_array_free (addresses->unmatched_array, TRUE); > + addresses->matched_array = NULL; > + addresses->unmatched_array = NULL; > + return 0; > +} > + > +static int > _notmuch_thread_destructor (notmuch_thread_t *thread) > { > g_hash_table_unref (thread->authors_hash); > @@ -64,6 +84,81 @@ _notmuch_thread_destructor (notmuch_thread_t *thread) > return 0; > } > > +/* Add address to a thread addresses struct. If matched is TRUE, then > + * the address will be added to the matched list.*/ > +static void > +_thread_add_address (notmuch_thread_addresses_t *addresses, > + const char *address, > + notmuch_bool_t matched) > +{ > + char *address_copy; > + GHashTable *hash; > + GPtrArray *array; > + > + if (matched) { > + hash = addresses->matched_hash; > + array = addresses->matched_array; > + } else { > + hash = addresses->unmatched_hash; > + array = addresses->unmatched_array; > + } > + > + if (address == NULL) > + return; > + > + if (g_hash_table_lookup_extended (hash, address, NULL, NULL)) > + return; > + > + address_copy = talloc_strdup (addresses, address); > + > + g_hash_table_insert (hash, address_copy, NULL); > + > + g_ptr_array_add (array, address_copy); > +} > + > +/* Construct an addresses string from matched and unmatched addresses > + * in notmuch_thread_addresses_t. The string contains matched > + * addresses first, then non-matched addresses (with the two groups > + * separated by '|'). Within each group, addresses are listed in date > + * order. */ I'd say the the addresses are listed in the order in which they have been added, which might or might not be the date order. > +static void > +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses) > +{ > + unsigned int i; > + char *address; > + int first_non_matched_address = 1; > + > + /* First, list all matched addressses in date order. */ > + for (i = 0; i < addresses->matched_array->len; i++) { > + address = (char *) g_ptr_array_index (addresses->matched_array, i); > + if (addresses->string) > + addresses->string = talloc_asprintf (addresses, "%s, %s", > + addresses->string, > + address); > + else > + addresses->string = address; > + } > + > + /* Next, append any non-matched addresses that haven't already appeared. */ > + for (i = 0; i < addresses->unmatched_array->len; i++) { > + address = (char *) g_ptr_array_index (addresses->unmatched_array, i); > + if (g_hash_table_lookup_extended (addresses->matched_hash, > + address, NULL, NULL)) > + continue; > + if (first_non_matched_address) { > + addresses->string = talloc_asprintf (addresses, "%s| %s", > + addresses->string, > + address); > + } else { > + addresses->string = talloc_asprintf (addresses, "%s, %s", > + addresses->string, > + address); > + } Simpler would be: addresses->string = talloc_asprintf (addresses, "%s%c %s", addresses->string, first_non_matched_address ? '|' : ',' address); Also, you might want to talloc_free the old address->string to not waste memory in the case of long lived notmuch_thread_addresses_t object. Or better use talloc_asprintf_append() function, which hopefully implements freeing internally. -Michal > + > + first_non_matched_address = 0; > + } > +} > + > /* Add each author of the thread to the thread's authors_hash and to > * the thread's authors_array. */ > static void > @@ -382,6 +477,27 @@ _resolve_thread_relationships (unused (notmuch_thread_t *thread)) > */ > } > > +/* Initialize a thread addresses struct. */ > +notmuch_thread_addresses_t * > +_thread_addresses_init (const void *ctx) > +{ > + notmuch_thread_addresses_t *addresses; > + > + addresses = talloc (ctx, notmuch_thread_addresses_t); > + if (unlikely (addresses == NULL)) > + return NULL; > + > + addresses->matched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, > + NULL, NULL); > + addresses->matched_array = g_ptr_array_new (); > + addresses->unmatched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, > + NULL, NULL); > + addresses->unmatched_array = g_ptr_array_new (); > + addresses->string = NULL; > + > + return addresses; > +} > + > /* Create a new notmuch_thread_t object by finding the thread > * containing the message with the given doc ID, treating any messages > * contained in match_set as "matched". Remove all messages in the > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 01/11] lib: new thread addresses structure 2012-08-30 15:38 ` [PATCH 01/11] lib: new thread addresses structure Michal Sojka @ 2012-08-30 16:33 ` Jameson Graef Rollins 0 siblings, 0 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-30 16:33 UTC (permalink / raw) To: Michal Sojka, Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 2689 bytes --] Hey, Michal. Thanks for the review. On Thu, Aug 30 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote: >> +/* Construct an addresses string from matched and unmatched addresses >> + * in notmuch_thread_addresses_t. The string contains matched >> + * addresses first, then non-matched addresses (with the two groups >> + * separated by '|'). Within each group, addresses are listed in date >> + * order. */ > > I'd say the the addresses are listed in the order in which they have > been added, which might or might not be the date order. I actually did not write the _resolve_thread_addresses_string function. I left the logic and behavior unchanged, and just modified to use the new addresses structure. If we think the logic should be changed, maybe we should do that in a separate patch. >> +static void >> +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses) >> +{ >> + unsigned int i; >> + char *address; >> + int first_non_matched_address = 1; >> + >> + /* First, list all matched addressses in date order. */ >> + for (i = 0; i < addresses->matched_array->len; i++) { >> + address = (char *) g_ptr_array_index (addresses->matched_array, i); >> + if (addresses->string) >> + addresses->string = talloc_asprintf (addresses, "%s, %s", >> + addresses->string, >> + address); >> + else >> + addresses->string = address; >> + } >> + >> + /* Next, append any non-matched addresses that haven't already appeared. */ >> + for (i = 0; i < addresses->unmatched_array->len; i++) { >> + address = (char *) g_ptr_array_index (addresses->unmatched_array, i); >> + if (g_hash_table_lookup_extended (addresses->matched_hash, >> + address, NULL, NULL)) >> + continue; >> + if (first_non_matched_address) { >> + addresses->string = talloc_asprintf (addresses, "%s| %s", >> + addresses->string, >> + address); >> + } else { >> + addresses->string = talloc_asprintf (addresses, "%s, %s", >> + addresses->string, >> + address); >> + } > > Simpler would be: > > addresses->string = talloc_asprintf (addresses, "%s%c %s", > addresses->string, > first_non_matched_address ? '|' : ',' > address); > > Also, you might want to talloc_free the old address->string to not waste > memory in the case of long lived notmuch_thread_addresses_t object. Or > better use talloc_asprintf_append() function, which hopefully implements > freeing internally. This looks like a nice simplification, but see comment above. I'll look into including these changes in the next version of the series. jamie. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 01/11] lib: new thread addresses structure 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 02/11] lib: use new addresses structure for thread authors Jameson Graef Rollins 2012-08-30 15:38 ` [PATCH 01/11] lib: new thread addresses structure Michal Sojka @ 2012-09-08 17:24 ` Austin Clements 2 siblings, 0 replies; 27+ messages in thread From: Austin Clements @ 2012-09-08 17:24 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm: > This new structure holds addresses associated with a thread, both > matched and unmatched. Initially this will be used to replace the > existing infrastructure for storing the addresses of thread authors. > Further patches will use it to store the addresses of threads > recipients. > > Init and destructor functions are included, as well as a function to > add addresses to a struct, either "matched" or not. > --- > lib/notmuch.h | 1 + > lib/thread.cc | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 117 insertions(+) > > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 3633bed..6acd38d 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -118,6 +118,7 @@ typedef struct _notmuch_database notmuch_database_t; > typedef struct _notmuch_query notmuch_query_t; > typedef struct _notmuch_threads notmuch_threads_t; > typedef struct _notmuch_thread notmuch_thread_t; > +typedef struct _notmuch_thread_addresses notmuch_thread_addresses_t; > typedef struct _notmuch_messages notmuch_messages_t; > typedef struct _notmuch_message notmuch_message_t; > typedef struct _notmuch_tags notmuch_tags_t; > diff --git a/lib/thread.cc b/lib/thread.cc > index e976d64..7af9eeb 100644 > --- a/lib/thread.cc > +++ b/lib/thread.cc > @@ -24,6 +24,14 @@ > #include <gmime/gmime.h> > #include <glib.h> /* GHashTable */ > > +struct visible _notmuch_thread_addresses { > + GHashTable *matched_hash; > + GPtrArray *matched_array; > + GHashTable *unmatched_hash; > + GPtrArray *unmatched_array; > + char *string; > +}; > + > struct visible _notmuch_thread { > notmuch_database_t *notmuch; > char *thread_id; > @@ -44,6 +52,18 @@ struct visible _notmuch_thread { > }; > > static int > +_notmuch_thread_addresses_destructor (notmuch_thread_addresses_t *addresses) > +{ > + g_hash_table_unref (addresses->matched_hash); > + g_hash_table_unref (addresses->unmatched_hash); > + g_ptr_array_free (addresses->matched_array, TRUE); > + g_ptr_array_free (addresses->unmatched_array, TRUE); The second argument should be FALSE for both of these, since the pointers contained in these pointer arrays are talloc-managed. > + addresses->matched_array = NULL; > + addresses->unmatched_array = NULL; Is this necessary? (Obviously there's no harm.) > + return 0; > +} > + > +static int > _notmuch_thread_destructor (notmuch_thread_t *thread) > { > g_hash_table_unref (thread->authors_hash); > @@ -64,6 +84,81 @@ _notmuch_thread_destructor (notmuch_thread_t *thread) > return 0; > } > > +/* Add address to a thread addresses struct. If matched is TRUE, then > + * the address will be added to the matched list.*/ > +static void > +_thread_add_address (notmuch_thread_addresses_t *addresses, _thread_addresses_add? > + const char *address, > + notmuch_bool_t matched) > +{ > + char *address_copy; > + GHashTable *hash; > + GPtrArray *array; > + > + if (matched) { > + hash = addresses->matched_hash; > + array = addresses->matched_array; > + } else { > + hash = addresses->unmatched_hash; > + array = addresses->unmatched_array; > + } > + > + if (address == NULL) > + return; > + > + if (g_hash_table_lookup_extended (hash, address, NULL, NULL)) > + return; > + > + address_copy = talloc_strdup (addresses, address); > + > + g_hash_table_insert (hash, address_copy, NULL); > + > + g_ptr_array_add (array, address_copy); > +} > + > +/* Construct an addresses string from matched and unmatched addresses > + * in notmuch_thread_addresses_t. The string contains matched > + * addresses first, then non-matched addresses (with the two groups > + * separated by '|'). Within each group, addresses are listed in date > + * order. */ > +static void > +_resolve_thread_addresses_string (notmuch_thread_addresses_t *addresses) A better API for this would be to return a const char*. On the first call (when addresses->string is NULL), construct the string and return it. On subsequent calls, just immediately return addresses->string. The approach you're using here makes sense from an incremental perspective, but since you're introducing an abstraction for thread address lists, I think it makes more sense to look at it from an API perspective. Also, the name should probably start with thread_addresses, since that's the abstraction it's part of. _thread_addresses_resolve_string? _thread_addresses_to_string? > +{ > + unsigned int i; > + char *address; > + int first_non_matched_address = 1; > + > + /* First, list all matched addressses in date order. */ Michal's comment about this not necessarily being date order applies here, too. > + for (i = 0; i < addresses->matched_array->len; i++) { > + address = (char *) g_ptr_array_index (addresses->matched_array, i); > + if (addresses->string) > + addresses->string = talloc_asprintf (addresses, "%s, %s", > + addresses->string, > + address); > + else > + addresses->string = address; > + } > + > + /* Next, append any non-matched addresses that haven't already appeared. */ > + for (i = 0; i < addresses->unmatched_array->len; i++) { > + address = (char *) g_ptr_array_index (addresses->unmatched_array, i); > + if (g_hash_table_lookup_extended (addresses->matched_hash, > + address, NULL, NULL)) > + continue; > + if (first_non_matched_address) { > + addresses->string = talloc_asprintf (addresses, "%s| %s", > + addresses->string, > + address); > + } else { > + addresses->string = talloc_asprintf (addresses, "%s, %s", > + addresses->string, > + address); > + } I second Michal's comments on this code; especially the one about using talloc_asprintf_append, even if you don't combine the two branches. Currently this code leaks O(n^2) memory (in a talloc context, so it's not permanent, but it's still unfortunate). > + > + first_non_matched_address = 0; > + } > +} > + > /* Add each author of the thread to the thread's authors_hash and to > * the thread's authors_array. */ > static void > @@ -382,6 +477,27 @@ _resolve_thread_relationships (unused (notmuch_thread_t *thread)) > */ > } > > +/* Initialize a thread addresses struct. */ > +notmuch_thread_addresses_t * > +_thread_addresses_init (const void *ctx) This should be "create" instead of "init". Also, this should probably be static, given that everything else is. If there's reason to make it non-static, then it should also start with _notmuch and have a prototype in notmuch-private.h. > +{ > + notmuch_thread_addresses_t *addresses; > + > + addresses = talloc (ctx, notmuch_thread_addresses_t); > + if (unlikely (addresses == NULL)) > + return NULL; You should set _notmuch_thread_addresses_destructor as the talloc destructor here, rather than calling it by hand from _notmuch_thread_destructor (in patch 2). > + > + addresses->matched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, > + NULL, NULL); > + addresses->matched_array = g_ptr_array_new (); > + addresses->unmatched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, > + NULL, NULL); > + addresses->unmatched_array = g_ptr_array_new (); > + addresses->string = NULL; > + > + return addresses; > +} > + > /* Create a new notmuch_thread_t object by finding the thread > * containing the message with the given doc ID, treating any messages > * contained in match_set as "matched". Remove all messages in the ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/11] add recipients to search output 2012-08-20 1:52 [PATCH 00/11] add recipients to search output Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins @ 2012-08-22 20:43 ` Jameson Graef Rollins 2012-08-23 7:21 ` Tomi Ollila 2012-09-08 17:23 ` Austin Clements 3 siblings, 0 replies; 27+ messages in thread From: Jameson Graef Rollins @ 2012-08-22 20:43 UTC (permalink / raw) To: Notmuch Mail [-- Attachment #1: Type: text/plain, Size: 225 bytes --] Apparently a recent addition to master is having a bad interaction with this series that breaks notmuch-reply. I'll try to fix and send a new version. In the mean time, comments on this series would be appreciated. jamie. [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/11] add recipients to search output 2012-08-20 1:52 [PATCH 00/11] add recipients to search output Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins 2012-08-22 20:43 ` [PATCH 00/11] add recipients to search output Jameson Graef Rollins @ 2012-08-23 7:21 ` Tomi Ollila 2012-09-08 17:23 ` Austin Clements 3 siblings, 0 replies; 27+ messages in thread From: Tomi Ollila @ 2012-08-23 7:21 UTC (permalink / raw) To: Jameson Graef Rollins, Notmuch Mail On Mon, Aug 20 2012, Jameson Graef Rollins wrote: > This series is an attempt to add thread recipients to the search > output. > > My personal overall goal of this series is to support the handling of > drafts in the emacs ui. For drafts we want to see recipients, instead > of authors, in the search output. I can imagine other uses for this > series as well, though. > > The first four patches generalize the author list handling in thread > objects to handle any address list. These patches could be applied > regardless of if the rest of the series is accepted. > > After that we modify the thread constructor such that it can hold > thread recipients as well. Since there is overhead in retrieving > thread recipients from the message files (recipients are not stored in > the database) this is handled with a switch. > > Further patches add the new switch to the search CLI that adds thread > recipients to the structured output formats. I didn't modify the text > output format, since there is no way to extend it. I can imagine > tweaking the text output such that the author field is instead > replaced by the recipients (as is done for the emacs UI at the end of > the series), but that's not done here. > > In the emacs UI, I add a new toggle function that will toggle display > of thread authors or recipients in the 'authors' field of the search > output. It's unfortunate that this ambiguity in that field name > remains, but I didn't know how to change that cleanly. I'm working on > some tests for the new emacs functionality that I'll include in the > inevitable v2 of this series. I did not read much of this introduction before browsing to the code, I was about to comment whether attempt yo do less trivial tests are to be done. > The last patch is mostly just a tickle to suggest adding the > recipients to the database. It would make the --include-recipient > searches much faster of course, but it might be overhead in the > database that folks aren't interested in. I got tickled... adding To (and Cc?!) to the database would also give (future notmuch?) address completion more addresses to match for. We should discuss whether to add other headers too? IIRC someone (Austin?) mentioned that everything (except Received:) headers could be there ? > As always, feedback, review, and comments are much appreciated. Overall, the code looks good (to me). > jamie. Tomi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/11] add recipients to search output 2012-08-20 1:52 [PATCH 00/11] add recipients to search output Jameson Graef Rollins ` (2 preceding siblings ...) 2012-08-23 7:21 ` Tomi Ollila @ 2012-09-08 17:23 ` Austin Clements 3 siblings, 0 replies; 27+ messages in thread From: Austin Clements @ 2012-09-08 17:23 UTC (permalink / raw) To: Jameson Graef Rollins; +Cc: Notmuch Mail Quoth Jameson Graef Rollins on Aug 19 at 6:52 pm: > This series is an attempt to add thread recipients to the search > output. > > My personal overall goal of this series is to support the handling of > drafts in the emacs ui. For drafts we want to see recipients, instead > of authors, in the search output. I can imagine other uses for this > series as well, though. > > The first four patches generalize the author list handling in thread > objects to handle any address list. These patches could be applied > regardless of if the rest of the series is accepted. > > After that we modify the thread constructor such that it can hold > thread recipients as well. Since there is overhead in retrieving > thread recipients from the message files (recipients are not stored in > the database) this is handled with a switch. > > Further patches add the new switch to the search CLI that adds thread > recipients to the structured output formats. I didn't modify the text > output format, since there is no way to extend it. I can imagine > tweaking the text output such that the author field is instead > replaced by the recipients (as is done for the emacs UI at the end of > the series), but that's not done here. I've gotten up through patch 8. Overall I really like this series and the abstractions you're introducing. However, I don't think you should replicate the way the authors list is handled in the CLI. The authors list is a kludge inherited from the text format, which had to invent a syntax for lists, that somehow got baked into the library. JSON, on the other hand, is very good at lists, and should use them for the recipients (I would love if it used them for the authors list, but that'll require an incompatible change, so I'd like to implement my/some JSON versioning scheme first). After all, "The string is a stark data structure and everywhere it is passed there is much duplication of process. It is a perfect vehicle for hiding information." I think treating recipients as a first-class list would lead to a much cleaner and more general API; it would hard-code less in the library, though it would put more responsibility on the CLI. What I'm imagining is just a single new public API function: notmuch_thread_get_messages, which returns a notmuch_messages_t of the thread's messages, in the original query's sort order. It's remarkable that we *don't* have this API already. In this case, it would give the library user (the CLI) the ability to easily construct the recipients list however it wants. JSON list or text string? No problem. Just to or to/cc/bcc? No problem. Separating matched and unmatched or merging them together? No problem. Also, since this would naturally construct the recipients list only when needed, we wouldn't have to worry about telling the library whether or not to pay the performance cost, and we could trivially add the headers we end up using to the database later and get an automatic speed boost. Relative to your current code, this would require either duplicating a bit of the matched/unmatched hash table code in the CLI (not perfect, but you wouldn't need the stringifying function, and there isn't much other code to that) or moving the thread_addresses abstraction into util. _thread_cleanup_author would probably also want to move into util (which is also completely reasonable since it's a pure leaf function that depends on nothing). > In the emacs UI, I add a new toggle function that will toggle display > of thread authors or recipients in the 'authors' field of the search > output. It's unfortunate that this ambiguity in that field name > remains, but I didn't know how to change that cleanly. I'm working on > some tests for the new emacs functionality that I'll include in the > inevitable v2 of this series. > > The last patch is mostly just a tickle to suggest adding the > recipients to the database. It would make the --include-recipient > searches much faster of course, but it might be overhead in the > database that folks aren't interested in. > > As always, feedback, review, and comments are much appreciated. > > jamie. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2012-09-08 17:25 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-20 1:52 [PATCH 00/11] add recipients to search output Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 01/11] lib: new thread addresses structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 02/11] lib: use new addresses structure for thread authors Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 04/11] lib: remove no longer needed author-specific thread functions Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 05/11] lib: add ability to store recipients in message structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 06/11] lib: store thread recipients in thread structure Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 07/11] test: search recipient output Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 08/11] cli: add thread recipients to search output Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 09/11] emacs: add ability to show recipients instead of author in search Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 10/11] emacs: add function to toggle showing authors/recipients " Jameson Graef Rollins 2012-08-20 1:52 ` [PATCH 11/11] lib: add recipients to database Jameson Graef Rollins 2012-08-31 21:34 ` Michal Sojka 2012-08-31 21:00 ` [PATCH 08/11] cli: add thread recipients to search output Michal Sojka 2012-08-31 20:44 ` [PATCH 06/11] lib: store thread recipients in thread structure Michal Sojka 2012-09-02 7:52 ` Mark Walters 2012-09-08 17:25 ` Austin Clements 2012-08-31 20:19 ` [PATCH 05/11] lib: add ability to store recipients in message structure Michal Sojka 2012-09-08 17:24 ` Austin Clements 2012-09-08 17:25 ` [PATCH 03/11] lib: give _thread_cleanup_author a more generic name Austin Clements 2012-09-08 17:24 ` [PATCH 02/11] lib: use new addresses structure for thread authors Austin Clements 2012-08-30 15:38 ` [PATCH 01/11] lib: new thread addresses structure Michal Sojka 2012-08-30 16:33 ` Jameson Graef Rollins 2012-09-08 17:24 ` Austin Clements 2012-08-22 20:43 ` [PATCH 00/11] add recipients to search output Jameson Graef Rollins 2012-08-23 7:21 ` Tomi Ollila 2012-09-08 17:23 ` Austin Clements
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).