[PATCH v2 (rebased) 0/3] Improve the display of matching/non-matching authors.

[PATCH v2 (rebased) 0/3] Improve the display of matching/non-matching authors.

[Mark Walters]

This is just a rebased (to current master) version of
id:1414172643-28270-1-git-send-email-dme@dme.org (git did the rebase by
itself using a three-way merge)

I will send some review comments as replies to this thread.

Best wishes

Mark


David Edmondson (3):
  search: Separately report matching and non-matching authors.
  emacs: Improved display of matching/non-matching authors.
  test: Update tests for 'authors_matched' and authors_non_matched'.

 emacs/notmuch.el             |  64 ++++++++++++++------------
 notmuch-search.c             | 105 +++++++++++++++++++++++++++++++++++++++++++
 test/T160-json.sh            |   9 ++++
 test/T170-sexp.sh            |   4 +-
 test/T470-missing-headers.sh |   8 ++++
 5 files changed, 159 insertions(+), 31 deletions(-)

-- 
2.1.4

[PATCH v2 (rebased) 1/3] search: Separately report matching and non-matching authors.

[Mark Walters]

From: David Edmondson <dme@dme.org>

In addition to the 'authors' attribute of each search result, include
'authors_matched' and 'authors_non_matched' attributes. Both
attributes are always included and are formatted as a list of
authors. If there are no matching authors, the 'authors_non_matched'
attribute is set to the empty list.
---
 notmuch-search.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/notmuch-search.c b/notmuch-search.c
index 6d08c25..6c6e497 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -22,6 +22,8 @@
 #include "sprinter.h"
 #include "string-util.h"
 
+#include <glib.h>
+
 typedef enum {
     /* Search command */
     OUTPUT_SUMMARY	= 1 << 0,
@@ -109,6 +111,105 @@ get_thread_query (notmuch_thread_t *thread,
     return 0;
 }
 
+/* Return a more pleasent rendering of the mail address
+ * `nasty_author'. */
+static const char *
+_nice_author (void *ctx, const char *nasty_author)
+{
+    const char *nice_author = NULL;
+
+    InternetAddressList *list = internet_address_list_parse_string (nasty_author);
+    if (list) {
+	InternetAddress *address = internet_address_list_get_address (list, 0);
+	if (address) {
+	    nice_author = internet_address_get_name (address);
+	    if (nice_author == NULL) {
+		InternetAddressMailbox *mailbox = INTERNET_ADDRESS_MAILBOX (address);
+		nice_author = internet_address_mailbox_get_addr (mailbox);
+	    }
+	}
+	/* Duplicate the string before `g_object_unref' destroys
+	 * it. */
+	if (nice_author)
+	    nice_author = talloc_strdup (ctx, nice_author);
+
+	g_object_unref (G_OBJECT (list));
+    }
+
+    if (nice_author)
+	return nice_author;
+    else
+	return nasty_author;
+}
+
+static int
+_enumerate_authors (sprinter_t *format,
+		 notmuch_thread_t *thread)
+{
+    notmuch_messages_t *messages;
+    GHashTable *matched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, NULL);
+    GHashTable *unmatched_hash = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, NULL);
+    GPtrArray *matched_array = g_ptr_array_new ();
+    GPtrArray *unmatched_array = g_ptr_array_new ();
+
+    /* Iterate over the messages in the thread collecting matching and
+     * non-matching authors. */
+    for (messages = notmuch_thread_get_messages (thread);
+	 notmuch_messages_valid (messages);
+	 notmuch_messages_move_to_next (messages))
+    {
+	notmuch_message_t *message = notmuch_messages_get (messages);
+	const char *author = _nice_author (thread, notmuch_message_get_header (message, "from"));
+
+	if (author) {
+	    GHashTable *hash;
+	    GPtrArray *array;
+
+	    if (notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH)) {
+		hash = matched_hash;
+		array = matched_array;
+	    } else {
+		hash = unmatched_hash;
+		array = unmatched_array;
+	    }
+
+	    if (!g_hash_table_lookup_extended (hash, author, NULL, NULL)) {
+		char *copy = talloc_strdup (thread, author);
+		g_hash_table_insert (hash, copy, NULL);
+		g_ptr_array_add (array, (char *) copy);
+	    }
+	}
+    }
+
+    /* Output the matched authors. */
+    unsigned int i;
+    format->map_key (format, "authors_matched");
+    format->begin_list (format);
+    for (i = 0; i < matched_array->len; i++)
+	format->string (format, (char *) g_ptr_array_index( matched_array, i));
+    format->end (format);
+
+    /* Output the non-matched authors, but not if they were seen
+     * already in the matched authors list. */
+    format->map_key (format, "authors_non_matched");
+    format->begin_list (format);
+    for (i = 0; i < unmatched_array->len; i++) {
+	char *author = (char *) g_ptr_array_index( unmatched_array, i);
+
+	if (!g_hash_table_lookup_extended (matched_hash, author, NULL, NULL))
+	    format->string (format, author);
+    }
+    format->end (format);
+
+    g_hash_table_unref (matched_hash);
+    g_hash_table_unref (unmatched_hash);
+
+    g_ptr_array_free (matched_array, TRUE);
+    g_ptr_array_free (unmatched_array, TRUE);
+
+    return 0;
+}
+
 static int
 do_search_threads (search_context_t *ctx)
 {
@@ -195,6 +296,10 @@ do_search_threads (search_context_t *ctx)
 		format->integer (format, total);
 		format->map_key (format, "authors");
 		format->string (format, authors);
+		if (_enumerate_authors (format, thread) < 0) {
+		    fprintf (stderr, "Out of memory\n");
+		    return 1;
+		}
 		format->map_key (format, "subject");
 		format->string (format, subject);
 		if (notmuch_format_version >= 2) {
-- 
2.1.4

[PATCH v2 (rebased) 2/3] emacs: Improved display of matching/non-matching authors.

[Mark Walters]

From: David Edmondson <dme@dme.org>

Rather than splitting the :authors attribute, which is error prone,
use the separate :authors_matched and :authors_non_matched attributes.

This improves the display of authors should one of them include a pipe
symbol (|) in their 'from' address.
---
 emacs/notmuch.el | 64 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 463b926..1f4f6c3 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -674,22 +674,24 @@ foreground and blue background."
 	;; Reverse the list so earlier entries take precedence
 	(reverse notmuch-search-line-faces)))
 
-(defun notmuch-search-author-propertize (authors)
+(defun notmuch-search-author-propertize (authors matching-length)
   "Split `authors' into matching and non-matching authors and
 propertize appropriately. If no boundary between authors and
 non-authors is found, assume that all of the authors match."
-  (if (string-match "\\(.*\\)|\\(.*\\)" authors)
-      (concat (propertize (concat (match-string 1 authors) ",")
-			  'face 'notmuch-search-matching-authors)
-	      (propertize (match-string 2 authors)
-			  'face 'notmuch-search-non-matching-authors))
-    (propertize authors 'face 'notmuch-search-matching-authors)))
-
-(defun notmuch-search-insert-authors (format-string authors)
+  (let ((match-part (substring authors 0 matching-length))
+	(non-match-part (substring authors matching-length)))
+
+      (concat (propertize match-part 'face 'notmuch-search-matching-authors)
+	      (propertize non-match-part 'face 'notmuch-search-non-matching-authors))))
+
+(defun notmuch-search-insert-authors (format-string matching-authors non-matching-authors)
   ;; Save the match data to avoid interfering with
   ;; `notmuch-search-process-filter'.
   (save-match-data
-    (let* ((formatted-authors (format format-string authors))
+    (let* ((authors (if (string= "" non-matching-authors)
+			matching-authors
+		      (concat matching-authors ", " non-matching-authors)))
+	   (formatted-authors (format format-string authors))
 	   (formatted-sample (format format-string ""))
 	   (visible-string formatted-authors)
 	   (invisible-string "")
@@ -705,9 +707,9 @@ non-authors is found, assume that all of the authors match."
 	    (setq visible-string (substring formatted-authors 0 visible-length)
 		  invisible-string (substring formatted-authors visible-length))
 	    ;; If possible, truncate the visible string at a natural
-	    ;; break (comma or pipe), as incremental search doesn't
-	    ;; match across the visible/invisible border.
-	    (when (string-match "\\(.*\\)\\([,|] \\)\\([^,|]*\\)" visible-string)
+	    ;; break (comma), as incremental search doesn't match
+	    ;; across the visible/invisible border.
+	    (when (string-match "\\(.*\\)\\(, \\)\\([^,]*\\)" visible-string)
 	      ;; Second clause is destructive on `visible-string', so
 	      ;; order is important.
 	      (setq invisible-string (concat (match-string 3 visible-string)
@@ -723,20 +725,23 @@ non-authors is found, assume that all of the authors match."
 				       ? ))))
 
       ;; Use different faces to show matching and non-matching authors.
-      (if (string-match "\\(.*\\)|\\(.*\\)" visible-string)
-	  ;; The visible string contains both matching and
-	  ;; non-matching authors.
-	  (setq visible-string (notmuch-search-author-propertize visible-string)
-		;; The invisible string must contain only non-matching
-		;; authors, as the visible-string contains both.
-		invisible-string (propertize invisible-string
-					     'face 'notmuch-search-non-matching-authors))
-	;; The visible string contains only matching authors.
-	(setq visible-string (propertize visible-string
-					 'face 'notmuch-search-matching-authors)
-	      ;; The invisible string may contain both matching and
-	      ;; non-matching authors.
-	      invisible-string (notmuch-search-author-propertize invisible-string)))
+      (let ((visible-length (length visible-string))
+	    (matching-length (length matching-authors)))
+
+	(if (> visible-length matching-length)
+	    ;; The visible string contains both matching and
+	    ;; non-matching authors.
+	    (setq visible-string (notmuch-search-author-propertize visible-string matching-length)
+		  ;; The invisible string must contain only non-matching
+		  ;; authors, as the visible-string contains both.
+		  invisible-string (propertize invisible-string
+					       'face 'notmuch-search-non-matching-authors))
+	  ;; The visible string contains only matching authors.
+	  (setq visible-string (propertize visible-string
+					   'face 'notmuch-search-matching-authors)
+		;; The invisible string may contain both matching and
+		;; non-matching authors.
+		invisible-string (notmuch-search-author-propertize invisible-string (- visible-length matching-length)))))
 
       ;; If there is any invisible text, add it as a tooltip to the
       ;; visible text.
@@ -770,8 +775,9 @@ 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 (notmuch-sanitize (plist-get result :authors))))
+    (notmuch-search-insert-authors format-string
+				   (notmuch-sanitize (mapconcat 'identity (plist-get result :authors_matched) ", "))
+				   (notmuch-sanitize (mapconcat 'identity (plist-get result :authors_non_matched) ", "))))
 
    ((string-equal field "tags")
     (let ((tags (plist-get result :tags))
-- 
2.1.4

[PATCH v2 (rebased) 3/3] test: Update tests for 'authors_matched' and authors_non_matched'.

[Mark Walters]

From: David Edmondson <dme@dme.org>

---
 test/T160-json.sh            | 9 +++++++++
 test/T170-sexp.sh            | 4 ++--
 test/T470-missing-headers.sh | 8 ++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/test/T160-json.sh b/test/T160-json.sh
index b346f37..3958841 100755
--- a/test/T160-json.sh
+++ b/test/T160-json.sh
@@ -25,6 +25,10 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
  \"matched\": 1,
  \"total\": 1,
  \"authors\": \"Notmuch Test Suite\",
+ \"authors_matched\": [
+     \"Notmuch Test Suite\"
+ ],
+ \"authors_non_matched\": [],
  \"subject\": \"json-search-subject\",
  \"query\": [\"id:$gen_msg_id\", null],
  \"tags\": [\"inbox\",
@@ -59,6 +63,11 @@ test_expect_equal_json "$output" "[{\"thread\": \"XXX\",
  \"matched\": 1,
  \"total\": 1,
  \"authors\": \"Notmuch Test Suite\",
+ \"authors\": \"Notmuch Test Suite\",
+ \"authors_matched\": [
+     \"Notmuch Test Suite\"
+ ],
+ \"authors_non_matched\": [],
  \"subject\": \"json-search-utf8-body-sübjéct\",
  \"query\": [\"id:$gen_msg_id\", null],
  \"tags\": [\"inbox\",
diff --git a/test/T170-sexp.sh b/test/T170-sexp.sh
index 800ebc6..6c71491 100755
--- a/test/T170-sexp.sh
+++ b/test/T170-sexp.sh
@@ -19,7 +19,7 @@ test_expect_equal "$output" "((((:id \"${gen_msg_id}\" :match t :excluded nil :f
 test_begin_subtest "Search message: sexp"
 add_message "[subject]=\"sexp-search-subject\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"sexp-search-message\""
 output=$(notmuch search --format=sexp "sexp-search-message" | notmuch_search_sanitize)
-test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-subject\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
+test_expect_equal "$output" "((:thread \"0000000000000002\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :authors_matched (\"Notmuch Test Suite\") :authors_non_matched () :subject \"sexp-search-subject\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
 
 test_begin_subtest "Show message: sexp, utf-8"
 add_message "[subject]=\"sexp-show-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-show-méssage\""
@@ -44,7 +44,7 @@ test_expect_equal "$output" "((((:id \"$id\" :match t :excluded nil :filename \"
 test_begin_subtest "Search message: sexp, utf-8"
 add_message "[subject]=\"sexp-search-utf8-body-sübjéct\"" "[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" "[body]=\"jsön-search-méssage\""
 output=$(notmuch search --format=sexp "jsön-search-méssage" | notmuch_search_sanitize)
-test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :subject \"sexp-search-utf8-body-sübjéct\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
+test_expect_equal "$output" "((:thread \"0000000000000005\" :timestamp 946728000 :date_relative \"2000-01-01\" :matched 1 :total 1 :authors \"Notmuch Test Suite\" :authors_matched (\"Notmuch Test Suite\") :authors_non_matched () :subject \"sexp-search-utf8-body-sübjéct\" :query (\"id:$gen_msg_id\" nil) :tags (\"inbox\" \"unread\")))"
 
 
 test_done
diff --git a/test/T470-missing-headers.sh b/test/T470-missing-headers.sh
index e256c10..66c2a1d 100755
--- a/test/T470-missing-headers.sh
+++ b/test/T470-missing-headers.sh
@@ -34,6 +34,10 @@ test_expect_equal_json "$output" '
 [
     {
         "authors": "",
+        "authors_matched": [
+            ""
+        ],
+        "authors_non_matched": [],
         "date_relative": "2001-01-05",
         "matched": 1,
         "subject": "",
@@ -48,6 +52,10 @@ test_expect_equal_json "$output" '
     },
     {
         "authors": "Notmuch Test Suite",
+        "authors_matched": [
+            "Notmuch Test Suite"
+        ],
+        "authors_non_matched": [],
         "date_relative": "1970-01-01",
         "matched": 1,
         "subject": "",
-- 
2.1.4

Re: [PATCH v2 (rebased) 0/3] Improve the display of matching/non-matching authors.

[David Edmondson]

On Mon, Dec 14 2015, Mark Walters wrote:
> This is just a rebased (to current master) version of
> id:1414172643-28270-1-git-send-email-dme@dme.org (git did the rebase by
> itself using a three-way merge)
>
> I will send some review comments as replies to this thread.

I didn't see any, but in general I think:

- the core notmuch changes should be okay to commit (presuming that they
  still apply cleanly),
- the emacs UI changes need some careful consideration. There is some
  small loss of functionality alluded to in
  id:1414172643-28270-1-git-send-email-dme@dme.org:

  % - More improvements to the code that renders the authors are possible
  %   (to improve the chosen break between visible and invisible), but a
  %   planned re-write of the `notmuch-search-result-format' code would
  %   render that irrelevant.

  Fixing that properly requires the further changes to how search
  results are presented. Those changes were never finished.

>
> Best wishes
>
> Mark
>
>
> David Edmondson (3):
>   search: Separately report matching and non-matching authors.
>   emacs: Improved display of matching/non-matching authors.
>   test: Update tests for 'authors_matched' and authors_non_matched'.
>
>  emacs/notmuch.el             |  64 ++++++++++++++------------
>  notmuch-search.c             | 105 +++++++++++++++++++++++++++++++++++++++++++
>  test/T160-json.sh            |   9 ++++
>  test/T170-sexp.sh            |   4 +-
>  test/T470-missing-headers.sh |   8 ++++
>  5 files changed, 159 insertions(+), 31 deletions(-)
>
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch