From e330ff29b168658d175a9408b87c6f952696d50e Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Sun, 30 Sep 2012 19:58:30 +2000 Subject: [PATCH] Re: [PATCH] emacs: simplify point placement in notmuch-hello refresh --- 0e/e8909648371c8a76e357e4805c399638417f49 | 298 ++++++++++++++++++++++ 1 file changed, 298 insertions(+) create mode 100644 0e/e8909648371c8a76e357e4805c399638417f49 diff --git a/0e/e8909648371c8a76e357e4805c399638417f49 b/0e/e8909648371c8a76e357e4805c399638417f49 new file mode 100644 index 000000000..5744fc37c --- /dev/null +++ b/0e/e8909648371c8a76e357e4805c399638417f49 @@ -0,0 +1,298 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 2CCD0431FBF + for ; Sat, 29 Sep 2012 16:58:36 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id NtBmrqh8Tr3F for ; + Sat, 29 Sep 2012 16:58:34 -0700 (PDT) +Received: from dmz-mailsec-scanner-8.mit.edu (DMZ-MAILSEC-SCANNER-8.MIT.EDU + [18.7.68.37]) + by olra.theworths.org (Postfix) with ESMTP id 30629431FBC + for ; Sat, 29 Sep 2012 16:58:34 -0700 (PDT) +X-AuditID: 12074425-b7fcc6d00000091f-91-50678b29a175 +Received: from mailhub-auth-4.mit.edu ( [18.7.62.39]) + by dmz-mailsec-scanner-8.mit.edu (Symantec Messaging Gateway) with SMTP + id 6A.3D.02335.92B87605; Sat, 29 Sep 2012 19:58:33 -0400 (EDT) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id q8TNwWiC016507; + Sat, 29 Sep 2012 19:58:33 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q8TNwUhZ005141 + (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); + Sat, 29 Sep 2012 19:58:31 -0400 (EDT) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) + (envelope-from ) + id 1TI6vW-0006xx-GH; Sat, 29 Sep 2012 19:58:30 -0400 +Date: Sat, 29 Sep 2012 19:58:30 -0400 +From: Austin Clements +To: Jani Nikula +Subject: Re: [PATCH] emacs: simplify point placement in notmuch-hello refresh +Message-ID: <20120929235830.GZ26662@mit.edu> +References: <1348958664-1767-1-git-send-email-jani@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +Content-Disposition: inline +In-Reply-To: <1348958664-1767-1-git-send-email-jani@nikula.org> +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFmpileLIzCtJLcpLzFFi42IRYrdT19XsTg8wuLxa1aJpurPF9ZszmR2Y + PG7df83u8WzVLeYApigum5TUnMyy1CJ9uwSujNsNX9gLNvtVbOq7xtjAOMu2i5GTQ0LAROLI + jx1sELaYxIV764FsLg4hgX2MEtc/rGCCcDYwSny8tJQFpEpI4CSTxMdzuhCJJYwSF1vXgCVY + BFQl/q1bzwxiswloSGzbv5wRxBYRUJTYfHI/mM0sIC3x7XczE4gtLOArcXPVNrDVvAI6Eh/W + 9TFCLLCTeLzpC1RcUOLkzCcsEL1aEjf+vQTq5QCbs/wfB0iYU8Be4vnkj2CtogIqElNObmOb + wCg0C0n3LCTdsxC6FzAyr2KUTcmt0s1NzMwpTk3WLU5OzMtLLdK10MvNLNFLTSndxAgOahfV + HYwTDikdYhTgYFTi4b15OjVAiDWxrLgy9xCjJAeTkiivZUd6gBBfUn5KZUZicUZ8UWlOavEh + RgkOZiUR3oxioHLelMTKqtSifJiUNAeLkjjvjZSb/kIC6YklqdmpqQWpRTBZGQ4OJQneyV1A + QwWLUtNTK9Iyc0oQ0kwcnCDDeYCGe4HU8BYXJOYWZ6ZD5E8xKkqJ82aBJARAEhmleXC9sKTz + ilEc6BVh3iCQKh5gwoLrfgU0mAlocNWqNJDBJYkIKakGxqiH9otL7ogGLvGbVHpoLUPavQTu + FRtZvjbsumXXuXiRx0y7K2knLhxQ8yywVf70OcTzEJPBuogXEmvuqP1Ue5u68RP/Tx/Jvini + O+7nuMyfIL7vELf3hc6zjdOedCerxUkyRn6U89raeGWduZLK3bboG0rZtZ8nCVUX55WbRFeJ + 6cw7oNEVr8RSnJFoqMVcVJwIAPy8KngVAwAA +Cc: notmuch@notmuchmail.org +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Sat, 29 Sep 2012 23:58:36 -0000 + +LGTM. I wasn't aware notmuch-hello even attempted to maintain point, +since I'd never seen it work. + +This patch could go a step further and remove found-target-pos from +notmuch-hello-insert-saved-searches, target-pos from +notmuch-hello-insert-searches, and found-target-pos from +notmuch-hello-insert-buttons. + +It might be more stable to record the line number and column count and +restore those, since point is sensitive to additions and removals of +text within earlier lines. + + +On a more abstract note, I feel like we have this same problem all +over the Emacs UI: we "update" some UI by completely rebuilding it, +and then we make some ad hoc, more-or-less (usually less) intelligent +guess about where point should go. notmuch-hello has this problem, +updating search-mode lines on tag change has this problem, refreshing +the search buffer has this problem, and various buffer-rebuilding +toggles in show have this problem. + +Proper incremental UI updates are simply not an option for some of +these cases, because we have to call out to the CLI to get new data +and that data might be completely different from what we're currently +showing. We could restructure things to support this---for example, +when toggling crypto, show could ask for the body of each message it's +already showing---but this would complicate the code and I'm not sure +we can restructure cases where the set of objects may change. + +But I wonder if there's an abstraction that would let us fully rebuild +UIs or parts of UIs, but keep enough context to make this fluid. I'm +imagining something like representing a buffer as a tree of UI nodes, +where the buffer itself is simply the flattening of this tree, and +each UI node has a identifier that's unique among its siblings. For +example, the search buffer would have a root child for each search +result, and then each piece of the result line would be a child of +that line's node. This mechanism would give us a persistent context. +Before an update, it could record the identifiers of all of the nodes +in the path leading to the leaf node containing point, along with all +of the prior siblings of each of these nodes and the offset of point +within the leaf node. After and update, it could replay this path as +best as possible: at each step in the path, if it can find a node with +the same identifier as the node in the path, it would descend into it; +otherwise, it would descend into the last recorded sibling it can find +in the new tree. When this decent reaches a leaf, it can place point +there. This same mechanism could be used to record and restore the +window start position, as well as other markers we may need. + +Quoth Jani Nikula on Sep 30 at 1:44 am: +> notmuch-hello (called also through notmuch-hello-update, bound to '=' +> by default) tries to find the widget under or following point before +> refresh, and put the point back to the widget afterwards. The code has +> grown quite complicated, and has at least the following issues: +> +> 1) All the individual section functions have to include code to +> support point placement. If there is no such support, point is +> dropped to the search box. Only saved searches and all tags +> sections support point placement. +> +> 2) Point placement is based on widget-value. If there are two widgets +> with the same widget-value (for example a saved search with the +> same name as a tag) the point is moved to the earlier one, even if +> point was on the later one. +> +> 3) When first entering notmuch-hello notmuch-hello-target is nil, and +> point is dropped to the search box. +> +> Moving the point to the search box is annoying because the user is +> required to move the point before being able to enter key bindings. +> +> Simplify the code by removing all point placement based on widgets, as +> it does not work properly, and trying to fix that would unnecessarily +> complicate the code. +> +> Point is simply saved before refresh, and put back to where it +> was. Sometimes, if notmuch-show-empty-saved-searches is nil, and the +> refresh adds or removes saved searches from the list, this has the +> appearance of moving the point relative to the nearest widgets. This +> is a much smaller and less frequent problem than the ones listed +> above. +> +> --- +> +> I've sent this before months ago, and received comments starting at +> id:"87aa2aohjs.fsf@gmail.com". I find the current behaviour annoying, +> and I prefer this simple fix over complicating the existing +> mess. Nobody has stood up to do anything better anyway. Perhaps we +> could take a step forward with this while waiting for Someone(tm) to +> come up with the perfect point placement? +> --- +> emacs/notmuch-hello.el | 70 ++++++++++++------------------------------------ +> 1 file changed, 17 insertions(+), 53 deletions(-) +> +> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el +> index 684bedc..04d90cf 100644 +> --- a/emacs/notmuch-hello.el +> +++ b/emacs/notmuch-hello.el +> @@ -154,11 +154,6 @@ International Bureau of Weights and Measures." +> (defvar notmuch-hello-url "http://notmuchmail.org" +> "The `notmuch' web site.") +> +> -(defvar notmuch-hello-search-pos nil +> - "Position of search widget, if any. +> - +> -This should only be set by `notmuch-hello-insert-search'.") +> - +> (defvar notmuch-hello-custom-section-options +> '((:filter (string :tag "Filter for each tag")) +> (:filter-count (string :tag "Different filter to generate message counts")) +> @@ -209,11 +204,8 @@ function produces a section simply by adding content to the current +> buffer. A section should not end with an empty line, because a +> newline will be inserted after each section by `notmuch-hello'. +> +> -Each function should take no arguments. If the produced section +> -includes `notmuch-hello-target' (i.e. cursor should be positioned +> -inside this section), the function should return this element's +> -position. +> -Otherwise, it should return nil. +> +Each function should take no arguments. The return value is +> +ignored. +> +> For convenience an element can also be a list of the form (FUNC ARG1 +> ARG2 .. ARGN) in which case FUNC will be applied to the rest of the +> @@ -240,15 +232,6 @@ supported for \"Customized queries section\" items." +> notmuch-hello-query-section +> (function :tag "Custom section")))) +> +> -(defvar notmuch-hello-target nil +> - "Button text at position of point before rebuilding the notmuch-buffer. +> - +> -This variable contains the text of the button, if any, the +> -point was positioned at before the notmuch-hello buffer was +> -rebuilt. This should never actually be global and is defined as a +> -defvar only for documentation purposes and to avoid a compiler +> -warning about it occurring as a free variable.") +> - +> (defvar notmuch-hello-hidden-sections nil +> "List of sections titles whose contents are hidden") +> +> @@ -449,8 +432,6 @@ Such a list can be computed with `notmuch-hello-query-counts'." +> (msg-count (third elem))) +> (widget-insert (format "%8s " +> (notmuch-hello-nice-number msg-count))) +> - (if (string= name notmuch-hello-target) +> - (setq found-target-pos (point-marker))) +> (widget-create 'push-button +> :notify #'notmuch-hello-widget-search +> :notmuch-search-terms query +> @@ -589,7 +570,6 @@ Complete list of currently available key bindings: +> (defun notmuch-hello-insert-search () +> "Insert a search widget." +> (widget-insert "Search: ") +> - (setq notmuch-hello-search-pos (point-marker)) +> (widget-create 'editable-field +> ;; Leave some space at the start and end of the +> ;; search boxes. +> @@ -763,13 +743,7 @@ following: +> (set-buffer "*notmuch-hello*") +> (switch-to-buffer "*notmuch-hello*")) +> +> - (let ((notmuch-hello-target (if (widget-at) +> - (widget-value (widget-at)) +> - (condition-case nil +> - (progn +> - (widget-forward 1) +> - (widget-value (widget-at))) +> - (error nil)))) +> + (let ((final-target-pos (point)) +> (inhibit-read-only t)) +> +> ;; Delete all editable widget fields. Editable widget fields are +> @@ -788,30 +762,20 @@ following: +> (mapc 'delete-overlay (car all)) +> (mapc 'delete-overlay (cdr all))) +> +> - (let (final-target-pos) +> - (mapc +> - (lambda (section) +> - (let ((point-before (point)) +> - (result (if (functionp section) +> - (funcall section) +> - (apply (car section) (cdr section))))) +> - (if (and (not final-target-pos) (integer-or-marker-p result)) +> - (setq final-target-pos result)) +> - ;; don't insert a newline when the previous section didn't show +> - ;; anything. +> - (unless (eq (point) point-before) +> - (widget-insert "\n")))) +> - notmuch-hello-sections) +> - (widget-setup) +> - +> - (when final-target-pos +> - (goto-char final-target-pos) +> - (unless (widget-at) +> - (widget-forward 1))) +> - +> - (unless (widget-at) +> - (when notmuch-hello-search-pos +> - (goto-char notmuch-hello-search-pos))))) +> + (mapc +> + (lambda (section) +> + (let ((point-before (point))) +> + (if (functionp section) +> + (funcall section) +> + (apply (car section) (cdr section))) +> + ;; don't insert a newline when the previous section didn't +> + ;; show anything. +> + (unless (eq (point) point-before) +> + (widget-insert "\n")))) +> + notmuch-hello-sections) +> + (widget-setup) +> + +> + (goto-char final-target-pos)) +> (run-hooks 'notmuch-hello-refresh-hook) +> (setq notmuch-hello-first-run nil)) +> + +-- +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. -- 2.26.2