29/6280c1beff6edee785e9add2845e4c745951b2

   1 Return-Path: <tomi.ollila@iki.fi>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id BF663431FC2\r
   6         for <notmuch@notmuchmail.org>; Sun, 30 Sep 2012 01:53:21 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 0\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
  12         autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id H+5tNzIE0XlU for <notmuch@notmuchmail.org>;\r
  16         Sun, 30 Sep 2012 01:53:19 -0700 (PDT)\r
  17 Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34])\r
  18         by olra.theworths.org (Postfix) with ESMTP id 6B203431FBC\r
  19         for <notmuch@notmuchmail.org>; Sun, 30 Sep 2012 01:53:19 -0700 (PDT)\r
  20 Received: from guru.guru-group.fi (localhost [IPv6:::1])\r
  21         by guru.guru-group.fi (Postfix) with ESMTP id 2A6F01000D0;\r
  22         Sun, 30 Sep 2012 11:53:23 +0300 (EEST)\r
  23 From: Tomi Ollila <tomi.ollila@iki.fi>\r
  24 To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
  25 Subject: Re: [PATCH v2] emacs: simplify point placement in notmuch-hello\r
  26         refresh\r
  27 In-Reply-To: <1348990572-7860-1-git-send-email-jani@nikula.org>\r
  28 References: <1348990572-7860-1-git-send-email-jani@nikula.org>\r
  29 User-Agent: Notmuch/0.14+48~g640455c (http://notmuchmail.org) Emacs/24.2.1\r
  30         (x86_64-unknown-linux-gnu)\r
  31 X-Face: HhBM'cA~<r"^Xv\KRN0P{vn'Y"Kd;zg_y3S[4)KSN~s?O\"QPoL\r
  32         $[Xv_BD:i/F$WiEWax}R(MPS`^UaptOGD`*/=@\1lKoVa9tnrg0TW?"r7aRtgk[F\r
  33         !)g;OY^,BjTbr)Np:%c_o'jj,Z\r
  34 Date: Sun, 30 Sep 2012 11:53:22 +0300\r
  35 Message-ID: <m28vbr7uyl.fsf@guru.guru-group.fi>\r
  36 MIME-Version: 1.0\r
  37 Content-Type: text/plain\r
  38 X-BeenThere: notmuch@notmuchmail.org\r
  39 X-Mailman-Version: 2.1.13\r
  40 Precedence: list\r
  41 List-Id: "Use and development of the notmuch mail system."\r
  42         <notmuch.notmuchmail.org>\r
  43 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  44         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  45 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  46 List-Post: <mailto:notmuch@notmuchmail.org>\r
  47 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  48 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  49         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  50 X-List-Received-Date: Sun, 30 Sep 2012 08:53:21 -0000\r
  51 \r
  52 On Sun, Sep 30 2012, Jani Nikula <jani@nikula.org> wrote:\r
  53 \r
  54 > notmuch-hello (called also through notmuch-hello-update, bound to '='\r
  55 > by default) tries to find the widget under or following point before\r
  56 > refresh, and put the point back to the widget afterwards. The code has\r
  57 > grown quite complicated, and has at least the following issues:\r
  58 >\r
  59 > 1) All the individual section functions have to include code to\r
  60 >    support point placement. If there is no such support, point is\r
  61 >    dropped to the search box. Only saved searches and all tags\r
  62 >    sections support point placement.\r
  63 >\r
  64 > 2) Point placement is based on widget-value. If there are two widgets\r
  65 >    with the same widget-value (for example a saved search with the\r
  66 >    same name as a tag) the point is moved to the earlier one, even if\r
  67 >    point was on the later one.\r
  68 >\r
  69 > 3) When first entering notmuch-hello notmuch-hello-target is nil, and\r
  70 >    point is dropped to the search box.\r
  71 >\r
  72 > Moving the point to the search box is annoying because the user is\r
  73 > required to move the point before being able to enter key bindings.\r
  74 >\r
  75 > Simplify the code by removing all point placement based on widgets, as\r
  76 > it does not work properly, and trying to fix that would unnecessarily\r
  77 > complicate the code.\r
  78 >\r
  79 > Save current line and column before refresh, and restore them\r
  80 > afterwards. Sometimes, if notmuch-show-empty-saved-searches is nil,\r
  81 > and the refresh adds or removes saved searches from the list, this has\r
  82 > the appearance of moving the point relative to the nearest\r
  83 > widgets. This is a much smaller and less frequent problem than the\r
  84 > ones listed above.\r
  85 \r
  86 LGTM.\r
  87 \r
  88 Tomi\r
  89 \r
  90 \r
  91 >\r
  92 > ---\r
  93 >\r
  94 > v2 of id:"1348958664-1767-1-git-send-email-jani@nikula.org": More\r
  95 > (found-)target-pos cleanup, and use line and column instead of point\r
  96 > directly, both suggested by Austin.\r
  97 > ---\r
  98 >  emacs/notmuch-hello.el |  108 ++++++++++++++++--------------------------------\r
  99 >  1 file changed, 35 insertions(+), 73 deletions(-)\r
 100 >\r
 101 > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el\r
 102 > index 684bedc..052aaeb 100644\r
 103 > --- a/emacs/notmuch-hello.el\r
 104 > +++ b/emacs/notmuch-hello.el\r
 105 > @@ -154,11 +154,6 @@ International Bureau of Weights and Measures."\r
 106 >  (defvar notmuch-hello-url "http://notmuchmail.org"\r
 107 >    "The `notmuch' web site.")\r
 108 >  \r
 109 > -(defvar notmuch-hello-search-pos nil\r
 110 > -  "Position of search widget, if any.\r
 111 > -\r
 112 > -This should only be set by `notmuch-hello-insert-search'.")\r
 113 > -\r
 114 >  (defvar notmuch-hello-custom-section-options\r
 115 >    '((:filter (string :tag "Filter for each tag"))\r
 116 >      (:filter-count (string :tag "Different filter to generate message counts"))\r
 117 > @@ -209,11 +204,8 @@ function produces a section simply by adding content to the current\r
 118 >  buffer.  A section should not end with an empty line, because a\r
 119 >  newline will be inserted after each section by `notmuch-hello'.\r
 120 >  \r
 121 > -Each function should take no arguments.  If the produced section\r
 122 > -includes `notmuch-hello-target' (i.e. cursor should be positioned\r
 123 > -inside this section), the function should return this element's\r
 124 > -position.\r
 125 > -Otherwise, it should return nil.\r
 126 > +Each function should take no arguments. The return value is\r
 127 > +ignored.\r
 128 >  \r
 129 >  For convenience an element can also be a list of the form (FUNC ARG1\r
 130 >  ARG2 .. ARGN) in which case FUNC will be applied to the rest of the\r
 131 > @@ -240,15 +232,6 @@ supported for \"Customized queries section\" items."\r
 132 >           notmuch-hello-query-section\r
 133 >           (function :tag "Custom section"))))\r
 134 >  \r
 135 > -(defvar notmuch-hello-target nil\r
 136 > -  "Button text at position of point before rebuilding the notmuch-buffer.\r
 137 > -\r
 138 > -This variable contains the text of the button, if any, the\r
 139 > -point was positioned at before the notmuch-hello buffer was\r
 140 > -rebuilt. This should never actually be global and is defined as a\r
 141 > -defvar only for documentation purposes and to avoid a compiler\r
 142 > -warning about it occurring as a free variable.")\r
 143 > -\r
 144 >  (defvar notmuch-hello-hidden-sections nil\r
 145 >    "List of sections titles whose contents are hidden")\r
 146 >  \r
 147 > @@ -435,8 +418,7 @@ Such a list can be computed with `notmuch-hello-query-counts'."\r
 148 >        (reordered-list (notmuch-hello-reflect searches tags-per-line))\r
 149 >        ;; Hack the display of the buttons used.\r
 150 >        (widget-push-button-prefix "")\r
 151 > -      (widget-push-button-suffix "")\r
 152 > -      (found-target-pos nil))\r
 153 > +      (widget-push-button-suffix ""))\r
 154 >      ;; dme: It feels as though there should be a better way to\r
 155 >      ;; implement this loop than using an incrementing counter.\r
 156 >      (mapc (lambda (elem)\r
 157 > @@ -449,8 +431,6 @@ Such a list can be computed with `notmuch-hello-query-counts'."\r
 158 >                    (msg-count (third elem)))\r
 159 >               (widget-insert (format "%8s "\r
 160 >                                      (notmuch-hello-nice-number msg-count)))\r
 161 > -             (if (string= name notmuch-hello-target)\r
 162 > -                 (setq found-target-pos (point-marker)))\r
 163 >               (widget-create 'push-button\r
 164 >                              :notify #'notmuch-hello-widget-search\r
 165 >                              :notmuch-search-terms query\r
 166 > @@ -466,8 +446,7 @@ Such a list can be computed with `notmuch-hello-query-counts'."\r
 167 >      ;; If the last line was not full (and hence did not include a\r
 168 >      ;; carriage return), insert one now.\r
 169 >      (unless (eq (% count tags-per-line) 0)\r
 170 > -      (widget-insert "\n"))\r
 171 > -    found-target-pos))\r
 172 > +      (widget-insert "\n"))))\r
 173 >  \r
 174 >  (defimage notmuch-hello-logo ((:type png :file "notmuch-logo.png")))\r
 175 >  \r
 176 > @@ -571,8 +550,7 @@ Complete list of currently available key bindings:\r
 177 >                      (funcall notmuch-saved-search-sort-function\r
 178 >                               notmuch-saved-searches)\r
 179 >                    notmuch-saved-searches)\r
 180 > -                :show-empty-searches notmuch-show-empty-saved-searches))\r
 181 > -     found-target-pos)\r
 182 > +                :show-empty-searches notmuch-show-empty-saved-searches)))\r
 183 >      (when searches\r
 184 >        (widget-insert "Saved searches: ")\r
 185 >        (widget-create 'push-button\r
 186 > @@ -581,15 +559,12 @@ Complete list of currently available key bindings:\r
 187 >                    "edit")\r
 188 >        (widget-insert "\n\n")\r
 189 >        (let ((start (point)))\r
 190 > -     (setq found-target-pos\r
 191 > -           (notmuch-hello-insert-buttons searches))\r
 192 > -     (indent-rigidly start (point) notmuch-hello-indent)\r
 193 > -     found-target-pos))))\r
 194 > +     (notmuch-hello-insert-buttons searches)\r
 195 > +     (indent-rigidly start (point) notmuch-hello-indent)))))\r
 196 >  \r
 197 >  (defun notmuch-hello-insert-search ()\r
 198 >    "Insert a search widget."\r
 199 >    (widget-insert "Search: ")\r
 200 > -  (setq notmuch-hello-search-pos (point-marker))\r
 201 >    (widget-create 'editable-field\r
 202 >                ;; Leave some space at the start and end of the\r
 203 >                ;; search boxes.\r
 204 > @@ -689,16 +664,13 @@ Supports the following entries in OPTIONS as a plist:\r
 205 >                               (notmuch-hello-update))\r
 206 >                    "hide"))\r
 207 >      (widget-insert "\n")\r
 208 > -    (let (target-pos)\r
 209 > -      (when (not is-hidden)\r
 210 > -     (let ((searches (apply 'notmuch-hello-query-counts query-alist options)))\r
 211 > -       (when (or (not (plist-get options :hide-if-empty))\r
 212 > -                 searches)\r
 213 > -         (widget-insert "\n")\r
 214 > -         (setq target-pos\r
 215 > -               (notmuch-hello-insert-buttons searches))\r
 216 > -         (indent-rigidly start (point) notmuch-hello-indent))))\r
 217 > -      target-pos)))\r
 218 > +    (when (not is-hidden)\r
 219 > +      (let ((searches (apply 'notmuch-hello-query-counts query-alist options)))\r
 220 > +     (when (or (not (plist-get options :hide-if-empty))\r
 221 > +               searches)\r
 222 > +       (widget-insert "\n")\r
 223 > +       (notmuch-hello-insert-buttons searches)\r
 224 > +       (indent-rigidly start (point) notmuch-hello-indent))))))\r
 225 >  \r
 226 >  (defun notmuch-hello-insert-tags-section (&optional title &rest options)\r
 227 >    "Insert a section displaying all tags with message counts.\r
 228 > @@ -763,13 +735,8 @@ following:\r
 229 >        (set-buffer "*notmuch-hello*")\r
 230 >      (switch-to-buffer "*notmuch-hello*"))\r
 231 >  \r
 232 > -  (let ((notmuch-hello-target (if (widget-at)\r
 233 > -                               (widget-value (widget-at))\r
 234 > -                             (condition-case nil\r
 235 > -                                 (progn\r
 236 > -                                   (widget-forward 1)\r
 237 > -                                   (widget-value (widget-at)))\r
 238 > -                               (error nil))))\r
 239 > +  (let ((target-line (line-number-at-pos))\r
 240 > +     (target-column (current-column))\r
 241 >       (inhibit-read-only t))\r
 242 >  \r
 243 >      ;; Delete all editable widget fields.  Editable widget fields are\r
 244 > @@ -788,30 +755,25 @@ following:\r
 245 >        (mapc 'delete-overlay (car all))\r
 246 >        (mapc 'delete-overlay (cdr all)))\r
 247 >  \r
 248 > -    (let (final-target-pos)\r
 249 > -      (mapc\r
 250 > -       (lambda (section)\r
 251 > -      (let ((point-before (point))\r
 252 > -            (result (if (functionp section)\r
 253 > -                        (funcall section)\r
 254 > -                      (apply (car section) (cdr section)))))\r
 255 > -        (if (and (not final-target-pos) (integer-or-marker-p result))\r
 256 > -            (setq final-target-pos result))\r
 257 > -        ;; don't insert a newline when the previous section didn't show\r
 258 > -        ;; anything.\r
 259 > -        (unless (eq (point) point-before)\r
 260 > -          (widget-insert "\n"))))\r
 261 > -       notmuch-hello-sections)\r
 262 > -      (widget-setup)\r
 263 > -\r
 264 > -      (when final-target-pos\r
 265 > -     (goto-char final-target-pos)\r
 266 > -     (unless (widget-at)\r
 267 > -       (widget-forward 1)))\r
 268 > -\r
 269 > -      (unless (widget-at)\r
 270 > -     (when notmuch-hello-search-pos\r
 271 > -       (goto-char notmuch-hello-search-pos)))))\r
 272 > +    (mapc\r
 273 > +     (lambda (section)\r
 274 > +       (let ((point-before (point)))\r
 275 > +      (if (functionp section)\r
 276 > +          (funcall section)\r
 277 > +        (apply (car section) (cdr section)))\r
 278 > +      ;; don't insert a newline when the previous section didn't\r
 279 > +      ;; show anything.\r
 280 > +      (unless (eq (point) point-before)\r
 281 > +        (widget-insert "\n"))))\r
 282 > +     notmuch-hello-sections)\r
 283 > +    (widget-setup)\r
 284 > +\r
 285 > +    ;; Move point back to where it was before refresh. Use line and\r
 286 > +    ;; column instead of point directly to be insensitive to additions\r
 287 > +    ;; and removals of text within earlier lines.\r
 288 > +    (goto-char (point-min))\r
 289 > +    (forward-line (1- target-line))\r
 290 > +    (move-to-column target-column))\r
 291 >    (run-hooks 'notmuch-hello-refresh-hook)\r
 292 >    (setq notmuch-hello-first-run nil))\r
 293 >  \r
 294 > -- \r
 295 > 1.7.9.5\r
 296 >\r
 297 > _______________________________________________\r
 298 > notmuch mailing list\r
 299 > notmuch@notmuchmail.org\r
 300 > http://notmuchmail.org/mailman/listinfo/notmuch\r