On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin <dmitry.kuroch...@gmail.com> wrote: > Hi Jani. > > Jani Nikula <j...@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@notmuchmail.org > > http://notmuchmail.org/mailman/listinfo/notmuch _______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch