[RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

Tue Apr 17 02:58:11 PDT 2012

On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin <dmitry.kurochkin at gmail.com> wrote:
> Hi Jani.
> 
> Jani Nikula <jani at nikula.org> 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 at notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch