bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

Hello, Juri.

On Tue, Dec 01, 2015 at 02:07:31AM +0200, Juri Linkov wrote:
> >> The patch in bug#20430 awaits the possibility of helping to fix this
> >> problem.  It adds a new hook replace-update-post-hook that is like
> >> its isearch counterpart hook isearch-update-post-hook is the right way
> >> to handle display updates like syncing follow windows, etc.

> > Does this patch exist, yet?

> Yes, you can find the patch in http://debbugs.gnu.org/20430#8

OK, I've got it.

> > It bothers me a little that we might be adding hook after hook into
> > Emacs, each one for a single special purpose.

> Fortunately, a new hook is not for a single special purpose -
> it's a general type of hook, requested for different user needs.

I'll stop arguing the philosophy right now (and take it up again a bit
lower down).  ;-)

> > Would it not perhaps be better to call `isearch-update-post-hook' also
> > from `perform-replace', since that would be more economical with hooks;
> > the meaning of the hook invocation would be "the same" in Isearch and
> > `perform-replace' - "hook called after having moved to the next match".

> Logically, it makes sense to reuse isearch hooks in query-replace
> since query-replace searches for matches like isearch does, but
> practically users might have such customizations in .emacs that
> would break query-replace in unpredictable ways.  This is why
> a separate query-replace hook would be much safer.

> I see no problem for follow-mode to add follow-post-command-hook
> to both hooks.

Because this ties Follow Mode to implementation details of isearch.el,
replace.el, and ispell.el.  It is plain ugly for Follow Mode to have to
add-hook an obscure hook for every package that attempts lazy
highlighting.

    (add-hook 'post-command-hook follow-post-command-hook nil t)

should be the only pertinent 'add-hook necessary in follow-mode (and it
would be if isearch.el were the only library to adpat for - I had it
working at one point).  It's not, because replace.el and ispell.el both
attempt to implement their own command loops rather than using Emacs's
standard one.  These design decisions were Bad.

> >> Together with changing the order of calling isearch-update-post-hook
> >> and isearch-lazy-highlight-new-loop in isearch-update, adding
> >> follow-post-command-hook to isearch-update-post-hook, and adding
> >> follow-post-command-hook to replace-update-post-hook to handle
> >> follow-mode in query-replace will comprise the least radical change
> >> just before the next release.

> > This sounds like a good idea.  Though, again, I think calling
> > isearch-update-post-hook from `query-replace' would be better than
> > implementing a new hook.

> Adding a new hook is just a one-liner, but we have to find the right place
> in perform-replace to call it.  I think replace-update-pre-hook should be
> called before (read-event), and replace-update-post-hook after (read-event).
> I'm not yet sure which one is needed for follow-mode to sync windows?

At the moment there is only replace-update-post-hook.  It is called after
point has been moved, but before i-l-highlight-new-loop is called.  I had
to move it slightly from where your patch had put it.

> > Would it still be possible to mark `isearch-update-post-hook' as "for
> > internal use only", so that we could get rid of it later?

> isearch-update-post-hook is a first-class hook added 5 years ago,
> so there is no need to remove it.

Sorry, I made a typo there.  I really meant replace-update-post-hook.
Can we somehow keep this "internal use only", so that we are not bound
somehow to keep supporting it should the `query-replace' command loop be
reformulated (as believe it should, ASAP)?  The same applies to
ispell-update-post-hook, which I've been forced to introduce into
ispell.el for the same reason.

#########################################################################

Anyhow, here's a status update with where I am on making isearch.el and
follow.el work together:

(i) Yesterday I rebased the scratch/follow branch on the emacs-25 branch.
(ii) I haven't yet replaced the GROUP parameter in the windows primitives
  with (e.g.) `window-group-start'.
(iii) isearch.el now appears to work properly.  For this, I had to swap
  the order of invocation of isearch-update-post-hook and i-l-h-new-loop,
  like you said.  I restored i-l-h-new-loop pretty much to the way it was
  prior to my experimentations.
(iv) replace.el now appears to work properly.
(v) ispell.el is more troublesome.  See bug #22097.  I have a problem
  with ispell putting its *Choices* window at the top of the left hand
  Follow Mode window.  Because of FM's sorting algorithm, this causes the
  two windows to be logically swapped, leading to confusing results.  I
  think the neatest solution would be to put *Choices* at the top of the
  rightmost window, preventing this.

Here's a diff of my current state, based off of the rebased
scratch/follow branch mentioned above in (i):



diff --git a/.gitignore b/.gitignore
index 34b0c02..5ef5a5c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -261,6 +261,10 @@ ChangeLog
 [0-9]*.txt
 .dir-locals?.el
 /vc-dwim-log-*
+*.diff
+
+.gitattributes
+*.acm
 
 # Built by 'make install'.
 etc/emacs.tmpdesktop
diff --git a/lisp/follow.el b/lisp/follow.el
index 2cbf0f2..609b29f 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -423,6 +423,9 @@ follow-mode
        (add-hook 'post-command-hook 'follow-post-command-hook t)
        (add-hook 'window-size-change-functions 'follow-window-size-change t)
         (add-hook 'after-change-functions 'follow-after-change nil t)
+        (add-hook 'isearch-update-post-hook 'follow-post-command-hook nil t)
+        (add-hook 'replace-update-post-hook 'follow-post-command-hook nil t)
+        (add-hook 'ispell-update-post-hook 'follow-post-command-hook nil t)
 
         (setq window-start-group-function 'follow-window-start)
         (setq window-end-group-function 'follow-window-end)
@@ -431,8 +434,7 @@ follow-mode
         (setq pos-visible-in-window-p-group-function
               'follow-pos-visible-in-window-p)
         (setq selected-window-group-function 'follow-all-followers)
-        (setq move-to-window-line-group-function 'follow-move-to-window-line)
-        (setq sit*-for-function 'follow-sit-for))
+        (setq move-to-window-line-group-function 'follow-move-to-window-line))
 
     ;; Remove globally-installed hook functions only if there is no
     ;; other Follow mode buffer.
@@ -445,7 +447,6 @@ follow-mode
        (remove-hook 'post-command-hook 'follow-post-command-hook)
        (remove-hook 'window-size-change-functions 'follow-window-size-change)))
 
-    (kill-local-variable 'sit*-for-function)
     (kill-local-variable 'move-to-window-line-group-function)
     (kill-local-variable 'selected-window-group-function)
     (kill-local-variable 'pos-visible-in-window-p-group-function)
@@ -454,6 +455,9 @@ follow-mode
     (kill-local-variable 'window-end-group-function)
     (kill-local-variable 'window-start-group-function)
 
+    (remove-hook 'ispell-update-post-hook 'follow-post-command-hook t)
+    (remove-hook 'replace-update-post-hook 'follow-post-command-hook t)
+    (remove-hook 'isearch-update-post-hook 'follow-post-command-hook t)
     (remove-hook 'after-change-functions 'follow-after-change t)
     (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows 
t)))
 
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 12ded02..e43d860 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1018,12 +1018,12 @@ isearch-update
   (setq ;; quit-flag nil  not for isearch-mode
    isearch-adjusted nil
    isearch-yank-flag nil)
-  (when isearch-lazy-highlight
-    (isearch-lazy-highlight-new-loop))
   ;; We must prevent the point moving to the end of composition when a
   ;; part of the composition has just been searched.
   (setq disable-point-adjustment t)
-  (run-hooks 'isearch-update-post-hook))
+  (run-hooks 'isearch-update-post-hook)
+  (when isearch-lazy-highlight
+    (isearch-lazy-highlight-new-loop)))
 
 (defun isearch-done (&optional nopush edit)
   "Exit Isearch mode.
@@ -3068,21 +3068,7 @@ 'isearch-lazy-highlight-cleanup
                                 "22.1")
 
 (defun isearch-lazy-highlight-new-loop (&optional beg end)
-  "Set an idle timer, which will trigger a new `lazy-highlight' loop.
-BEG and END specify the bounds within which highlighting should
-occur.  This is called when `isearch-update' is invoked (which
-can cause the search string to change or the window(s) to
-scroll).  It is also used by other Emacs features.  Do not start
-the loop when we are executing a keyboard macro."
-  (setq isearch-lazy-highlight-start-limit beg
-        isearch-lazy-highlight-end-limit end)
-  (when (null executing-kbd-macro)
-    (setq isearch-lazy-highlight-timer
-          (run-with-idle-timer lazy-highlight-initial-delay nil
-                               'isearch-lazy-highlight-maybe-new-loop))))
-
-(defun isearch-lazy-highlight-maybe-new-loop ()
-  "If needed cleanup any previous `lazy-highlight' loop and begin a new one.
+  "Cleanup any previous `lazy-highlight' loop and begin a new one.
 BEG and END specify the bounds within which highlighting should occur.
 This is called when `isearch-update' is invoked (which can cause the
 search string to change or the window to scroll).  It is also used
@@ -3118,6 +3104,8 @@ isearch-lazy-highlight-maybe-new-loop
     ;; It used to check for `(not isearch-error)' here, but actually
     ;; lazy-highlighting might find matches to highlight even when
     ;; `isearch-error' is non-nil.  (Bug#9918)
+    (setq isearch-lazy-highlight-start-limit beg
+         isearch-lazy-highlight-end-limit end)
     (setq isearch-lazy-highlight-window       (selected-window)
           isearch-lazy-highlight-window-group (selected-window-group)
          isearch-lazy-highlight-window-start (window-start nil t)
@@ -3140,7 +3128,9 @@ isearch-lazy-highlight-maybe-new-loop
          isearch-lazy-highlight-regexp-function  isearch-regexp-function
          isearch-lazy-highlight-forward      isearch-forward)
     (unless (equal isearch-string "")
-      (isearch-lazy-highlight-update))))
+      (setq isearch-lazy-highlight-timer
+            (run-with-idle-timer lazy-highlight-initial-delay nil
+                                 'isearch-lazy-highlight-update)))))
 
 (defun isearch-lazy-highlight-search ()
   "Search ahead for the next or previous match, for lazy highlighting.
diff --git a/lisp/replace.el b/lisp/replace.el
index 54b3a71..d48f4f3 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2011,6 +2011,9 @@ replace-match-maybe-edit
   (when backward (goto-char (nth 0 match-data)))
   noedit)
 
+(defvar replace-update-post-hook nil
+  "Function(s) to call after query-replace has found a match in the buffer.")
+
 (defvar replace-search-function nil
   "Function to use when searching for strings to replace.
 It is used by `query-replace' and `replace-string', and is called
@@ -2264,7 +2267,7 @@ perform-replace
                (and nonempty-match
                     (or (not regexp-flag)
                         (and (if backward
-                                 (looking-back search-string)
+                                 (looking-back search-string nil)
                                (looking-at search-string))
                              (let ((match (match-data)))
                                (and (/= (nth 0 match) (nth 1 match))
@@ -2318,7 +2321,8 @@ perform-replace
                ;; `real-match-data'.
                (while (not done)
                  (set-match-data real-match-data)
-                 (replace-highlight
+                  (run-hooks 'replace-update-post-hook) ; Before 
`replace-highlight'.
+                  (replace-highlight
                   (match-beginning 0) (match-end 0)
                   start end search-string
                   regexp-flag delimited-flag case-fold-search backward)
diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index fe27f0f..fae0549 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -2248,6 +2248,11 @@ ispell-pdict-save
   (setq ispell-pdict-modified-p nil))
 
 
+(defvar ispell-update-post-hook nil
+  "A normal hook invoked from the ispell command loop.
+It is called once per iteration, before displaying a prompt to
+the user.")
+
 (defun ispell-command-loop (miss guess word start end)
   "Display possible corrections from list MISS.
 GUESS lists possibly valid affix construction of WORD.
@@ -2315,8 +2320,10 @@ ispell-command-loop
              count (ispell-int-char (1+ count))))
       (setq count (ispell-int-char (- count ?0 skipped))))
 
+    (run-hooks 'ispell-update-post-hook)
+
     ;; ensure word is visible
-    (if (not (pos-visible-in-window-p end))
+    (if (not (pos-visible-in-window-p end nil nil t))
        (sit-for 0))
 
     ;; Display choices for misspelled word.


-- 
Alan Mackenzie (Nuremberg, Germany).