Re: [Patch v3 5/8] emacs: show: mark tags changed since buffer loaded
authorMark Walters <markwalters1009@gmail.com>
Sat, 22 Mar 2014 11:44:18 +0000 (11:44 +0000)
committerW. Trevor King <wking@tremily.us>
Fri, 7 Nov 2014 18:00:52 +0000 (10:00 -0800)
1f/11655ea65f9051d17114dc72d248134791f946 [new file with mode: 0644]

diff --git a/1f/11655ea65f9051d17114dc72d248134791f946 b/1f/11655ea65f9051d17114dc72d248134791f946
new file mode 100644 (file)
index 0000000..a1667c8
--- /dev/null
@@ -0,0 +1,415 @@
+Return-Path: <m.walters@qmul.ac.uk>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 21BA8431FC0\r
+       for <notmuch@notmuchmail.org>; Sat, 22 Mar 2014 04:44:35 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -1.098\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
+       tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
+       NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id MV3vWZ7vlNgX for <notmuch@notmuchmail.org>;\r
+       Sat, 22 Mar 2014 04:44:27 -0700 (PDT)\r
+Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
+       (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
+       (No client certificate requested)\r
+       by olra.theworths.org (Postfix) with ESMTPS id 09C36431FBC\r
+       for <notmuch@notmuchmail.org>; Sat, 22 Mar 2014 04:44:27 -0700 (PDT)\r
+Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
+       by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
+       (envelope-from <m.walters@qmul.ac.uk>)\r
+       id 1WRKLc-00060L-Kt; Sat, 22 Mar 2014 11:44:21 +0000\r
+Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
+       by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.71)\r
+       (envelope-from <m.walters@qmul.ac.uk>)\r
+       id 1WRKLb-00046f-T7; Sat, 22 Mar 2014 11:44:20 +0000\r
+From: Mark Walters <markwalters1009@gmail.com>\r
+To: Austin Clements <amdragon@MIT.EDU>\r
+Subject: Re: [Patch v3 5/8] emacs: show: mark tags changed since buffer loaded\r
+In-Reply-To: <87pplek4v0.fsf@qmul.ac.uk>\r
+References: <1394597397-8486-1-git-send-email-markwalters1009@gmail.com>\r
+       <1394597397-8486-6-git-send-email-markwalters1009@gmail.com>\r
+       <20140322033933.GC31187@mit.edu> <87pplek4v0.fsf@qmul.ac.uk>\r
+User-Agent: Notmuch/0.15.2+615~g78e3a93 (http://notmuchmail.org) Emacs/23.4.1\r
+       (x86_64-pc-linux-gnu)\r
+Date: Sat, 22 Mar 2014 11:44:18 +0000\r
+Message-ID: <87mwgih2zx.fsf@qmul.ac.uk>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+X-Sender-Host-Address: 93.97.24.31\r
+X-QM-Geographic: According to ripencc,\r
+       this message was delivered by a machine in Britain (UK) (GB).\r
+X-QM-SPAM-Info: Sender has good ham record.  :)\r
+X-QM-Body-MD5: fea0858007c2b9064a64c69cb1ffd51b (of first 20000 bytes)\r
+X-SpamAssassin-Score: 0.0\r
+X-SpamAssassin-SpamBar: /\r
+X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
+       determine if it is\r
+       spam. We require at least 5.0 points to mark a message as spam.\r
+       This message scored 0.0 points. Summary of the scoring: \r
+       * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
+       provider *      (markwalters1009[at]gmail.com)\r
+       * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay\r
+       *      domain\r
+       *  0.0 AWL AWL: From: address is in the auto white-list\r
+X-QM-Scan-Virus: ClamAV says the message is clean\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Sat, 22 Mar 2014 11:44:35 -0000\r
+\r
+\r
+On Sat, 22 Mar 2014, Mark Walters <markwalters1009@gmail.com> wrote:\r
+> Hi\r
+>\r
+> Ok I have something working based on your version below. I will post\r
+> once I have tested a little more. Since the diff with my version will be\r
+> quite big I will just comment on the differences from your version.\r
+>\r
+> On Sat, 22 Mar 2014, Austin Clements <amdragon@MIT.EDU> wrote:\r
+>> Quoth Mark Walters on Mar 12 at  4:09 am:\r
+>>> This allows (and requires) the original-tags to be passed along with\r
+>>> the current-tags to be passed to notmuch-tag-format-tags. This allows\r
+>>> the tag formatting to show added and deleted tags.By default a removed\r
+>>> tag is displayed with strike-through in red (if strike-through is not\r
+>>> available, eg on a terminal, inverse video is used instead) and an\r
+>>> added tag is displayed underlined in green.\r
+>>> \r
+>>> If the caller does not wish to use the new feature it can pass\r
+>>> current-tags for both arguments and, at this point, we do exactly that\r
+>>> in the three callers of this function.\r
+>>> \r
+>>> Note, we cannot tidily allow original-tags to be optional because we would\r
+>>> need to distinguish nil meaning "we are not specifying original-tags"\r
+>>> from nil meaning there were no original-tags (an empty list).\r
+>>> \r
+>>> We use this in subsequent patches to make it clear when a message was\r
+>>> unread when you first loaded a show buffer (previously the unread tag\r
+>>> could be removed before a user realised that it had been unread).\r
+>>> \r
+>>> The code adds into the existing tag formatting code. The user can\r
+>>> specify exactly how a tag should be displayed normally, when deleted,\r
+>>> or when added.\r
+>>> \r
+>>> Since the formatting code matches regexps a user can match all deleted\r
+>>> tags with a ".*" in notmuch-tag-deleted-formats.  For example setting\r
+>>> notmuch-tag-deleted-formats to '((".*" nil)) tells notmuch not to show\r
+>>> deleted tags at all.\r
+>>> \r
+>>> All the variables are customizable; however, more complicated cases\r
+>>> like changing the face depending on the type of display will require\r
+>>> custom lisp.\r
+>>> \r
+>>> Currently this overrides notmuch-tag-deleted-formats for the tests\r
+>>> setting it to '((".*" nil)) so that they get removed from the display\r
+>>> and, thus, all tests still pass.\r
+>>> ---\r
+>>>  emacs/notmuch-show.el |    4 +-\r
+>>>  emacs/notmuch-tag.el  |   72 +++++++++++++++++++++++++++++++++++-------------\r
+>>>  emacs/notmuch-tree.el |    2 +-\r
+>>>  emacs/notmuch.el      |    2 +-\r
+>>>  test/test-lib.el      |    5 +++\r
+>>>  5 files changed, 61 insertions(+), 24 deletions(-)\r
+>>> \r
+>>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
+>>> index 019f51d..5492be4 100644\r
+>>> --- a/emacs/notmuch-show.el\r
+>>> +++ b/emacs/notmuch-show.el\r
+>>> @@ -344,7 +344,7 @@ (defun notmuch-show-update-tags (tags)\r
+>>>      (if (re-search-forward "(\\([^()]*\\))$" (line-end-position) t)\r
+>>>    (let ((inhibit-read-only t))\r
+>>>      (replace-match (concat "("\r
+>>> -                           (notmuch-tag-format-tags tags)\r
+>>> +                           (notmuch-tag-format-tags tags tags)\r
+>>>                             ")"))))))\r
+>>>  \r
+>>>  (defun notmuch-clean-address (address)\r
+>>> @@ -423,7 +423,7 @@ (defun notmuch-show-insert-headerline (headers date tags depth)\r
+>>>        " ("\r
+>>>        date\r
+>>>        ") ("\r
+>>> -      (notmuch-tag-format-tags tags)\r
+>>> +      (notmuch-tag-format-tags tags tags)\r
+>>>        ")\n")\r
+>>>      (overlay-put (make-overlay start (point)) 'face 'notmuch-message-summary-face)))\r
+>>>  \r
+>>> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el\r
+>>> index 4698856..cfccb8e 100644\r
+>>> --- a/emacs/notmuch-tag.el\r
+>>> +++ b/emacs/notmuch-tag.el\r
+>>> @@ -184,45 +184,77 @@ (defun notmuch-tag-clear-cache ()\r
+>>>    "Clear the internal cache of tag formats."\r
+>>>    (clrhash notmuch-tag--format-cache))\r
+>>>  \r
+>>> -(defun notmuch-tag-format-tag (tag)\r
+>>> -  "Format TAG by according to `notmuch-tag-formats'.\r
+>>> -\r
+>>> -Callers must ensure that the tag format cache has been recently cleared\r
+>>> -via `notmuch-tag-clear-cache' before using this function.  For example,\r
+>>> -it would be appropriate to clear the cache just prior to filling a\r
+>>> -buffer that uses formatted tags."\r
+>>> -\r
+>>> -  (let ((formatted (gethash tag notmuch-tag--format-cache 'missing)))\r
+>>> +(defun notmuch-tag-format-tag-by-state (tag formatted-tag tag-state)\r
+>>> +  "Format TAG according to the appropriate `notmuch-tag-formats`.\r
+>>> +\r
+>>> +Applies formats for TAG from the appropriate one of\r
+>>> +`notmuch-tag-formats`, `notmuch-tag-deleted-formats` and\r
+>>> +`notmuch-tag-added-formats` based on TAG-STATE to the partially\r
+>>\r
+>> The second ` should be a ' on all four of the above references.\r
+>>\r
+>>> +formatted tag FORMATTED-TAG."\r
+>>> +  (let ((formatted (gethash (cons tag tag-state) notmuch-tag--format-cache 'missing)))\r
+>>\r
+>> Something's strange here.  If this hits in the cache, it will ignore\r
+>> formatted-tag.  I can't actually construct a situation where this does\r
+>> the wrong thing, but it always seems to do the right thing for the\r
+>> wrong reasons.\r
+>>\r
+>> This code would make a lot more sense to me if it were turned\r
+>> inside-out with `notmuch-tag-format-tag':\r
+>>\r
+>> (defun notmuch-tag-format-tag (tags orig-tags tag)\r
+>>   "Format TAG according to `notmuch-tag-formats'.\r
+>>\r
+>> TAGS and ORIG-TAGS are lists of the current tags and the original\r
+>> tags; tags which have been deleted (i.e., are in ORIG-TAGS but\r
+>> are not in TAGS) are shown using formats from\r
+>> `notmuch-tag-deleted-formats'; tags which have been added (i.e.,\r
+>> are in TAGS but are not in ORIG-TAGS) are shown using formats\r
+>> from `notmuch-tag-added-formats' and tags which have not been\r
+>> changed (the normal case) are shown using formats from\r
+>> `notmuch-tag-formats'"\r
+>>   (let* ((tag-state (cond ((not (member tag tags)) 'deleted)\r
+>>                           ((not (member tag orig-tags)) 'added)))\r
+>>          (formatted (gethash (cons tag tag-state) notmuch-tag--format-cache\r
+>>                              'missing)))\r
+>>     (when (eq formatted 'missing)\r
+>\r
+> I changed formatted to formatted-tag.\r
+>\r
+>>       (let ((base (notmuch-tag--get-formats tag notmuch-tag-formats))\r
+>>             (over (case tag-state\r
+>>                         ((nil) nil)\r
+>>                         (deleted (notmuch-tag--get-formats \r
+>>                                   tag notmuch-tag-deleted-formats))\r
+>>                         (added (notmuch-tag--get-formats \r
+>>                                 tag notmuch-tag-deleted-formats)))))\r
+>\r
+> I moved the nil case to an otherwise case (and corrected the typo of\r
+> deleted-formats to added-formats in the second clause).\r
+>\r
+>>         (setq formatted (notmuch-tag--do-format\r
+>>                          (notmuch-tag--do-format tag) base over))\r
+>\r
+> I split this into two steps. I also slightly changed\r
+> notmuch-tag--do-format so it gets passed tag and formatted-tag\r
+>\r
+>>         (puthash (cons tag tag-state) formatted notmuch-tag--format-cache)))\r
+>>     formatted))\r
+>>\r
+>> (defun notmuch-tag--get-formats (tag format-alist)\r
+>>   "Find the first item whose car regexp-matches TAG."\r
+>>   (save-match-data\r
+>>     ;; Don't use assoc-default since there's no way to distinguish a\r
+>>     ;; missing key from a present key with a null cdr.\r
+>>     (assoc* tag format-alist\r
+>>             :test (lambda (tag key)\r
+>>                     (and (eq (string-match key tag) 0)\r
+>>                           (= (match-end 0) (length tag)))))))\r
+>\r
+> I have not changed this.\r
+>\r
+>> (defun notmuch-tag--do-format (tag formats)\r
+>>   "Apply a tag-formats entry to TAG."\r
+>\r
+> We need to pass formatted-tag as well as the original unformatted tag\r
+> because we want to do the look up to decide what to do based on the\r
+> original tag not whatever it has become.\r
+\r
+Ok I have realised I was wrong and your version was fine (as the choice\r
+of formats was already made). However, I do still pass the unformatted\r
+tag so that the formatter can access that as `bare-tag'.\r
+\r
+>>   (cond ((null formats)             ;; - Tag not in `formats',\r
+>>          tag)                       ;;   the format is the tag itself.\r
+>>         ((null (cdr formats))       ;; - Tag was deliberately hidden,\r
+>>          nil)                       ;;   no format must be returned\r
+>>         (t\r
+>>          ;; Tag was found and has formats, we must apply all the\r
+>>          ;; formats.  TAG may be null so treat that as a special case.\r
+>>          (let ((old-tag tag) (tag (or tag "")))\r
+>>            (dolist (format (cdr formats))\r
+>>              (setq tag (eval format)))\r
+>>            (if (and (null old-tag) (equal tag ""))\r
+>>                nil\r
+>>              tag)))))\r
+\r
+More importantly I was getting (erratic) weird effects because some of\r
+these changes changed tag (and even the copy of tag outside this\r
+function). I fixed this by using copy-sequence for the second let\r
+clause.\r
+\r
+Best wishes \r
+\r
+Mark\r
+\r
+\r
+> This got slightly tweaked because of the formatted/original split above.\r
+>\r
+>\r
+>> (Completely untested and all indented with spaces and probably\r
+>> incorrectly because I wrote it all in my email buffer, but you get the\r
+>> idea.)\r
+>\r
+> And I think I fixed all the whitespace\r
+>\r
+> This looks much nicer than my version. Thanks!\r
+>\r
+> Best wishes\r
+>\r
+> Mark\r
+>\r
+>\r
+>\r
+>>\r
+>>>      (when (eq formatted 'missing)\r
+>>> -      (let* ((formats\r
+>>> +      (let* ((tag-formats (case tag-state\r
+>>> +                          ((list nil) notmuch-tag-formats)\r
+>>\r
+>> While this isn't *technically* wrong, I don't think you meant to\r
+>> accept a tag-state of 'list.  Should be\r
+>>\r
+>> (case tag-state\r
+>>    ((nil) notmuch-tag-formats)\r
+>>    (deleted ...\r
+>>\r
+>>> +                          (deleted notmuch-tag-deleted-formats)\r
+>>> +                          (added notmuch-tag-added-formats)))\r
+>>> +       (formats\r
+>>>          (save-match-data\r
+>>>            ;; Don't use assoc-default since there's no way to\r
+>>>            ;; distinguish a missing key from a present key with a\r
+>>>            ;; null cdr:.\r
+>>> -          (assoc* tag notmuch-tag-formats\r
+>>> +          (assoc* tag tag-formats\r
+>>>                    :test (lambda (tag key)\r
+>>>                            (and (eq (string-match key tag) 0)\r
+>>>                                 (= (match-end 0) (length tag))))))))\r
+>>>    (setq formatted\r
+>>>          (cond\r
+>>> -         ((null formats)          ;; - Tag not in `notmuch-tag-formats',\r
+>>> -          tag)                    ;;   the format is the tag itself.\r
+>>> +         ((null formats)          ;; - Tag not in `tag-formats',\r
+>>> +          formatted-tag)          ;;   the format is the tag itself.\r
+>>>           ((null (cdr formats))    ;; - Tag was deliberately hidden,\r
+>>>            nil)                    ;;   no format must be returned\r
+>>> -         (t                       ;; - Tag was found and has formats,\r
+>>> -          (let ((tag tag))        ;;   we must apply all the formats.\r
+>>> +         (t\r
+>>> +          ;; Tag was found and has formats, we must apply all\r
+>>> +          ;; the formats.  FORMATTED-TAG may be null so treat\r
+>>> +          ;; that as a special case.\r
+>>> +          (let ((tag (or formatted-tag "")))\r
+>>>              (dolist (format (cdr formats) tag)\r
+>>> -              (setq tag (eval format)))))))\r
+>>> -  (puthash tag formatted notmuch-tag--format-cache)))\r
+>>> +              (setq tag (eval format)))\r
+>>> +            (if (and (null formatted-tag)\r
+>>> +                     (equal tag ""))\r
+>>> +                nil\r
+>>> +              tag)))))\r
+>>> +  (puthash (cons tag tag-state) formatted notmuch-tag--format-cache)))\r
+>>>      formatted))\r
+>>>  \r
+>>> -(defun notmuch-tag-format-tags (tags &optional face)\r
+>>> +(defun notmuch-tag-format-tag (tags orig-tags tag)\r
+>>> +  "Format TAG according to `notmuch-tag-formats'.\r
+>>> +\r
+>>> +TAGS and ORIG-TAGS are lists of the current tags and the original\r
+>>> +tags; tags which have been deleted (i.e., are in ORIG-TAGS but\r
+>>> +are not in TAGS) are shown using formats from\r
+>>> +`notmuch-tag-deleted-formats'; tags which have been added (i.e.,\r
+>>> +are in TAGS but are not in ORIG-TAGS) are shown using formats\r
+>>> +from `notmuch-tag-added-formats' and tags which have not been\r
+>>> +changed (the normal case) are shown using formats from\r
+>>> +`notmuch-tag-formats'"\r
+>>> +  (let* ((formatted-tag (notmuch-tag-format-tag-by-state tag tag nil)))\r
+>>> +    (cond ((not (member tag tags))\r
+>>> +     (notmuch-tag-format-tag-by-state tag formatted-tag 'deleted))\r
+>>> +    ((not (member tag orig-tags))\r
+>>> +     (notmuch-tag-format-tag-by-state tag formatted-tag 'added))\r
+>>> +    (t\r
+>>> +      formatted-tag))))\r
+>>> +\r
+>>> +(defun notmuch-tag-format-tags (tags orig-tags &optional face)\r
+>>>    "Return a string representing formatted TAGS."\r
+>>> -  (let ((face (or face 'notmuch-tag-face)))\r
+>>> +  (let ((face (or face 'notmuch-tag-face))\r
+>>> +  (all-tags (sort (delete-dups (append tags orig-tags nil)) #'string<)))\r
+>>>      (notmuch-apply-face\r
+>>>       (mapconcat #'identity\r
+>>>            ;; nil indicated that the tag was deliberately hidden\r
+>>> -          (delq nil (mapcar #'notmuch-tag-format-tag tags))\r
+>>> +          (delq nil (mapcar\r
+>>> +                     (apply-partially #'notmuch-tag-format-tag tags orig-tags)\r
+>>> +                     all-tags))\r
+>>>            " ")\r
+>>>       face\r
+>>>       t)))\r
+>>> diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el\r
+>>> index c78d9de..8bf2fbc 100644\r
+>>> --- a/emacs/notmuch-tree.el\r
+>>> +++ b/emacs/notmuch-tree.el\r
+>>> @@ -704,7 +704,7 @@ (defun notmuch-tree-format-field (field format-string msg)\r
+>>>        (face (if match\r
+>>>                  'notmuch-tree-match-tag-face\r
+>>>                'notmuch-tree-no-match-tag-face)))\r
+>>> -  (format format-string (notmuch-tag-format-tags tags face)))))))\r
+>>> +  (format format-string (notmuch-tag-format-tags tags tags face)))))))\r
+>>>  \r
+>>>  (defun notmuch-tree-format-field-list (field-list msg)\r
+>>>    "Format fields of MSG according to FIELD-LIST and return string"\r
+>>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el\r
+>>> index 93a6d8b..609f408 100644\r
+>>> --- a/emacs/notmuch.el\r
+>>> +++ b/emacs/notmuch.el\r
+>>> @@ -754,7 +754,7 @@ (defun notmuch-search-insert-field (field format-string result)\r
+>>>  \r
+>>>     ((string-equal field "tags")\r
+>>>      (let ((tags (plist-get result :tags)))\r
+>>> -      (insert (format format-string (notmuch-tag-format-tags tags)))))))\r
+>>> +      (insert (format format-string (notmuch-tag-format-tags tags tags)))))))\r
+>>>  \r
+>>>  (defun notmuch-search-show-result (result &optional pos)\r
+>>>    "Insert RESULT at POS or the end of the buffer if POS is null."\r
+>>> diff --git a/test/test-lib.el b/test/test-lib.el\r
+>>> index 37fcb3d..437f83f 100644\r
+>>> --- a/test/test-lib.el\r
+>>> +++ b/test/test-lib.el\r
+>>> @@ -165,3 +165,8 @@ (defun notmuch-test-expect-equal (output expected)\r
+>>>  \r
+>>>       (t\r
+>>>        (notmuch-test-report-unexpected output expected)))))\r
+>>> +\r
+>>> +;; For historical reasons, we hide deleted tags by default in the test\r
+>>> +;; suite\r
+>>> +(setq notmuch-tag-deleted-formats\r
+>>> +      '((".*" nil)))\r