From 78220cea1818180994b3a854bff94b8790305c8f Mon Sep 17 00:00:00 2001 From: Tomi Ollila Date: Sun, 30 Sep 2012 11:53:22 +0300 Subject: [PATCH] Re: [PATCH v2] emacs: simplify point placement in notmuch-hello refresh --- 29/6280c1beff6edee785e9add2845e4c745951b2 | 300 ++++++++++++++++++++++ 1 file changed, 300 insertions(+) create mode 100644 29/6280c1beff6edee785e9add2845e4c745951b2 diff --git a/29/6280c1beff6edee785e9add2845e4c745951b2 b/29/6280c1beff6edee785e9add2845e4c745951b2 new file mode 100644 index 000000000..95ce0fd3a --- /dev/null +++ b/29/6280c1beff6edee785e9add2845e4c745951b2 @@ -0,0 +1,300 @@ +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 BF663431FC2 + for ; Sun, 30 Sep 2012 01:53:21 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + 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 H+5tNzIE0XlU for ; + Sun, 30 Sep 2012 01:53:19 -0700 (PDT) +Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) + by olra.theworths.org (Postfix) with ESMTP id 6B203431FBC + for ; Sun, 30 Sep 2012 01:53:19 -0700 (PDT) +Received: from guru.guru-group.fi (localhost [IPv6:::1]) + by guru.guru-group.fi (Postfix) with ESMTP id 2A6F01000D0; + Sun, 30 Sep 2012 11:53:23 +0300 (EEST) +From: Tomi Ollila +To: Jani Nikula , notmuch@notmuchmail.org +Subject: Re: [PATCH v2] emacs: simplify point placement in notmuch-hello + refresh +In-Reply-To: <1348990572-7860-1-git-send-email-jani@nikula.org> +References: <1348990572-7860-1-git-send-email-jani@nikula.org> +User-Agent: Notmuch/0.14+48~g640455c (http://notmuchmail.org) Emacs/24.2.1 + (x86_64-unknown-linux-gnu) +X-Face: HhBM'cA~ +MIME-Version: 1.0 +Content-Type: text/plain +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: Sun, 30 Sep 2012 08:53:21 -0000 + +On Sun, Sep 30 2012, Jani Nikula wrote: + +> 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. +> +> Save current line and column before refresh, and restore them +> afterwards. 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. + +LGTM. + +Tomi + + +> +> --- +> +> v2 of id:"1348958664-1767-1-git-send-email-jani@nikula.org": More +> (found-)target-pos cleanup, and use line and column instead of point +> directly, both suggested by Austin. +> --- +> emacs/notmuch-hello.el | 108 ++++++++++++++++-------------------------------- +> 1 file changed, 35 insertions(+), 73 deletions(-) +> +> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el +> index 684bedc..052aaeb 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") +> +> @@ -435,8 +418,7 @@ Such a list can be computed with `notmuch-hello-query-counts'." +> (reordered-list (notmuch-hello-reflect searches tags-per-line)) +> ;; Hack the display of the buttons used. +> (widget-push-button-prefix "") +> - (widget-push-button-suffix "") +> - (found-target-pos nil)) +> + (widget-push-button-suffix "")) +> ;; dme: It feels as though there should be a better way to +> ;; implement this loop than using an incrementing counter. +> (mapc (lambda (elem) +> @@ -449,8 +431,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 +> @@ -466,8 +446,7 @@ Such a list can be computed with `notmuch-hello-query-counts'." +> ;; If the last line was not full (and hence did not include a +> ;; carriage return), insert one now. +> (unless (eq (% count tags-per-line) 0) +> - (widget-insert "\n")) +> - found-target-pos)) +> + (widget-insert "\n")))) +> +> (defimage notmuch-hello-logo ((:type png :file "notmuch-logo.png"))) +> +> @@ -571,8 +550,7 @@ Complete list of currently available key bindings: +> (funcall notmuch-saved-search-sort-function +> notmuch-saved-searches) +> notmuch-saved-searches) +> - :show-empty-searches notmuch-show-empty-saved-searches)) +> - found-target-pos) +> + :show-empty-searches notmuch-show-empty-saved-searches))) +> (when searches +> (widget-insert "Saved searches: ") +> (widget-create 'push-button +> @@ -581,15 +559,12 @@ Complete list of currently available key bindings: +> "edit") +> (widget-insert "\n\n") +> (let ((start (point))) +> - (setq found-target-pos +> - (notmuch-hello-insert-buttons searches)) +> - (indent-rigidly start (point) notmuch-hello-indent) +> - found-target-pos)))) +> + (notmuch-hello-insert-buttons searches) +> + (indent-rigidly start (point) notmuch-hello-indent))))) +> +> (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. +> @@ -689,16 +664,13 @@ Supports the following entries in OPTIONS as a plist: +> (notmuch-hello-update)) +> "hide")) +> (widget-insert "\n") +> - (let (target-pos) +> - (when (not is-hidden) +> - (let ((searches (apply 'notmuch-hello-query-counts query-alist options))) +> - (when (or (not (plist-get options :hide-if-empty)) +> - searches) +> - (widget-insert "\n") +> - (setq target-pos +> - (notmuch-hello-insert-buttons searches)) +> - (indent-rigidly start (point) notmuch-hello-indent)))) +> - target-pos))) +> + (when (not is-hidden) +> + (let ((searches (apply 'notmuch-hello-query-counts query-alist options))) +> + (when (or (not (plist-get options :hide-if-empty)) +> + searches) +> + (widget-insert "\n") +> + (notmuch-hello-insert-buttons searches) +> + (indent-rigidly start (point) notmuch-hello-indent)))))) +> +> (defun notmuch-hello-insert-tags-section (&optional title &rest options) +> "Insert a section displaying all tags with message counts. +> @@ -763,13 +735,8 @@ 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 ((target-line (line-number-at-pos)) +> + (target-column (current-column)) +> (inhibit-read-only t)) +> +> ;; Delete all editable widget fields. Editable widget fields are +> @@ -788,30 +755,25 @@ 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) +> + +> + ;; Move point back to where it was before refresh. Use line and +> + ;; column instead of point directly to be insensitive to additions +> + ;; and removals of text within earlier lines. +> + (goto-char (point-min)) +> + (forward-line (1- target-line)) +> + (move-to-column target-column)) +> (run-hooks 'notmuch-hello-refresh-hook) +> (setq notmuch-hello-first-run nil)) +> +> -- +> 1.7.9.5 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch -- 2.26.2