From eb1359a88a5474a94b326f146f75ce8cc4088c9a Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Tue, 17 Apr 2012 09:58:11 +0000 Subject: [PATCH] Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello --- c8/f6b447eba681e9d1623520f03fedf0b415f104 | 295 ++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 c8/f6b447eba681e9d1623520f03fedf0b415f104 diff --git a/c8/f6b447eba681e9d1623520f03fedf0b415f104 b/c8/f6b447eba681e9d1623520f03fedf0b415f104 new file mode 100644 index 000000000..be5c29acd --- /dev/null +++ b/c8/f6b447eba681e9d1623520f03fedf0b415f104 @@ -0,0 +1,295 @@ +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 8E002431FB6 + for ; Tue, 17 Apr 2012 02:58:20 -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 cFHKOenqjoIz for ; + Tue, 17 Apr 2012 02:58:16 -0700 (PDT) +Received: from mail-qc0-f181.google.com (mail-qc0-f181.google.com + [209.85.216.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 9F55F431FAE + for ; Tue, 17 Apr 2012 02:58:16 -0700 (PDT) +Received: by qcsk26 with SMTP id k26so4648239qcs.26 + for ; Tue, 17 Apr 2012 02:58:16 -0700 (PDT) +X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; + d=google.com; s=20120113; + h=from:to:subject:in-reply-to:references:user-agent:date:message-id + :mime-version:content-type:x-gm-message-state; + bh=Bty/NDpfJZnspuai5BZaVMK5l5phv4gf6byMKxbrBYg=; + b=OKvljrvm5LyaO9NFJEMTCm6FFhOTVQj4b4/99Trt1aImELuTaIAB9wHxjw4gtYIGGU + dackCLLBw0FRFNSiikP+KnEZa2GTgBSIShJZ8Fo3Scv5JtXsIjylQ56oyb4K9pywGFBI + 2BS78mpQ6IFvlnzQXQRqlmd4EysyGu5cSnrTHccWRnL46yO8YEHxAQ+FmFH6NaBKDuVW + k+S5Elow62ByGRf+pspy3/RCIFfNMkJEAyoN1LndNKXWXxNpIVWlyzCmPCGGsYwuhZOU + vhf7On7GXJZAPnOMW6jsO9KTALwHUMoE5eX+qnvu5HS1RSRaxObnNxyA3pDSBsRiiWjw + jDgw== +Received: by 10.224.39.211 with SMTP id h19mr19924717qae.24.1334656695982; + Tue, 17 Apr 2012 02:58:15 -0700 (PDT) +Received: from localhost (nikula.org. [92.243.24.172]) + by mx.google.com with ESMTPS id g10sm2058363qae.11.2012.04.17.02.58.13 + (version=SSLv3 cipher=OTHER); Tue, 17 Apr 2012 02:58:15 -0700 (PDT) +From: Jani Nikula +To: Dmitry Kurochkin , notmuch@notmuchmail.org +Subject: Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello +In-Reply-To: <87aa2aohjs.fsf@gmail.com> +References: + + <87aa2aohjs.fsf@gmail.com> +User-Agent: Notmuch/0.11.1+222~ga47a98c (http://notmuchmail.org) Emacs/23.1.1 + (i686-pc-linux-gnu) +Date: Tue, 17 Apr 2012 09:58:11 +0000 +Message-ID: <878vhuvfws.fsf@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Gm-Message-State: + ALoCoQmHXQbNirF8HlPXP/3lngz68n4Xjn/PCBBS4rytiR0cGhXnK7XWrbjVB15/uZZnUcckd1d/ +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:58:20 -0000 + +On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin wrote: +> 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? + +My approach was this: keep it extremely simple, and catch the low +hanging fruit. It places the point where I want, say, 90% of the +time. And when it's wrong, it's not far off. In contrast, the current +implementation places the cursor exactly where I do *not* want it about +half the time. + +What you describe sounds smart, but complicated. Maybe it just sounds +complicated from my limited lisp skills perspective. Personally I don't +think sections should have to implement their own logic for point +placement. And as a fallback, I prefer keeping the point where it is +instead of moving it to the beginning of buffer. + +I don't oppose to your plan, but I don't think I'm up to implementing it +either. I just cooked up something together to fix the #1 annoyance I've +lately had with notmuch, and Tomi persuaded me to share the patches as +RFC. I still think it's pretty good, considering the simplicity of it +all, but certainly not perfect. + + +BR, +Jani. + + +> +> 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 -- 2.26.2