From: Dmitry Kurochkin Date: Tue, 17 Apr 2012 09:04:39 +0000 (+0400) Subject: Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=a5e46cf7fdffffd777a8d5394437e860cd900500;p=notmuch-archives.git Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello --- diff --git a/22/2b7e2871686c124a04c9cb8c58bc5d43a59e18 b/22/2b7e2871686c124a04c9cb8c58bc5d43a59e18 new file mode 100644 index 000000000..ed12b88a5 --- /dev/null +++ b/22/2b7e2871686c124a04c9cb8c58bc5d43a59e18 @@ -0,0 +1,268 @@ +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 88FF1431FB6 + for ; Tue, 17 Apr 2012 02:06:36 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.799 +X-Spam-Level: +X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5 + tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, + FREEMAIL_FROM=0.001, 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 BVIk8pujlIwU for ; + Tue, 17 Apr 2012 02:06:32 -0700 (PDT) +Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com + [209.85.217.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id D7EB7431FAE + for ; Tue, 17 Apr 2012 02:06:31 -0700 (PDT) +Received: by lbdb5 with SMTP id b5so1760128lbd.26 + for ; Tue, 17 Apr 2012 02:06:30 -0700 (PDT) +DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; + h=from:to:subject:in-reply-to:references:user-agent:date:message-id + :mime-version:content-type; + bh=wRBPB2EEMEQaXTLKjKIq4GRxfKv+b6jRCRn/thl/mIg=; + b=eQNxLBpCsRwvv/eJMAyu1esaqKs2rRAnnvOZWTun6K6f7YCXCBZhx5hWf2xN/Ir0eF + eQ1ImeGCynmWkNeWFPP/k1KWgERecarVGUzpSyNFmGoPpr1mMT12bKotyPqW4ypWZfRc + yOOgLI1SMFQ+cQUuYnJOGsBtarLSZze8K61dKaeC93hi3c4JdkDJfOLnYCx3G0u+85il + X1cnlNJ3fYyaosuQwi3G1RExBKEHbYcCKRg65tsGo6Ep2W6mVFBKf7Ycvj4pju5nobBs + jn7H3/HRkf8eBZSxsI2w5cUpwLjdYzOlcRII3Bc8/Bca0f9i3//Xiv8ewEIE9+CyLQCW + 37YA== +Received: by 10.112.85.228 with SMTP id k4mr6500563lbz.76.1334653590068; + Tue, 17 Apr 2012 02:06:30 -0700 (PDT) +Received: from localhost ([91.144.186.21]) + by mx.google.com with ESMTPS id nv7sm10584298lab.9.2012.04.17.02.06.28 + (version=TLSv1/SSLv3 cipher=OTHER); + Tue, 17 Apr 2012 02:06:29 -0700 (PDT) +From: Dmitry Kurochkin +To: Jani Nikula , notmuch@notmuchmail.org +Subject: Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello +In-Reply-To: + +References: + +User-Agent: Notmuch/0.12+104~gf9ad237 (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Tue, 17 Apr 2012 13:04:39 +0400 +Message-ID: <87aa2aohjs.fsf@gmail.com> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +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: Tue, 17 Apr 2012 09:06:36 -0000 + +Hi Jani. + +Jani Nikula writes: + +> 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 +> gotten a bit 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. +> +> 3) When first entering notmuch-hello notmuch-hello-target is nil, and +> point is dropped to the search box. +> +> This patch simplifies the code by removing all point placement based +> on widgets. Point is simply saved before refresh, and put back to +> where it was. Sometimes, but not very often, this would have the +> appearance of moving the point relative to the nearest widgets. IMHO +> this is a minor problem compared to the issues listed above. +> +> A downside is that there's no visual cue (point movement) to indicate +> that refresh has finished. Then again, neither was there before, if +> point was at the beginning of a widget. + +Thanks for looking into this. This is an annoying issue indeed. And I +was thinking about fixing it myself. + +I am not sure I like the approach of moving the cursor to the same +position. It is common that buffer content would change significantly +after a refresh (e.g. after I archived all new mail). That would mean +the cursor would just randomly jump somewhere. IMO we should allow +smart cursor positioning which means that logic should go to individual +sections. I would propose the following plan: + +1. Remove special case for search box. No section should be special. + Moreover it is possible to remove it (I did it) and in that case the + cursor would be left at the end of the buffer. By default, the + cursor should be moved to the beginning of the buffer. + +2. Replace current cursor positioning logic with section specific code. + I.e. `notmuch-hello' would not do any cursor positioning (except for + item 1) but queries and tags section would save required state when a + button is clicked and the same section would use this state to + restore cursor position on refresh. What state should be saved would + depend on the section but we should at least save the section + name/ID. If during refresh no section set the cursor position, then + the cursor is moved to the beginning of the buffer. + +3. Provide a custom variable to set the default section to move the + cursor to. I.e. set the section name/ID part of the state from item + 2. Again, details on what the default position inside the section is + would depend on the section. For search box, it would be the input + field. For queries/tags it would be the first tag. + +Item 1 is pretty simple. The rest may be more tricky. What do you +think? + +Regards, + Dmitry + +> --- +> emacs/notmuch-hello.el | 70 +++++++++++------------------------------------ +> 1 files changed, 17 insertions(+), 53 deletions(-) +> +> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el +> index 71d37b8..9cd907a 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)) +> +> -- +> 1.7.1 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch