* bug#17453: Isearch doesn't work properly with Follow Mode.
@ 2014-05-09 22:44 Alan Mackenzie
2014-05-10 2:40 ` Stefan Monnier
[not found] ` <handler.17453.B.139967578531952.ack@debbugs.gnu.org>
0 siblings, 2 replies; 96+ messages in thread
From: Alan Mackenzie @ 2014-05-09 22:44 UTC (permalink / raw
To: 17453
Hi, Emacs.
Isearch doesn't work properly with Follow Mode. There follows a patch
which is a first attempt to fix this.
There are three main problems (there may be others):
1. Isearch commands stay in the same window when they ought to move
across the entire screen. This is only actually a problem with lazy
highlighting enabled.
2. Lazy highlighting is only applied to the current follow window, not
all of them.
3. Scroll commands cannot scroll out of the current window. They ought
to be able to scroll through any follow window.
Most of the exercise was straightforward. The only trouble came where
isearch-message (or isearch-message-function) was called from
isearch-search. There are circumstances (see comments in the code)
where calling isearch-message with follow-mode enabled can cause
unwanted vertical scrolling. This happens when point is temporarily
moved to isearch-other-end and then calling isearch-search.
To get round this I refactored isearch-message out of isearch-search and
inserted into the places just before isearch-search is called.
Here's the patch:
--- emacs/emacs.bzr/trunk/lisp/isearch.el 2014-03-03 11:50:56.000000000 +0000
+++ /home/acm/isearch.el 2014-05-09 22:10:29.000000000 +0000
@@ -166,6 +166,21 @@
:type 'boolean
:group 'isearch)
+(defvar isearch-windows nil
+ "List of windows active in the incremental search.
+This is either the ordered list of active windows administered by
+`follow-mode' or a list of the single window involved in the
+search.")
+
+(defmacro isearch-windows-start (&optional wins)
+ "Get the start point of the windows involved in the isearch."
+ `(window-start (car ,(if wins wins 'isearch-windows))))
+
+(defmacro isearch-windows-end (&optional wins update)
+ "Get the end point of the windows involved in the isearch."
+ `(window-end (car (last ,(if wins wins 'isearch-windows)))
+ ,@(if update `(,update))))
+
(defvar isearch-mode-hook nil
"Function(s) to call after starting up an incremental search.")
@@ -184,6 +199,11 @@
"Function to call to display the search prompt.
If nil, use function `isearch-message'.")
+(defmacro isearch-call-message (&optional cqh ellip)
+ `(if isearch-message-function
+ (funcall isearch-message-function ,cqh ,ellip)
+ (isearch-message ,cqh ,ellip)))
+
(defvar isearch-wrap-function nil
"Function to call to wrap the search when search is failed.
If nil, move point to the beginning of the buffer for a forward search,
@@ -203,7 +223,7 @@
`isearch-message-prefix' advice property to specify the prefix string
displayed in the search message.")
-;; Search ring.
+;;; Search ring.
(defvar search-ring nil
"List of search string sequences.")
@@ -860,6 +880,9 @@
isearch-regexp regexp
isearch-word word
isearch-op-fun op-fun
+ isearch-windows (if (and (boundp 'follow-mode) follow-mode)
+ (follow-all-followers)
+ (list (selected-window)))
isearch-last-case-fold-search isearch-case-fold-search
isearch-case-fold-search case-fold-search
isearch-invisible search-invisible
@@ -1254,13 +1277,6 @@
(unwind-protect
(progn ,@body)
- ;; Set point at the start (end) of old match if forward (backward),
- ;; so after exiting minibuffer isearch resumes at the start (end)
- ;; of this match and can find it again.
- (if (and old-other-end (eq old-point (point))
- (eq isearch-forward isearch-new-forward))
- (goto-char old-other-end))
-
;; Always resume isearching by restarting it.
(isearch-mode isearch-forward
isearch-regexp
@@ -1273,7 +1289,17 @@
isearch-message isearch-new-message
isearch-forward isearch-new-forward
isearch-word isearch-new-word
- isearch-case-fold-search isearch-new-case-fold))
+ isearch-case-fold-search isearch-new-case-fold)
+
+ ;; Restore the minibuffer message before moving point.
+ (isearch-call-message nil t)
+
+ ;; Set point at the start (end) of old match if forward (backward),
+ ;; so after exiting minibuffer isearch resumes at the start (end)
+ ;; of this match and can find it again.
+ (if (and old-other-end (eq old-point (point))
+ (eq isearch-forward isearch-new-forward))
+ (goto-char old-other-end)))
;; Empty isearch-string means use default.
(when (= 0 (length isearch-string))
@@ -1417,16 +1443,20 @@
(isearch-ring-adjust1 nil))
;; If already have what to search for, repeat it.
(or isearch-success
- (progn
- ;; Set isearch-wrapped before calling isearch-wrap-function
- (setq isearch-wrapped t)
- (if isearch-wrap-function
- (funcall isearch-wrap-function)
- (goto-char (if isearch-forward (point-min) (point-max)))))))
+ ;; Set isearch-wrapped before calling isearch-wrap-function
+ (setq isearch-wrapped t)))
;; C-s in reverse or C-r in forward, change direction.
(setq isearch-forward (not isearch-forward)
isearch-success t))
+ (isearch-call-message nil t) ; Call before moving point.
+ (if (and (eq isearch-forward (eq direction 'forward))
+ (not (equal isearch-string ""))
+ (not isearch-success))
+ (if isearch-wrap-function
+ (funcall isearch-wrap-function)
+ (goto-char (if isearch-forward (point-min) (point-max)))))
+
(setq isearch-barrier (point)) ; For subsequent \| if regexp.
(if (equal isearch-string "")
@@ -1867,6 +1897,7 @@
(length isearch-string))))
isearch-message (mapconcat 'isearch-text-char-description
isearch-string "")))
+ (isearch-call-message nil t) ; Do this before moving point.
;; Use the isearch-other-end as new starting point to be able
;; to find the remaining part of the search string again.
;; This is like what `isearch-search-and-update' does,
@@ -2041,6 +2072,7 @@
(setq isearch-case-fold-search
(isearch-no-upper-case-p isearch-string isearch-regexp))))
;; Not regexp, not reverse, or no match at point.
+ (isearch-call-message nil t) ; Do this before moving point.
(if (and isearch-other-end (not isearch-adjusted))
(goto-char (if isearch-forward isearch-other-end
(min isearch-opoint
@@ -2207,10 +2239,12 @@
together with as much of the search string as will fit; the symbol
`above' if we need to scroll the text downwards; the symbol `below',
if upwards."
- (let ((w-start (window-start))
- (w-end (window-end nil t))
- (w-L1 (save-excursion (move-to-window-line 1) (point)))
- (w-L-1 (save-excursion (move-to-window-line -1) (point)))
+ (let ((w-start (isearch-windows-start))
+ (w-end (isearch-windows-end nil t))
+ (w-L1 (with-selected-window (car isearch-windows)
+ (save-excursion (move-to-window-line 1) (point))))
+ (w-L-1 (with-selected-window (car (last isearch-windows))
+ (save-excursion (move-to-window-line -1) (point))))
start end) ; start and end of search string in buffer
(if isearch-forward
(setq end isearch-point start (or isearch-other-end isearch-point))
@@ -2236,14 +2270,18 @@
(setq start isearch-point end (or isearch-other-end isearch-point)))
(if above
(progn
- (goto-char start)
+ (select-window (car isearch-windows))
+ (goto-char start)
(recenter 0)
- (when (>= isearch-point (window-end nil t))
- (goto-char isearch-point)
+ (when (>= isearch-point (isearch-windows-end nil t))
+ (select-window (car (last isearch-windows)))
+ (goto-char isearch-point)
(recenter -1)))
+ (select-window (car (last isearch-windows)))
(goto-char end)
(recenter -1)
- (when (< isearch-point (window-start))
+ (when (< isearch-point (isearch-windows-start))
+ (select-window (car isearch-windows))
(goto-char isearch-point)
(recenter 0))))
(goto-char isearch-point))
@@ -2390,6 +2428,7 @@
(isearch-ring-adjust1 advance)
(if search-ring-update
(progn
+ (isearch-call-message nil t)
(isearch-search)
(isearch-push-state)
(isearch-update))
@@ -2462,6 +2501,13 @@
(defun isearch-message (&optional c-q-hack ellipsis)
;; Generate and print the message string.
+
+ ;; N.B.: This function should always be called with point at the
+ ;; search point, because in certain (rare) circumstances, undesired
+ ;; scrolling can happen when point is elsewhere. These
+ ;; circumstances are when follow-mode is active, the search string
+ ;; spans two (or several) windows, and the message about to be
+ ;; displayed will cause the echo area to expand.
(let ((cursor-in-echo-area ellipsis)
(m isearch-message)
(fail-pos (isearch-fail-pos t)))
@@ -2630,9 +2676,6 @@
(defun isearch-search ()
;; Do the search with the current search string.
- (if isearch-message-function
- (funcall isearch-message-function nil t)
- (isearch-message nil t))
(if (and (eq isearch-case-fold-search t) search-upper-case)
(setq isearch-case-fold-search
(isearch-no-upper-case-p isearch-string isearch-regexp)))
@@ -2936,8 +2979,9 @@
(defvar isearch-lazy-highlight-timer nil)
(defvar isearch-lazy-highlight-last-string nil)
(defvar isearch-lazy-highlight-window nil)
-(defvar isearch-lazy-highlight-window-start nil)
-(defvar isearch-lazy-highlight-window-end nil)
+(defvar isearch-lazy-highlight-windows nil)
+(defvar isearch-lazy-highlight-windows-start 0)
+(defvar isearch-lazy-highlight-windows-end 0)
(defvar isearch-lazy-highlight-case-fold-search nil)
(defvar isearch-lazy-highlight-regexp nil)
(defvar isearch-lazy-highlight-lax-whitespace nil)
@@ -2972,11 +3016,15 @@
search string to change or the window to scroll). It is also used
by other Emacs features."
(when (and (null executing-kbd-macro)
- (sit-for 0) ;make sure (window-start) is credible
+ ;; make sure (window-start) is credible
+ (if (and (boundp 'follow-mode) follow-mode)
+ (progn (follow-adjust-window (selected-window))
+ t)
+ (sit-for 0))
(or (not (equal isearch-string
isearch-lazy-highlight-last-string))
- (not (eq (selected-window)
- isearch-lazy-highlight-window))
+ (not (memq (selected-window)
+ isearch-lazy-highlight-windows))
(not (eq isearch-lazy-highlight-case-fold-search
isearch-case-fold-search))
(not (eq isearch-lazy-highlight-regexp
@@ -2987,10 +3035,10 @@
isearch-lax-whitespace))
(not (eq isearch-lazy-highlight-regexp-lax-whitespace
isearch-regexp-lax-whitespace))
- (not (= (window-start)
- isearch-lazy-highlight-window-start))
- (not (= (window-end) ; Window may have been split/joined.
- isearch-lazy-highlight-window-end))
+ (not (= (isearch-windows-start isearch-lazy-highlight-windows)
+ isearch-lazy-highlight-windows-start))
+ (not (= (isearch-windows-end isearch-lazy-highlight-windows) ; Window may have been split/joined.
+ isearch-lazy-highlight-windows-end))
(not (eq isearch-forward
isearch-lazy-highlight-forward))
;; In case we are recovering from an error.
@@ -3005,8 +3053,14 @@
(setq isearch-lazy-highlight-start-limit beg
isearch-lazy-highlight-end-limit end)
(setq isearch-lazy-highlight-window (selected-window)
- isearch-lazy-highlight-window-start (window-start)
- isearch-lazy-highlight-window-end (window-end)
+ isearch-lazy-highlight-windows
+ (if (and (boundp 'follow-mode) follow-mode)
+ (follow-all-followers)
+ (list (selected-window)))
+ isearch-lazy-highlight-windows-start
+ (isearch-windows-start isearch-lazy-highlight-windows)
+ isearch-lazy-highlight-windows-end
+ (isearch-windows-end isearch-lazy-highlight-windows)
;; Start lazy-highlighting at the beginning of the found
;; match (`isearch-other-end'). If no match, use point.
;; One of the next two variables (depending on search direction)
@@ -3048,11 +3102,11 @@
(min (or isearch-lazy-highlight-end-limit (point-max))
(if isearch-lazy-highlight-wrapped
isearch-lazy-highlight-start
- (window-end)))
+ isearch-lazy-highlight-windows-end))
(max (or isearch-lazy-highlight-start-limit (point-min))
(if isearch-lazy-highlight-wrapped
isearch-lazy-highlight-end
- (window-start))))))
+ isearch-lazy-highlight-windows-start)))))
;; Use a loop like in `isearch-search'.
(while retry
(setq success (isearch-search-string
@@ -3068,6 +3122,24 @@
success)
(error nil)))
+(defun isearch-lazy-highlight-put-overlays (mb me)
+ "Put a highlighting overlay on the buffer for each pertinent window.
+
+An overlay is put over the positions (MB ME) in each window in
+`isearch-lazy-highlight-windows' which (at least partially) contains them."
+ ;; non-zero-length match
+ (mapc (lambda (w)
+ (if (and (< mb (window-end w))
+ (> me (window-start w)))
+ (let ((ov (make-overlay mb me)))
+ (push ov isearch-lazy-highlight-overlays)
+ ;; 1000 is higher than ediff's 100+,
+ ;; but lower than isearch main overlay's 1001
+ (overlay-put ov 'priority 1000)
+ (overlay-put ov 'face lazy-highlight-face)
+ (overlay-put ov 'window w))))
+ isearch-lazy-highlight-windows))
+
(defun isearch-lazy-highlight-update ()
"Update highlighting of other matches for current search."
(let ((max lazy-highlight-max-at-a-time)
@@ -3096,23 +3168,15 @@
(if isearch-lazy-highlight-forward
(if (= mb (if isearch-lazy-highlight-wrapped
isearch-lazy-highlight-start
- (window-end)))
+ isearch-lazy-highlight-windows-end))
(setq found nil)
(forward-char 1))
(if (= mb (if isearch-lazy-highlight-wrapped
isearch-lazy-highlight-end
- (window-start)))
+ isearch-lazy-highlight-windows-start))
(setq found nil)
(forward-char -1)))
-
- ;; non-zero-length match
- (let ((ov (make-overlay mb me)))
- (push ov isearch-lazy-highlight-overlays)
- ;; 1000 is higher than ediff's 100+,
- ;; but lower than isearch main overlay's 1001
- (overlay-put ov 'priority 1000)
- (overlay-put ov 'face lazy-highlight-face)
- (overlay-put ov 'window (selected-window))))
+ (isearch-lazy-highlight-put-overlays mb me))
;; Remember the current position of point for
;; the next call of `isearch-lazy-highlight-update'
;; when `lazy-highlight-max-at-a-time' is too small.
@@ -3128,12 +3192,12 @@
(setq isearch-lazy-highlight-wrapped t)
(if isearch-lazy-highlight-forward
(progn
- (setq isearch-lazy-highlight-end (window-start))
+ (setq isearch-lazy-highlight-end isearch-lazy-highlight-windows-start)
(goto-char (max (or isearch-lazy-highlight-start-limit (point-min))
- (window-start))))
- (setq isearch-lazy-highlight-start (window-end))
+ isearch-lazy-highlight-windows-start)))
+ (setq isearch-lazy-highlight-start isearch-lazy-highlight-windows-end)
(goto-char (min (or isearch-lazy-highlight-end-limit (point-max))
- (window-end))))))))
+ isearch-lazy-highlight-windows-end)))))))
(unless nomore
(setq isearch-lazy-highlight-timer
(run-at-time lazy-highlight-interval nil
@@ -3151,6 +3215,7 @@
(setq isearch-string string
isearch-message message
isearch-case-fold-search case-fold)
+ (isearch-call-message nil t)
(isearch-search)
(isearch-update))
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-09 22:44 bug#17453: Isearch doesn't work properly with Follow Mode Alan Mackenzie
@ 2014-05-10 2:40 ` Stefan Monnier
2014-05-11 12:58 ` Alan Mackenzie
` (2 more replies)
[not found] ` <handler.17453.B.139967578531952.ack@debbugs.gnu.org>
1 sibling, 3 replies; 96+ messages in thread
From: Stefan Monnier @ 2014-05-10 2:40 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453
> +(defvar isearch-windows nil
> + "List of windows active in the incremental search.
> +This is either the ordered list of active windows administered by
> +`follow-mode' or a list of the single window involved in the
> +search.")
> +
> +(defmacro isearch-windows-start (&optional wins)
> + "Get the start point of the windows involved in the isearch."
> + `(window-start (car ,(if wins wins 'isearch-windows))))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(or wins 'isearch-windows)
I must say I really dislike this hard-coding of follow-mode support in
isearch.el. Isearch should not know so much about follow-mode and
follow-mode should not know so much about Isearch either.
IOW we should try harder to come up with more general hooks.
> + `(if isearch-message-function
> + (funcall isearch-message-function ,cqh ,ellip)
> + (isearch-message ,cqh ,ellip)))
Aka (funcall (or isearch-message-function #'isearch-message) ,cqh ,ellip)
BTW, isearch-message-function should be changed to default to
isearch-message rather than to nil.
> @@ -2207,10 +2239,12 @@
> together with as much of the search string as will fit; the symbol
> `above' if we need to scroll the text downwards; the symbol `below',
> if upwards."
> - (let ((w-start (window-start))
> - (w-end (window-end nil t))
> - (w-L1 (save-excursion (move-to-window-line 1) (point)))
> - (w-L-1 (save-excursion (move-to-window-line -1) (point)))
> + (let ((w-start (isearch-windows-start))
> + (w-end (isearch-windows-end nil t))
> + (w-L1 (with-selected-window (car isearch-windows)
> + (save-excursion (move-to-window-line 1) (point))))
> + (w-L-1 (with-selected-window (car (last isearch-windows))
> + (save-excursion (move-to-window-line -1) (point))))
This isearch-string-out-of-window+isearch-back-into-window business, for
example should be generalized to a function along the lines of
"bring-region-into-sight". It's not useful only for isearch.
E.g. ediff-next-hunk would also benefit from it.
And of course, it should come with a bring-region-into-sight-function
hook (which does not default to nil but to the default implementation),
which follow-mode can then override.
> + ;; 1000 is higher than ediff's 100+,
[ Side note: Ediff doesn't use priorities any more. ]
Stefan
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-10 2:40 ` Stefan Monnier
@ 2014-05-11 12:58 ` Alan Mackenzie
2014-05-11 16:09 ` Stefan Monnier
2015-10-29 23:23 ` Alan Mackenzie
2015-10-29 23:23 ` Alan Mackenzie
2 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2014-05-11 12:58 UTC (permalink / raw
To: Stefan Monnier; +Cc: 17453
Hello, Stefan.
On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
> I must say I really dislike this hard-coding of follow-mode support in
> isearch.el. Isearch should not know so much about follow-mode and
> follow-mode should not know so much about Isearch either.
Follow Mode knows nothing of Isearch. The problem the other way round is
in Lisp files that themselves play with redisplay (including calling
`sit-for') and `set-window-start', and so on. Between these invocations
and the actual redisplay, Follow Mode must get a chance to make its
adjustments.
> IOW we should try harder to come up with more general hooks.
For what?
Given that Follow Mode uses a list of windows where most elisp programs
(including Isearch up till now) are expecting to deal with a single
window, there is quite a large clash, possibly unbridgeable with any
reasonable amount of effort. Some sort of abstraction that smoothes over
the difference between the window and the window list is difficult to
conceive of - Follow Mode itself is such an abstraction.
Maybe a useful hook here would be `redisplay-hook' where Follow Mode
could do its stuff more effectively than on `post-command-hook'.
The extreme solution would be to enhance the display code to handle
multiple column windows, rendering Follow Mode redundant. I don't think
anybody with the expertise has the time for this.
> > @@ -2207,10 +2239,12 @@
> > together with as much of the search string as will fit; the symbol
> > `above' if we need to scroll the text downwards; the symbol `below',
> > if upwards."
> > - (let ((w-start (window-start))
> > - (w-end (window-end nil t))
> > - (w-L1 (save-excursion (move-to-window-line 1) (point)))
> > - (w-L-1 (save-excursion (move-to-window-line -1) (point)))
> > + (let ((w-start (isearch-windows-start))
> > + (w-end (isearch-windows-end nil t))
> > + (w-L1 (with-selected-window (car isearch-windows)
> > + (save-excursion (move-to-window-line 1) (point))))
> > + (w-L-1 (with-selected-window (car (last isearch-windows))
> > + (save-excursion (move-to-window-line -1) (point))))
> This isearch-string-out-of-window+isearch-back-into-window business, for
> example should be generalized to a function along the lines of
> "bring-region-into-sight". It's not useful only for isearch.
This seems to be a different bug to the one I reported, and orthogonal to
it.
> E.g. ediff-next-hunk would also benefit from it.
Not necessarily. isearch-back-into-window, in addition to doing
"bring-region-into-sight" ensures that it's the isearch-point end of it
which is visible when it is too big for the window. This may be
problematic for other uses, like in ediff.
> And of course, it should come with a bring-region-into-sight-function
> hook (which does not default to nil but to the default implementation),
> which follow-mode can then override.
Yes.
Question is, what about the main bug? The patch I wrote fixes a real
bug, and works (modulo any remaining bugs). Do you have any concrete
suggestions as to how to improve the fix?
> Stefan
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-11 12:58 ` Alan Mackenzie
@ 2014-05-11 16:09 ` Stefan Monnier
2014-05-11 18:18 ` Alan Mackenzie
0 siblings, 1 reply; 96+ messages in thread
From: Stefan Monnier @ 2014-05-11 16:09 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453
> Follow Mode knows nothing of Isearch.
follow-mode.el doesn't, indeed, but since you're moving some follow mode
code to isearch.el, that means that follow mode (which is now spread
over follow-mode.el and isearch.el) knows something about Isearch.
> The problem the other way round is in Lisp files that themselves play
> with redisplay (including calling `sit-for') and `set-window-start',
> and so on. Between these invocations and the actual redisplay, Follow
> Mode must get a chance to make its adjustments.
There's no doubt that follow mode's task is a tricky one, and I'm
willing to add special support for it. I just want this special support
to be a bit more generic than what you provided.
It shouldn't be too difficult. It's just a matter of refactoring:
change your patch so that on isearch.el's side it only adds some hooks,
which are then set follow-mode.el.
>> IOW we should try harder to come up with more general hooks.
> For what?
So that the same hooks can be used by other code than Isearch, for example.
> Maybe a useful hook here would be `redisplay-hook' where Follow Mode
> could do its stuff more effectively than on `post-command-hook'.
There's pre-redisplay-function, which I think it does get us closer but
is not sufficient yet. And of course, it won't help with things
like "bring-region-into-sight".
>> > @@ -2207,10 +2239,12 @@
>> > together with as much of the search string as will fit; the symbol
>> > `above' if we need to scroll the text downwards; the symbol `below',
>> > if upwards."
>> > - (let ((w-start (window-start))
>> > - (w-end (window-end nil t))
>> > - (w-L1 (save-excursion (move-to-window-line 1) (point)))
>> > - (w-L-1 (save-excursion (move-to-window-line -1) (point)))
>> > + (let ((w-start (isearch-windows-start))
>> > + (w-end (isearch-windows-end nil t))
>> > + (w-L1 (with-selected-window (car isearch-windows)
>> > + (save-excursion (move-to-window-line 1) (point))))
>> > + (w-L-1 (with-selected-window (car (last isearch-windows))
>> > + (save-excursion (move-to-window-line -1) (point))))
>> This isearch-string-out-of-window+isearch-back-into-window business, for
>> example should be generalized to a function along the lines of
>> "bring-region-into-sight". It's not useful only for isearch.
> This seems to be a different bug to the one I reported, and orthogonal
> to it.
What bug? I'm just pointing out that the code you're modifying is more
generally useful and that this generality is a good guide to help us
decide where to put needed hooks.
>> E.g. ediff-next-hunk would also benefit from it.
^^^^^
I meant "diff", sorry. Tho it would also be useful to ediff and smerge-mode
as well, indeed.
> Not necessarily. isearch-back-into-window, in addition to doing
> "bring-region-into-sight" ensures that it's the isearch-point end of it
> which is visible when it is too big for the window. This may be
> problematic for other uses, like in ediff.
I doubt this will be problematic. It seems quite natural for
"bring-region-into-sight" to take as arguments not just the region but
also some indication of the part to favor in case it can't all
be displayed.
> Question is, what about the main bug? The patch I wrote fixes a real
> bug, and works (modulo any remaining bugs). Do you have any concrete
> suggestions as to how to improve the fix?
Hmm... I'm sorry I got lost along the way. I thought the whole patch is
the bug fix (and the bring-region-into-sight part does seem like
a bug-fix as well, tho maybe of top priority, admittedly).
Could you show which part of the patch addresses the main bug?
Stefan
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-11 16:09 ` Stefan Monnier
@ 2014-05-11 18:18 ` Alan Mackenzie
2014-05-11 19:05 ` Stefan Monnier
0 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2014-05-11 18:18 UTC (permalink / raw
To: Stefan Monnier; +Cc: 17453
Hello, Stefan.
On Sun, May 11, 2014 at 12:09:38PM -0400, Stefan Monnier wrote:
> > Follow Mode knows nothing of Isearch.
> follow-mode.el doesn't, indeed, but since you're moving some follow mode
> code to isearch.el, ....
Am I? No code that currently is in follow-mode.el will cease to be in
follow-mode.el.
> .... that means that follow mode (which is now spread
> over follow-mode.el and isearch.el) knows something about Isearch.
No, Follow Mode will know nothing about Isearch. Isearch will know about
follow-mode's internal structures, namely its list of windows, and what
their sequencing means. In that sense, the two libraries are coupled,
which isn't, other things being equal, good, but I can't see how to
avoid it. Can you see a way of avoiding this coupling?
> > The problem the other way round is in Lisp files that themselves play
> > with redisplay (including calling `sit-for') and `set-window-start',
> > and so on. Between these invocations and the actual redisplay, Follow
> > Mode must get a chance to make its adjustments.
> There's no doubt that follow mode's task is a tricky one, and I'm
> willing to add special support for it. I just want this special support
> to be a bit more generic than what you provided.
I'm having trouble discerning WHAT, specifically, should be made more
generic (with the exception of the two functions discussed below). Could
you talk more in specifics, please.
> It shouldn't be too difficult. It's just a matter of refactoring:
> change your patch so that on isearch.el's side it only adds some hooks,
> which are then set follow-mode.el.
Do you mean "set to a function in follow-mode.el". I think you're
suggesting writing more functions in follow-mode.el. Do you mean
something like a Follow Mode equivalent of `window-start'?
> >> IOW we should try harder to come up with more general hooks.
> > For what?
> So that the same hooks can be used by other code than Isearch, for example.
I know the motivation for the change. For what functionalities should
code be come up with that will be incorporated into these hooks? What
will the hooks do, specifically?
> > Maybe a useful hook here would be `redisplay-hook' where Follow Mode
> > could do its stuff more effectively than on `post-command-hook'.
> There's pre-redisplay-function, which I think it does get us closer but
> is not sufficient yet. And of course, it won't help with things
> like "bring-region-into-sight".
I didn't know about pre-redisplay-function. It looks new. As an aside,
its documentation is unclear (what is a "set" of windows, for example,
and what sort of things are allowed/not allowed in a hook function
(deleting windows, for example)?)
Is it for "bring-region-into-sight" and friends that you envisage adding
hooks?
> >> > @@ -2207,10 +2239,12 @@
> >> > together with as much of the search string as will fit; the symbol
> >> > `above' if we need to scroll the text downwards; the symbol `below',
> >> > if upwards."
> >> > - (let ((w-start (window-start))
> >> > - (w-end (window-end nil t))
> >> > - (w-L1 (save-excursion (move-to-window-line 1) (point)))
> >> > - (w-L-1 (save-excursion (move-to-window-line -1) (point)))
> >> > + (let ((w-start (isearch-windows-start))
> >> > + (w-end (isearch-windows-end nil t))
> >> > + (w-L1 (with-selected-window (car isearch-windows)
> >> > + (save-excursion (move-to-window-line 1) (point))))
> >> > + (w-L-1 (with-selected-window (car (last isearch-windows))
> >> > + (save-excursion (move-to-window-line -1) (point))))
> >> This isearch-string-out-of-window+isearch-back-into-window business, for
> >> example should be generalized to a function along the lines of
> >> "bring-region-into-sight". It's not useful only for isearch.
> > This seems to be a different bug to the one I reported, and orthogonal
> > to it.
> What bug? I'm just pointing out that the code you're modifying is more
> generally useful and that this generality is a good guide to help us
> decide where to put needed hooks.
I meant that forming a more general subroutine out of
isearch-back-into-window is not properly part of bug #17453. It is a
separate exercise, which can be done independently of #17453.
I'm not sure how well a hook would work for this functionality, since in
the "plain" hook function you'd want to pass it a window, whereas in the
"follow" hook function you'd want to pass it a list of windows.
> >> E.g. ediff-next-hunk would also benefit from it.
> ^^^^^
> I meant "diff", sorry. Tho it would also be useful to ediff and smerge-mode
> as well, indeed.
OK. So you're thinking of somewhere like subr.el, with something like
(defun scroll-region-into-window/s (start end window/s above
opposite-important) ....)
where WINDOW/S is either a single window or a list of them, START END
bound the region, OPPOSITE-IMPORTANT is t when the region boundary
"nearest the window" is to be preferred for display when the region's too
big. Or something like that. Maybe we could add a parameter for desired
margin at the top/bottom of the window.
> > Not necessarily. isearch-back-into-window, in addition to doing
> > "bring-region-into-sight" ensures that it's the isearch-point end of it
> > which is visible when it is too big for the window. This may be
> > problematic for other uses, like in ediff.
> I doubt this will be problematic. It seems quite natural for
> "bring-region-into-sight" to take as arguments not just the region but
> also some indication of the part to favor in case it can't all
> be displayed.
OK. Formulating a good description of that parameter is tricky, though.
> > Question is, what about the main bug? The patch I wrote fixes a real
> > bug, and works (modulo any remaining bugs). Do you have any concrete
> > suggestions as to how to improve the fix?
> Hmm... I'm sorry I got lost along the way. I thought the whole patch is
> the bug fix (and the bring-region-into-sight part does seem like
> a bug-fix as well, tho maybe of top priority, admittedly).
> Could you show which part of the patch addresses the main bug?
The whole patch addresses the main bug; forming generally useful
subroutines out of isearch-string-out-of-window and friends is the other
bug, which I'm suggesting be dealt with separately.
> Stefan
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-11 18:18 ` Alan Mackenzie
@ 2014-05-11 19:05 ` Stefan Monnier
2014-05-11 20:40 ` Alan Mackenzie
0 siblings, 1 reply; 96+ messages in thread
From: Stefan Monnier @ 2014-05-11 19:05 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453
>> follow-mode.el doesn't, indeed, but since you're moving some follow mode
>> code to isearch.el, ....
> Am I? No code that currently is in follow-mode.el will cease to be in
> follow-mode.el.
Please replace "moving" with "adding" in my phrase, then.
> No, Follow Mode will know nothing about Isearch. Isearch will know about
> follow-mode's internal structures, namely its list of windows, and what
> their sequencing means. In that sense, the two libraries are coupled,
> which isn't, other things being equal, good, but I can't see how to
> avoid it. Can you see a way of avoiding this coupling?
I gave one suggestion to uncouple them in one specific spot, yes.
There's no general answer for such problems, so we have to look at
every part of the coupling and try to find solutions for each part.
>> It shouldn't be too difficult. It's just a matter of refactoring:
>> change your patch so that on isearch.el's side it only adds some hooks,
>> which are then set follow-mode.el.
> Do you mean "set to a function in follow-mode.el". I think you're
> suggesting writing more functions in follow-mode.el. Do you mean
> something like a Follow Mode equivalent of `window-start'?
Right, I think introducing new functions/hooks to get the beginning/end of the
visible part of the buffer, which can then be overridden by follow-mode
would probably be part of the solution.
> I know the motivation for the change. For what functionalities should
> code be come up with that will be incorporated into these hooks? What
> will the hooks do, specifically?
I don't know. You know the code better than I do, so hopefully you can
come up with good ideas.
> I didn't know about pre-redisplay-function. It looks new. As an aside,
> its documentation is unclear (what is a "set" of windows, for example,
As is common in (Emacs) Lisp, sets are represented as a list.
> and what sort of things are allowed/not allowed in a hook function
> (deleting windows, for example)?)
Noone knows, really.
> Is it for "bring-region-into-sight" and friends that you envisage adding
> hooks?
Yes.
> I'm not sure how well a hook would work for this functionality, since in
> the "plain" hook function you'd want to pass it a window, whereas in the
> "follow" hook function you'd want to pass it a list of windows.
I don't see why you'd need to pass anything like a window or a list
of windows. All it needs is a region and a point. The window (or set
thereof) would be passed implicitly via selected-window, as usual.
> OK. So you're thinking of somewhere like subr.el, with something like
> (defun scroll-region-into-window/s (start end window/s above
> opposite-important) ....)
> where WINDOW/S is either a single window or a list of them, START END
> bound the region, OPPOSITE-IMPORTANT is t when the region boundary
> "nearest the window" is to be preferred for display when the region's too
> big. Or something like that. Maybe we could add a parameter for desired
> margin at the top/bottom of the window.
Something like that, yes.
> OK. Formulating a good description of that parameter is tricky, though.
Right, designing an API is often tricky.
> The whole patch addresses the main bug; forming generally useful
> subroutines out of isearch-string-out-of-window and friends is the other
> bug, which I'm suggesting be dealt with separately.
I don't want to add code specific to follow-mode directly in Isearch.
So the way to fix the bug is in 2 steps:
- refactor Isearch so that it adds the hooks we need (some of this,
may involve creating new functions in subr.el such as discussed above).
- change follow-mode.el to set those hooks as appropriate.
There's then a 3rd step which is to make other packages than Isearch use
those same new functions/hooks, but that one can be postponed.
Stefan
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-11 19:05 ` Stefan Monnier
@ 2014-05-11 20:40 ` Alan Mackenzie
2014-05-11 21:46 ` Stefan Monnier
0 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2014-05-11 20:40 UTC (permalink / raw
To: Stefan Monnier; +Cc: 17453
Hello, Stefan.
On Sun, May 11, 2014 at 03:05:17PM -0400, Stefan Monnier wrote:
> >> It shouldn't be too difficult. It's just a matter of refactoring:
> >> change your patch so that on isearch.el's side it only adds some hooks,
> >> which are then set follow-mode.el.
> > Do you mean "set to a function in follow-mode.el". I think you're
> > suggesting writing more functions in follow-mode.el. Do you mean
> > something like a Follow Mode equivalent of `window-start'?
> Right, I think introducing new functions/hooks to get the beginning/end of the
> visible part of the buffer, which can then be overridden by follow-mode
> would probably be part of the solution.
OK, but it really needs a window list as a parameter, and that needs to
be stored in a variable somewhere.
> > I know the motivation for the change. For what functionalities should
> > code be come up with that will be incorporated into these hooks? What
> > will the hooks do, specifically?
> I don't know. You know the code better than I do, so hopefully you can
> come up with good ideas.
I don't really see opportunities for this whose costs don't outweigh
them. We can make a variable called `window-start-function', but then
everybody has to start using
(funcall window-start-function w)
which is a little more obfuscated and confusing than
(window-start w)
just to save a (very) few calls of
(window-start (car foo-windows))
. The last form is typically going to be much faster when Follow Mode is
active (see below).
[ Snipped bit about pre-redisplay-function. Thanks. ]
> > Is it for "bring-region-into-sight" and friends that you envisage adding
> > hooks?
> Yes.
> > I'm not sure how well a hook would work for this functionality, since in
> > the "plain" hook function you'd want to pass it a window, whereas in the
> > "follow" hook function you'd want to pass it a list of windows.
> I don't see why you'd need to pass anything like a window or a list
> of windows. All it needs is a region and a point. The window (or set
> thereof) would be passed implicitly via selected-window, as usual.
Because each time the Follow Mode window list would have to be
recalculated, and this is a moderately expensive calculation, involving
filtering and sorting the windows in a frame (`follow-all-followers').
This is no big deal once per command, but if done much more often will
start to drag.
[ .... ]
> Right, designing an API is often tricky.
:-)
> > The whole patch addresses the main bug; forming generally useful
> > subroutines out of isearch-string-out-of-window and friends is the other
> > bug, which I'm suggesting be dealt with separately.
> I don't want to add code specific to follow-mode directly in Isearch.
There is one place where I think it is unavoidable, and that is adding
lazy highlight overlays. These are currently given a `window' property
to restrict their effect to the current window. In my patch, a
particular match gets _two_ (or even several) overlays when it spans two
or more Follow Mode windows. This can surely not be abstracted away in
any sensible way.
> So the way to fix the bug is in 2 steps:
> - refactor Isearch so that it adds the hooks we need (some of this,
> may involve creating new functions in subr.el such as discussed above).
> - change follow-mode.el to set those hooks as appropriate.
> There's then a 3rd step which is to make other packages than Isearch use
> those same new functions/hooks, but that one can be postponed.
> Stefan
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-11 20:40 ` Alan Mackenzie
@ 2014-05-11 21:46 ` Stefan Monnier
0 siblings, 0 replies; 96+ messages in thread
From: Stefan Monnier @ 2014-05-11 21:46 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453
>> Right, I think introducing new functions/hooks to get the
>> beginning/end of the visible part of the buffer, which can then be
>> overridden by follow-mode would probably be part of the solution.
> OK, but it really needs a window list as a parameter, and that needs to
> be stored in a variable somewhere.
I don't understand. Why would it need to be both in a parameter and in
a variable somewhere? And isn't it already in some follow-mode
"variable" somewhere?
window-start does not need a window parameter (it does accept such
a parameter, optionally, but that's just to avoid some
with-selected-window), so I don't think such new function would need
such a parameter either.
> I don't really see opportunities for this whose costs don't outweigh
> them. We can make a variable called `window-start-function', but then
> everybody has to start using
> (funcall window-start-function w)
I see two options:
- either some/many calls need to be changed to use the new API.
- or follow-mode advises window-start.
And of course, it would be (funcall window-start-function), without the `w'.
> which is a little more obfuscated and confusing than
> (window-start w)
To make it less obfuscated, we can provide a new function
`viewable-area-start' which does (funcall window-start-function).
> . The last form is typically going to be much faster when Follow Mode is
> active (see below).
Not sure about "much". My gut feeling would be rather around "not
noticeably".
>> I don't see why you'd need to pass anything like a window or a list
>> of windows. All it needs is a region and a point. The window (or set
>> thereof) would be passed implicitly via selected-window, as usual.
> Because each time the Follow Mode window list
I don't see why. Isn't/can't the "Follow Mode window list" be kept as
a window-parameter, so you can get it quickly?
> would have to be recalculated, and this is a moderately expensive
> calculation, involving filtering and sorting the windows in a frame
> (`follow-all-followers'). This is no big deal once per command, but
> if done much more often will start to drag.
Indeed, recomputing it for every window-start call would make it
expensive. I assume that it can be cached such that we only recompute
it when it changes (e.g. using window-configuration-change-hook).
> There is one place where I think it is unavoidable, and that is adding
> lazy highlight overlays. These are currently given a `window' property
> to restrict their effect to the current window. In my patch, a
> particular match gets _two_ (or even several) overlays when it spans two
> or more Follow Mode windows. This can surely not be abstracted away in
> any sensible way.
You might be right. As mentioned, I think follow-mode is important
enough to warrant special treatment, so we can probably leave a few
hacks in there, in case no useful/sensible abstraction can be invented.
In this case of isearch-vs-followmode, I think the way to find the
useful abstractions is to try and figure out "what would be
useful/useable for other packages than isearch".
Window-specific overlays are not used very often, the candidates seem to be:
- region highlighting.
- rectangular region highlighting.
- compare-w.el.
Stefan
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-10 2:40 ` Stefan Monnier
2014-05-11 12:58 ` Alan Mackenzie
@ 2015-10-29 23:23 ` Alan Mackenzie
2015-10-29 23:23 ` Alan Mackenzie
2 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-29 23:23 UTC (permalink / raw
To: Stefan Monnier; +Cc: 17453, emacs-devel
Hello, Stefan, hello, Emacs.
Resuming a conversation from a year and a half ago, in it I had proposed
a patch which made isearch work properly in a Follow Mode set of
windows. In the end, you rejected my patch (in effect), because...
On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
> I must say I really dislike this hard-coding of follow-mode support in
> isearch.el. Isearch should not know so much about follow-mode and
> follow-mode should not know so much about Isearch either.
> IOW we should try harder to come up with more general hooks.
That irritated me at the time, but you were right - putting Follow Mode
support directly into isearch would not have been a good idea.
I've recently started working on the bug again, and although it's not
your job anymore, I'd still appreciate you giving me some feedback on my
new approach.
What I propose is that several functions which operate on windows have
extended versions which operate on @dfn{groups of windows}, this meaning
(for the moment) Follow Mode groups of windows. These new functions are
flexible enough to work with methods other than Follow Mode of grouping
windows, should such ever be implemented.
The new functions are window*-start, window*-end, set-window*-start,
recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
window*-start returns the window-start of the _first_ window in the
group, window*-end returns the window-end of the _last_ window in the
group, and so on, all these functions doing the Right Thing for a group
of windows. sit*-for exists, because Follow Mode needs a chance to
resynchronise its windows before redisplay happens.
If Follow Mode is not enabled, all the above functions do the Right
Thing on the single window.
In addition, there is a function window*-windows, which returns an
ordered list of the windows involved (or a list of the single window
when F.M. is not active).
I have a trial implementation of the above, and a trial adaptation of
isearch.el which uses it. It works.
What do you think?
> Stefan
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2014-05-10 2:40 ` Stefan Monnier
2014-05-11 12:58 ` Alan Mackenzie
2015-10-29 23:23 ` Alan Mackenzie
@ 2015-10-29 23:23 ` Alan Mackenzie
2015-10-31 22:35 ` John Wiegley
` (5 more replies)
2 siblings, 6 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-29 23:23 UTC (permalink / raw
To: Stefan Monnier; +Cc: 17453, emacs-devel
Hello, Stefan, hello, Emacs.
Resuming a conversation from a year and a half ago, in it I had proposed
a patch which made isearch work properly in a Follow Mode set of
windows. In the end, you rejected my patch (in effect), because...
On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
> I must say I really dislike this hard-coding of follow-mode support in
> isearch.el. Isearch should not know so much about follow-mode and
> follow-mode should not know so much about Isearch either.
> IOW we should try harder to come up with more general hooks.
That irritated me at the time, but you were right - putting Follow Mode
support directly into isearch would not have been a good idea.
I've recently started working on the bug again, and although it's not
your job anymore, I'd still appreciate you giving me some feedback on my
new approach.
What I propose is that several functions which operate on windows have
extended versions which operate on @dfn{groups of windows}, this meaning
(for the moment) Follow Mode groups of windows. These new functions are
flexible enough to work with methods other than Follow Mode of grouping
windows, should such ever be implemented.
The new functions are window*-start, window*-end, set-window*-start,
recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
window*-start returns the window-start of the _first_ window in the
group, window*-end returns the window-end of the _last_ window in the
group, and so on, all these functions doing the Right Thing for a group
of windows. sit*-for exists, because Follow Mode needs a chance to
resynchronise its windows before redisplay happens.
If Follow Mode is not enabled, all the above functions do the Right
Thing on the single window.
In addition, there is a function window*-windows, which returns an
ordered list of the windows involved (or a list of the single window
when F.M. is not active).
I have a trial implementation of the above, and a trial adaptation of
isearch.el which uses it. It works.
What do you think?
> Stefan
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-29 23:23 ` Alan Mackenzie
@ 2015-10-31 22:35 ` John Wiegley
2015-10-31 22:35 ` John Wiegley
` (4 subsequent siblings)
5 siblings, 0 replies; 96+ messages in thread
From: John Wiegley @ 2015-10-31 22:35 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>>>>> Alan Mackenzie <acm@muc.de> writes:
> The new functions are window*-start, window*-end, set-window*-start,
> recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
>
> window*-start returns the window-start of the _first_ window in the group,
> window*-end returns the window-end of the _last_ window in the group, and so
> on, all these functions doing the Right Thing for a group of windows.
> sit*-for exists, because Follow Mode needs a chance to resynchronise its
> windows before redisplay happens.
What is the reason for having separate functions such as window*-start,
instead of just taking the car of a list of windows? I may be missing some
context here, but this sounds like special-casing general behavior, and I'm
wondering why it's necessary...
John
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-29 23:23 ` Alan Mackenzie
2015-10-31 22:35 ` John Wiegley
@ 2015-10-31 22:35 ` John Wiegley
2015-10-31 23:25 ` Alan Mackenzie
` (3 more replies)
2015-10-31 23:13 ` Artur Malabarba
` (3 subsequent siblings)
5 siblings, 4 replies; 96+ messages in thread
From: John Wiegley @ 2015-10-31 22:35 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>>>>> Alan Mackenzie <acm@muc.de> writes:
> The new functions are window*-start, window*-end, set-window*-start,
> recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
>
> window*-start returns the window-start of the _first_ window in the group,
> window*-end returns the window-end of the _last_ window in the group, and so
> on, all these functions doing the Right Thing for a group of windows.
> sit*-for exists, because Follow Mode needs a chance to resynchronise its
> windows before redisplay happens.
What is the reason for having separate functions such as window*-start,
instead of just taking the car of a list of windows? I may be missing some
context here, but this sounds like special-casing general behavior, and I'm
wondering why it's necessary...
John
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-29 23:23 ` Alan Mackenzie
2015-10-31 22:35 ` John Wiegley
2015-10-31 22:35 ` John Wiegley
@ 2015-10-31 23:13 ` Artur Malabarba
2015-10-31 23:13 ` Artur Malabarba
` (2 subsequent siblings)
5 siblings, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-10-31 23:13 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
[-- Attachment #1: Type: text/plain, Size: 889 bytes --]
On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm@muc.de> wrote:
>
> Hello, Stefan, hello, Emacs.
>
> Resuming a conversation from a year and a half ago, in it I had proposed
> a patch which made isearch work properly in a Follow Mode set of
> windows. In the end, you rejected my patch (in effect), because...
>
> On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
>
> > I must say I really dislike this hard-coding of follow-mode support in
> > isearch.el. Isearch should not know so much about follow-mode and
> > follow-mode should not know so much about Isearch either.
>
> > IOW we should try harder to come up with more general hooks.
What if isearch just took into account all windows displaying
current-buffer, instead of just the selected one?
This wouldn’t involve anything specific to follow mode, and I believe it
would solve the issue, no?
[-- Attachment #2: Type: text/html, Size: 1085 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-29 23:23 ` Alan Mackenzie
` (2 preceding siblings ...)
2015-10-31 23:13 ` Artur Malabarba
@ 2015-10-31 23:13 ` Artur Malabarba
2015-10-31 23:32 ` Alan Mackenzie
2015-10-31 23:32 ` Alan Mackenzie
2015-10-31 23:35 ` Juri Linkov
2015-10-31 23:35 ` Juri Linkov
5 siblings, 2 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-10-31 23:13 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
[-- Attachment #1: Type: text/plain, Size: 889 bytes --]
On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm@muc.de> wrote:
>
> Hello, Stefan, hello, Emacs.
>
> Resuming a conversation from a year and a half ago, in it I had proposed
> a patch which made isearch work properly in a Follow Mode set of
> windows. In the end, you rejected my patch (in effect), because...
>
> On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
>
> > I must say I really dislike this hard-coding of follow-mode support in
> > isearch.el. Isearch should not know so much about follow-mode and
> > follow-mode should not know so much about Isearch either.
>
> > IOW we should try harder to come up with more general hooks.
What if isearch just took into account all windows displaying
current-buffer, instead of just the selected one?
This wouldn’t involve anything specific to follow mode, and I believe it
would solve the issue, no?
[-- Attachment #2: Type: text/html, Size: 1085 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 22:35 ` John Wiegley
@ 2015-10-31 23:25 ` Alan Mackenzie
2015-10-31 23:25 ` Alan Mackenzie
` (2 subsequent siblings)
3 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-31 23:25 UTC (permalink / raw
To: Stefan Monnier, 17453, emacs-devel
Hello, John.
On Sat, Oct 31, 2015 at 03:35:22PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:
> > The new functions are window*-start, window*-end, set-window*-start,
> > recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
> > window*-start returns the window-start of the _first_ window in the group,
> > window*-end returns the window-end of the _last_ window in the group, and so
> > on, all these functions doing the Right Thing for a group of windows.
> > sit*-for exists, because Follow Mode needs a chance to resynchronise its
> > windows before redisplay happens.
> What is the reason for having separate functions such as window*-start,
> instead of just taking the car of a list of windows? I may be missing some
> context here, but this sounds like special-casing general behavior, and I'm
> wondering why it's necessary...
The prime use case is in isearch. At the moment isearch doesn't work
well when Follow Mode is active (try it!).
My first attempt at amending isearch for this involved manipulating
Follow Mode's list of windows directly inside isearch.el. The result
was too close a coupling between isearch and Follow Mode, and was also
rather ragged. Stefan rejected this patch, asking for a more abstract
solution.
What I am proposing now is a solution where any library which needs to
manipulate things like window positions will be trivially upgradable to
working with Follow Mode, merely by replacing `window-start' by
`window*-start', etc.
The fact that the "group" of windows is represented by a list is an
implementation detail to be encapsulated within follow.el. In the
(fairly distant) future, this might perhaps be superseded by code in
redisplay. Perhaps.
> John
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 22:35 ` John Wiegley
2015-10-31 23:25 ` Alan Mackenzie
@ 2015-10-31 23:25 ` Alan Mackenzie
2015-10-31 23:41 ` John Wiegley
2015-10-31 23:41 ` John Wiegley
2015-11-01 0:17 ` Drew Adams
2015-11-01 0:17 ` Drew Adams
3 siblings, 2 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-31 23:25 UTC (permalink / raw
To: Stefan Monnier, 17453, emacs-devel
Hello, John.
On Sat, Oct 31, 2015 at 03:35:22PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:
> > The new functions are window*-start, window*-end, set-window*-start,
> > recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
> > window*-start returns the window-start of the _first_ window in the group,
> > window*-end returns the window-end of the _last_ window in the group, and so
> > on, all these functions doing the Right Thing for a group of windows.
> > sit*-for exists, because Follow Mode needs a chance to resynchronise its
> > windows before redisplay happens.
> What is the reason for having separate functions such as window*-start,
> instead of just taking the car of a list of windows? I may be missing some
> context here, but this sounds like special-casing general behavior, and I'm
> wondering why it's necessary...
The prime use case is in isearch. At the moment isearch doesn't work
well when Follow Mode is active (try it!).
My first attempt at amending isearch for this involved manipulating
Follow Mode's list of windows directly inside isearch.el. The result
was too close a coupling between isearch and Follow Mode, and was also
rather ragged. Stefan rejected this patch, asking for a more abstract
solution.
What I am proposing now is a solution where any library which needs to
manipulate things like window positions will be trivially upgradable to
working with Follow Mode, merely by replacing `window-start' by
`window*-start', etc.
The fact that the "group" of windows is represented by a list is an
implementation detail to be encapsulated within follow.el. In the
(fairly distant) future, this might perhaps be superseded by code in
redisplay. Perhaps.
> John
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:13 ` Artur Malabarba
@ 2015-10-31 23:32 ` Alan Mackenzie
2015-10-31 23:32 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-31 23:32 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, Stefan Monnier, emacs-devel
Hello, Artur
On Sat, Oct 31, 2015 at 11:13:34PM +0000, Artur Malabarba wrote:
> On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> > Hello, Stefan, hello, Emacs.
> > Resuming a conversation from a year and a half ago, in it I had proposed
> > a patch which made isearch work properly in a Follow Mode set of
> > windows. In the end, you rejected my patch (in effect), because...
> > On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
> > > I must say I really dislike this hard-coding of follow-mode support in
> > > isearch.el. Isearch should not know so much about follow-mode and
> > > follow-mode should not know so much about Isearch either.
> > > IOW we should try harder to come up with more general hooks.
> What if isearch just took into account all windows displaying
> current-buffer, instead of just the selected one?
> This wouldn’t involve anything specific to follow mode, and I believe it
> would solve the issue, no?
I don't think so, really. What exactly does "took into account" mean?
With Follow Mode active, on a forward search you explicitly want point to
move into the next window when the search target is visible there. When
FM is not active, you most definitely don't want this to happen.
I have working code which make isearch and FM work together nicely. I
think it's a sufficiently "nice" implementation to commit.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:13 ` Artur Malabarba
2015-10-31 23:32 ` Alan Mackenzie
@ 2015-10-31 23:32 ` Alan Mackenzie
2015-11-01 12:20 ` Artur Malabarba
2015-11-01 12:20 ` Artur Malabarba
1 sibling, 2 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-31 23:32 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, Stefan Monnier, emacs-devel
Hello, Artur
On Sat, Oct 31, 2015 at 11:13:34PM +0000, Artur Malabarba wrote:
> On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> > Hello, Stefan, hello, Emacs.
> > Resuming a conversation from a year and a half ago, in it I had proposed
> > a patch which made isearch work properly in a Follow Mode set of
> > windows. In the end, you rejected my patch (in effect), because...
> > On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
> > > I must say I really dislike this hard-coding of follow-mode support in
> > > isearch.el. Isearch should not know so much about follow-mode and
> > > follow-mode should not know so much about Isearch either.
> > > IOW we should try harder to come up with more general hooks.
> What if isearch just took into account all windows displaying
> current-buffer, instead of just the selected one?
> This wouldn’t involve anything specific to follow mode, and I believe it
> would solve the issue, no?
I don't think so, really. What exactly does "took into account" mean?
With Follow Mode active, on a forward search you explicitly want point to
move into the next window when the search target is visible there. When
FM is not active, you most definitely don't want this to happen.
I have working code which make isearch and FM work together nicely. I
think it's a sufficiently "nice" implementation to commit.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-29 23:23 ` Alan Mackenzie
` (3 preceding siblings ...)
2015-10-31 23:13 ` Artur Malabarba
@ 2015-10-31 23:35 ` Juri Linkov
2015-10-31 23:35 ` Juri Linkov
5 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-10-31 23:35 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
> If Follow Mode is not enabled, all the above functions do the Right
> Thing on the single window.
From another perspective, settings lazy-highlight-buffer to t
(implemented in bug#21092) and removing the current restriction of
(overlay-put ov 'window (selected-window)) will lazy-highlight matches
in all follow windows with no effort.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-29 23:23 ` Alan Mackenzie
` (4 preceding siblings ...)
2015-10-31 23:35 ` Juri Linkov
@ 2015-10-31 23:35 ` Juri Linkov
2015-10-31 23:56 ` Alan Mackenzie
2015-10-31 23:56 ` Alan Mackenzie
5 siblings, 2 replies; 96+ messages in thread
From: Juri Linkov @ 2015-10-31 23:35 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
> If Follow Mode is not enabled, all the above functions do the Right
> Thing on the single window.
From another perspective, settings lazy-highlight-buffer to t
(implemented in bug#21092) and removing the current restriction of
(overlay-put ov 'window (selected-window)) will lazy-highlight matches
in all follow windows with no effort.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:25 ` Alan Mackenzie
@ 2015-10-31 23:41 ` John Wiegley
2015-10-31 23:41 ` John Wiegley
1 sibling, 0 replies; 96+ messages in thread
From: John Wiegley @ 2015-10-31 23:41 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>>>>> Alan Mackenzie <acm@muc.de> writes:
> What I am proposing now is a solution where any library which needs to
> manipulate things like window positions will be trivially upgradable to
> working with Follow Mode, merely by replacing `window-start' by
> `window*-start', etc.
Ah, I see. How many libraries do you think would need this change? Would using
window-start become bad practice under this regime?
John
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:25 ` Alan Mackenzie
2015-10-31 23:41 ` John Wiegley
@ 2015-10-31 23:41 ` John Wiegley
2015-11-01 11:59 ` Alan Mackenzie
2015-11-01 11:59 ` Alan Mackenzie
1 sibling, 2 replies; 96+ messages in thread
From: John Wiegley @ 2015-10-31 23:41 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>>>>> Alan Mackenzie <acm@muc.de> writes:
> What I am proposing now is a solution where any library which needs to
> manipulate things like window positions will be trivially upgradable to
> working with Follow Mode, merely by replacing `window-start' by
> `window*-start', etc.
Ah, I see. How many libraries do you think would need this change? Would using
window-start become bad practice under this regime?
John
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:35 ` Juri Linkov
@ 2015-10-31 23:56 ` Alan Mackenzie
2015-10-31 23:56 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-31 23:56 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Stefan Monnier, emacs-devel
Hello, Juri.
On Sun, Nov 01, 2015 at 01:35:53AM +0200, Juri Linkov wrote:
> > If Follow Mode is not enabled, all the above functions do the Right
> > Thing on the single window.
> >>From another perspective, settings lazy-highlight-buffer to t
> (implemented in bug#21092) and removing the current restriction of
> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
> in all follow windows with no effort.
I wasn't actually aware of that fix.
There were three main problems my patch fixed:
1) Searching commands were restricted to a single follow window. This
was caused by the lazy highlighting mechanism, as you say.
2) Lazy highlighting was only being done in a single window.
3) In scrolling commands, point was restricted to the singled window,
rather than being able to move freely throughout all the windows.
There were also some subtle things which could go wrong, in particular
when the current isearch match spans two windows, and the next command
causes the echo area to expand a line. This caused spurious scrolling
of the windows.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:35 ` Juri Linkov
2015-10-31 23:56 ` Alan Mackenzie
@ 2015-10-31 23:56 ` Alan Mackenzie
2015-11-02 0:14 ` Juri Linkov
2015-11-02 0:14 ` Juri Linkov
1 sibling, 2 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-10-31 23:56 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Stefan Monnier, emacs-devel
Hello, Juri.
On Sun, Nov 01, 2015 at 01:35:53AM +0200, Juri Linkov wrote:
> > If Follow Mode is not enabled, all the above functions do the Right
> > Thing on the single window.
> >>From another perspective, settings lazy-highlight-buffer to t
> (implemented in bug#21092) and removing the current restriction of
> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
> in all follow windows with no effort.
I wasn't actually aware of that fix.
There were three main problems my patch fixed:
1) Searching commands were restricted to a single follow window. This
was caused by the lazy highlighting mechanism, as you say.
2) Lazy highlighting was only being done in a single window.
3) In scrolling commands, point was restricted to the singled window,
rather than being able to move freely throughout all the windows.
There were also some subtle things which could go wrong, in particular
when the current isearch match spans two windows, and the next command
causes the echo area to expand a line. This caused spurious scrolling
of the windows.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 22:35 ` John Wiegley
` (2 preceding siblings ...)
2015-11-01 0:17 ` Drew Adams
@ 2015-11-01 0:17 ` Drew Adams
3 siblings, 0 replies; 96+ messages in thread
From: Drew Adams @ 2015-11-01 0:17 UTC (permalink / raw
To: John Wiegley, Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
It's really not great when people cross-post a bug thread
to emacs-devel (or vice versa).
It can sometimes be appropriate to post a message to one
or the other that references the other, to make people aware
of the thread in the other list. And it can sometimes be
appropriate to move a discussion from one list to the other.
But this needless multiplication is a bother.
(And yes, I've left both lists in this message to both.)
^ permalink raw reply [flat|nested] 96+ messages in thread
* RE: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 22:35 ` John Wiegley
2015-10-31 23:25 ` Alan Mackenzie
2015-10-31 23:25 ` Alan Mackenzie
@ 2015-11-01 0:17 ` Drew Adams
2015-11-01 0:17 ` Drew Adams
3 siblings, 0 replies; 96+ messages in thread
From: Drew Adams @ 2015-11-01 0:17 UTC (permalink / raw
To: John Wiegley, Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
It's really not great when people cross-post a bug thread
to emacs-devel (or vice versa).
It can sometimes be appropriate to post a message to one
or the other that references the other, to make people aware
of the thread in the other list. And it can sometimes be
appropriate to move a discussion from one list to the other.
But this needless multiplication is a bother.
(And yes, I've left both lists in this message to both.)
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:41 ` John Wiegley
@ 2015-11-01 11:59 ` Alan Mackenzie
2015-11-01 11:59 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-01 11:59 UTC (permalink / raw
To: Stefan Monnier, 17453, emacs-devel
Hello, John.
On Sat, Oct 31, 2015 at 04:41:01PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:
> > What I am proposing now is a solution where any library which needs to
> > manipulate things like window positions will be trivially upgradable to
> > working with Follow Mode, merely by replacing `window-start' by
> > `window*-start', etc.
> Ah, I see. How many libraries do you think would need this change?
I honestly don't know. At a guess, I'd say several rather than many.
As a quick benchmark, there are 127 occurrences of set-window-start in
our lisp sources, in 59 files.
> Would using window-start become bad practice under this regime?
I hadn't actually thought of that. Thinking about set-window-start
(rather than simply window-start), there will be lots of places where a
mode is explicitly handling its own windows (? speedbar.el, for example),
and set-window*-start would be the wrong thing. But there are also
surely examples where set-window-start is currently used just to scroll
the text to a specific position. Here set-window*-start would be
better.
> John
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:41 ` John Wiegley
2015-11-01 11:59 ` Alan Mackenzie
@ 2015-11-01 11:59 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-01 11:59 UTC (permalink / raw
To: Stefan Monnier, 17453, emacs-devel
Hello, John.
On Sat, Oct 31, 2015 at 04:41:01PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:
> > What I am proposing now is a solution where any library which needs to
> > manipulate things like window positions will be trivially upgradable to
> > working with Follow Mode, merely by replacing `window-start' by
> > `window*-start', etc.
> Ah, I see. How many libraries do you think would need this change?
I honestly don't know. At a guess, I'd say several rather than many.
As a quick benchmark, there are 127 occurrences of set-window-start in
our lisp sources, in 59 files.
> Would using window-start become bad practice under this regime?
I hadn't actually thought of that. Thinking about set-window-start
(rather than simply window-start), there will be lots of places where a
mode is explicitly handling its own windows (? speedbar.el, for example),
and set-window*-start would be the wrong thing. But there are also
surely examples where set-window-start is currently used just to scroll
the text to a specific position. Here set-window*-start would be
better.
> John
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:32 ` Alan Mackenzie
@ 2015-11-01 12:20 ` Artur Malabarba
2015-11-01 12:20 ` Artur Malabarba
1 sibling, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 12:20 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
Hi Alan,
>> What if isearch just took into account all windows displaying
>> current-buffer, instead of just the selected one?
>> This wouldn’t involve anything specific to follow mode, and I believe it
>> would solve the issue, no?
>
> I don't think so, really. What exactly does "took into account" mean?
> With Follow Mode active, on a forward search you explicitly want point to
> move into the next window when the search target is visible there. When
> FM is not active, you most definitely don't want this to happen.
Well, one of the reasons I suggested it is because I think I actually
would want that. But, of course, it would be conditioned on a variable
(disabled by default), and follow-mode would simply have to enable it.
> I have working code which make isearch and FM work together nicely. I
> think it's a sufficiently "nice" implementation to commit.
Sure. Don't take this as me trying to push you in a different
direction. I brought it up because it sounded like it would be
simpler.
You have a better understanding of the issue and the code than I do,
don't feel obliged to try my suggestion.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:32 ` Alan Mackenzie
2015-11-01 12:20 ` Artur Malabarba
@ 2015-11-01 12:20 ` Artur Malabarba
2015-11-01 12:23 ` Artur Malabarba
2015-11-01 12:23 ` Artur Malabarba
1 sibling, 2 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 12:20 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
Hi Alan,
>> What if isearch just took into account all windows displaying
>> current-buffer, instead of just the selected one?
>> This wouldn’t involve anything specific to follow mode, and I believe it
>> would solve the issue, no?
>
> I don't think so, really. What exactly does "took into account" mean?
> With Follow Mode active, on a forward search you explicitly want point to
> move into the next window when the search target is visible there. When
> FM is not active, you most definitely don't want this to happen.
Well, one of the reasons I suggested it is because I think I actually
would want that. But, of course, it would be conditioned on a variable
(disabled by default), and follow-mode would simply have to enable it.
> I have working code which make isearch and FM work together nicely. I
> think it's a sufficiently "nice" implementation to commit.
Sure. Don't take this as me trying to push you in a different
direction. I brought it up because it sounded like it would be
simpler.
You have a better understanding of the issue and the code than I do,
don't feel obliged to try my suggestion.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 12:20 ` Artur Malabarba
2015-11-01 12:23 ` Artur Malabarba
@ 2015-11-01 12:23 ` Artur Malabarba
1 sibling, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 12:23 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
2015-11-01 12:20 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> You have a better understanding of the issue and the code than I do,
> don't feel obliged to try my suggestion.
Anyway, could we see this code? :-)
Can you send it as patch or push it to a scratch/window-groups branch?
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 12:20 ` Artur Malabarba
@ 2015-11-01 12:23 ` Artur Malabarba
2015-11-01 13:52 ` Alan Mackenzie
2015-11-01 12:23 ` Artur Malabarba
1 sibling, 1 reply; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 12:23 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
2015-11-01 12:20 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> You have a better understanding of the issue and the code than I do,
> don't feel obliged to try my suggestion.
Anyway, could we see this code? :-)
Can you send it as patch or push it to a scratch/window-groups branch?
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 12:23 ` Artur Malabarba
@ 2015-11-01 13:52 ` Alan Mackenzie
2015-11-01 16:50 ` Eli Zaretskii
0 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-01 13:52 UTC (permalink / raw
To: Artur Malabarba; +Cc: emacs-devel
Hello, Artur.
On Sun, Nov 01, 2015 at 12:23:52PM +0000, Artur Malabarba wrote:
> 2015-11-01 12:20 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> > You have a better understanding of the issue and the code than I do,
> > don't feel obliged to try my suggestion.
> Anyway, could we see this code? :-)
> Can you send it as patch or push it to a scratch/window-groups branch?
No problem! The patch below doesn't actually apply at the moment, due
to some (?very) recent change in isearch.el. But it wouldn't run
anyway, because window*-start and friends aren't there yet.
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..07ec534 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -184,6 +184,11 @@ isearch-message-function
"Function to call to display the search prompt.
If nil, use function `isearch-message'.")
+(defmacro isearch-call-message (&optional cqh ellip)
+ `(if isearch-message-function
+ (funcall isearch-message-function ,cqh ,ellip)
+ (isearch-message ,cqh ,ellip)))
+
(defvar isearch-wrap-function nil
"Function to call to wrap the search when search is failed.
If nil, move point to the beginning of the buffer for a forward search,
@@ -615,6 +620,7 @@ isearch-adjusted
(defvar isearch-slow-terminal-mode nil)
;; If t, using a small window.
(defvar isearch-small-window nil)
+(defvar isearch-suspended-follow-mode nil) ; only used with a small window.
(defvar isearch-opoint 0)
;; The window configuration active at the beginning of the search.
(defvar isearch-window-configuration nil)
@@ -885,6 +891,7 @@ isearch-mode
(abs search-slow-window-lines))))
isearch-other-end nil
isearch-small-window nil
+ isearch-suspended-follow-mode nil
isearch-just-started t
isearch-start-hscroll (window-hscroll)
@@ -971,14 +978,15 @@ isearch-update
(null executing-kbd-macro))
(progn
(if (not (input-pending-p))
- (if isearch-message-function
- (funcall isearch-message-function)
- (isearch-message)))
+ (isearch-call-message))
(if (and isearch-slow-terminal-mode
(not (or isearch-small-window
- (pos-visible-in-window-p))))
+ (pos-visible-in-window*-p))))
(let ((found-point (point)))
(setq isearch-small-window t)
+ (when (and (boundp 'follow-mode) follow-mode)
+ (follow-mode 0)
+ (setq isearch-suspended-follow-mode t))
(move-to-window-line 0)
(let ((window-min-height 1))
(split-window nil (if (< search-slow-window-lines 0)
@@ -997,7 +1005,7 @@ isearch-update
(let ((current-scroll (window-hscroll))
visible-p)
(set-window-hscroll (selected-window) isearch-start-hscroll)
- (setq visible-p (pos-visible-in-window-p nil nil t))
+ (setq visible-p (pos-visible-in-window*-p nil nil t))
(if (or (not visible-p)
;; When point is not visible because of hscroll,
;; pos-visible-in-window-p returns non-nil, but
@@ -1051,17 +1059,20 @@ isearch-done
(setq minibuffer-message-timeout isearch-original-minibuffer-message-timeout)
(isearch-dehighlight)
(lazy-highlight-cleanup lazy-highlight-cleanup)
- (let ((found-start (window-start))
+ (let ((found-start (window*-start))
(found-point (point)))
(when isearch-window-configuration
(set-window-configuration isearch-window-configuration)
(if isearch-small-window
- (goto-char found-point)
+ (progn
+ (when isearch-suspended-follow-mode
+ (follow-mode 1))
+ (goto-char found-point))
;; set-window-configuration clobbers window-start; restore it.
;; This has an annoying side effect of clearing the last_modiff
;; field of the window, which can cause unwanted scrolling,
;; so don't do it unless truly necessary.
- (set-window-start (selected-window) found-start t))))
+ (set-window*-start (selected-window) found-start t))))
(setq isearch-mode nil)
(if isearch-input-method-local-p
@@ -1284,13 +1295,6 @@ with-isearch-suspended
(unwind-protect
(progn ,@body)
- ;; Set point at the start (end) of old match if forward (backward),
- ;; so after exiting minibuffer isearch resumes at the start (end)
- ;; of this match and can find it again.
- (if (and old-other-end (eq old-point (point))
- (eq isearch-forward isearch-new-forward))
- (goto-char old-other-end))
-
;; Always resume isearching by restarting it.
(isearch-mode isearch-forward
isearch-regexp
@@ -1303,7 +1307,17 @@ with-isearch-suspended
isearch-message isearch-new-message
isearch-forward isearch-new-forward
isearch-word isearch-new-word
- isearch-case-fold-search isearch-new-case-fold))
+ isearch-case-fold-search isearch-new-case-fold)
+
+ ;; Restore the minibuffer message before moving point.
+ (isearch-call-message nil t)
+
+ ;; Set point at the start (end) of old match if forward (backward),
+ ;; so after exiting minibuffer isearch resumes at the start (end)
+ ;; of this match and can find it again.
+ (if (and old-other-end (eq old-point (point))
+ (eq isearch-forward isearch-new-forward))
+ (goto-char old-other-end)))
;; Empty isearch-string means use default.
(when (= 0 (length isearch-string))
@@ -1898,6 +1912,7 @@ isearch-del-char
(length isearch-string))))
isearch-message (mapconcat 'isearch-text-char-description
isearch-string "")))
+ (isearch-call-message nil t) ; Do this before moving point.
;; Use the isearch-other-end as new starting point to be able
;; to find the remaining part of the search string again.
;; This is like what `isearch-search-and-update' does,
@@ -2074,6 +2089,7 @@ isearch-search-and-update
(setq isearch-case-fold-search
(isearch-no-upper-case-p isearch-string isearch-regexp))))
;; Not regexp, not reverse, or no match at point.
+ (isearch-call-message nil t) ; Do this before moving point.
(if (and isearch-other-end (not isearch-adjusted))
(goto-char (if isearch-forward isearch-other-end
(min isearch-opoint
@@ -2234,16 +2250,23 @@ isearch-allow-prefix
:type 'boolean
:group 'isearch)
+;; (defmacro save-excursion-and-window (&rest body)
+;; "FIXME!!!"
+;; `(let ((-old-win- (selected-window)))
+;; (save-excursion
+;; (unwind-protect (progn ,@body)
+;; (select-window -old-win- t)))))
+
(defun isearch-string-out-of-window (isearch-point)
"Test whether the search string is currently outside of the window.
Return nil if it's completely visible, or if point is visible,
together with as much of the search string as will fit; the symbol
`above' if we need to scroll the text downwards; the symbol `below',
if upwards."
- (let ((w-start (window-start))
- (w-end (window-end nil t))
- (w-L1 (save-excursion (move-to-window-line 1) (point)))
- (w-L-1 (save-excursion (move-to-window-line -1) (point)))
+ (let ((w-start (window*-start))
+ (w-end (window*-end nil t))
+ (w-L1 (save-selected-window (move-to-window*-line 1) (point)))
+ (w-L-1 (save-selected-window (move-to-window*-line -1) (point)))
start end) ; start and end of search string in buffer
(if isearch-forward
(setq end isearch-point start (or isearch-other-end isearch-point))
@@ -2270,15 +2293,15 @@ isearch-back-into-window
(if above
(progn
(goto-char start)
- (recenter 0)
- (when (>= isearch-point (window-end nil t))
+ (recenter* 0)
+ (when (>= isearch-point (window*-end nil t))
(goto-char isearch-point)
- (recenter -1)))
+ (recenter* -1)))
(goto-char end)
- (recenter -1)
- (when (< isearch-point (window-start))
+ (recenter* -1)
+ (when (< isearch-point (window*-start))
(goto-char isearch-point)
- (recenter 0))))
+ (recenter* 0))))
(goto-char isearch-point))
(defvar isearch-pre-scroll-point nil)
@@ -2425,6 +2448,7 @@ isearch-ring-adjust
(isearch-ring-adjust1 advance)
(if search-ring-update
(progn
+ (isearch-call-message nil t)
(isearch-search)
(isearch-push-state)
(isearch-update))
@@ -2497,6 +2521,13 @@ isearch-complete-edit
(defun isearch-message (&optional c-q-hack ellipsis)
;; Generate and print the message string.
+
+ ;; N.B.: This function should always be called with point at the
+ ;; search point, because in certain (rare) circumstances, undesired
+ ;; scrolling can happen when point is elsewhere. These
+ ;; circumstances are when follow-mode is active, the search string
+ ;; spans two (or several) windows, and the message about to be
+ ;; displayed will cause the echo area to expand.
(let ((cursor-in-echo-area ellipsis)
(m isearch-message)
(fail-pos (isearch-fail-pos t)))
@@ -2679,9 +2710,6 @@ isearch-search-string
(defun isearch-search ()
;; Do the search with the current search string.
- (if isearch-message-function
- (funcall isearch-message-function nil t)
- (isearch-message nil t))
(if (and (eq isearch-case-fold-search t) search-upper-case)
(setq isearch-case-fold-search
(isearch-no-upper-case-p isearch-string isearch-regexp)))
@@ -3015,11 +3043,13 @@ isearch-lazy-highlight-new-loop
search string to change or the window to scroll). It is also used
by other Emacs features."
(when (and (null executing-kbd-macro)
- (sit-for 0) ;make sure (window-start) is credible
+ ;; make sure (window*-start) is credible and windows are
+ ;; properly synchronised.
+ (sit*-for 0)
(or (not (equal isearch-string
isearch-lazy-highlight-last-string))
- (not (eq (selected-window)
- isearch-lazy-highlight-window))
+ (not (memq (selected-window)
+ (window*-windows isearch-lazy-highlight-window)))
(not (eq isearch-lazy-highlight-case-fold-search
isearch-case-fold-search))
(not (eq isearch-lazy-highlight-regexp
@@ -3030,9 +3060,9 @@ isearch-lazy-highlight-new-loop
isearch-lax-whitespace))
(not (eq isearch-lazy-highlight-regexp-lax-whitespace
isearch-regexp-lax-whitespace))
- (not (= (window-start)
+ (not (= (window*-start)
isearch-lazy-highlight-window-start))
- (not (= (window-end) ; Window may have been split/joined.
+ (not (= (window*-end) ; Window may have been split/joined.
isearch-lazy-highlight-window-end))
(not (eq isearch-forward
isearch-lazy-highlight-forward))
@@ -3048,8 +3078,8 @@ isearch-lazy-highlight-new-loop
(setq isearch-lazy-highlight-start-limit beg
isearch-lazy-highlight-end-limit end)
(setq isearch-lazy-highlight-window (selected-window)
- isearch-lazy-highlight-window-start (window-start)
- isearch-lazy-highlight-window-end (window-end)
+ isearch-lazy-highlight-window-start (window*-start)
+ isearch-lazy-highlight-window-end (window*-end)
;; Start lazy-highlighting at the beginning of the found
;; match (`isearch-other-end'). If no match, use point.
;; One of the next two variables (depending on search direction)
@@ -3093,13 +3123,13 @@ isearch-lazy-highlight-search
(+ isearch-lazy-highlight-start
;; Extend bound to match whole string at point
(1- (length isearch-lazy-highlight-last-string)))
- (window-end)))
+ (window*-end)))
(max (or isearch-lazy-highlight-start-limit (point-min))
(if isearch-lazy-highlight-wrapped
(- isearch-lazy-highlight-end
;; Extend bound to match whole string at point
(1- (length isearch-lazy-highlight-last-string)))
- (window-start))))))
+ (window*-start))))))
;; Use a loop like in `isearch-search'.
(while retry
(setq success (isearch-search-string
@@ -3115,6 +3145,24 @@ isearch-lazy-highlight-search
success)
(error nil)))
+(defun isearch-lazy-highlight-put-overlays (mb me)
+ "Put a highlighting overlay on the buffer for each pertinent window.
+
+An overlay is put over the positions (MB ME) in each window in the group
+of windows (e.g. in Follow Mode) which (at least partially) contains them."
+ ;; non-zero-length match
+ (mapc (lambda (w)
+ (if (and (< mb (window-end w))
+ (> me (window-start w)))
+ (let ((ov (make-overlay mb me)))
+ (push ov isearch-lazy-highlight-overlays)
+ ;; 1000 is higher than ediff's 100+,
+ ;; but lower than isearch main overlay's 1001
+ (overlay-put ov 'priority 1000)
+ (overlay-put ov 'face lazy-highlight-face)
+ (overlay-put ov 'window w))))
+ (window*-windows)))
+
(defun isearch-lazy-highlight-update ()
"Update highlighting of other matches for current search."
(let ((max lazy-highlight-max-at-a-time)
@@ -3143,23 +3191,15 @@ isearch-lazy-highlight-update
(if isearch-lazy-highlight-forward
(if (= mb (if isearch-lazy-highlight-wrapped
isearch-lazy-highlight-start
- (window-end)))
+ (window*-end)))
(setq found nil)
(forward-char 1))
(if (= mb (if isearch-lazy-highlight-wrapped
isearch-lazy-highlight-end
- (window-start)))
+ (window*-start)))
(setq found nil)
(forward-char -1)))
-
- ;; non-zero-length match
- (let ((ov (make-overlay mb me)))
- (push ov isearch-lazy-highlight-overlays)
- ;; 1000 is higher than ediff's 100+,
- ;; but lower than isearch main overlay's 1001
- (overlay-put ov 'priority 1000)
- (overlay-put ov 'face lazy-highlight-face)
- (overlay-put ov 'window (selected-window))))
+ (isearch-lazy-highlight-put-overlays mb me))
;; Remember the current position of point for
;; the next call of `isearch-lazy-highlight-update'
;; when `lazy-highlight-max-at-a-time' is too small.
@@ -3175,12 +3215,12 @@ isearch-lazy-highlight-update
(setq isearch-lazy-highlight-wrapped t)
(if isearch-lazy-highlight-forward
(progn
- (setq isearch-lazy-highlight-end (window-start))
+ (setq isearch-lazy-highlight-end (window*-start))
(goto-char (max (or isearch-lazy-highlight-start-limit (point-min))
- (window-start))))
- (setq isearch-lazy-highlight-start (window-end))
+ (window*-start))))
+ (setq isearch-lazy-highlight-start (window*-end))
(goto-char (min (or isearch-lazy-highlight-end-limit (point-max))
- (window-end))))))))
+ (window*-end))))))))
(unless nomore
(setq isearch-lazy-highlight-timer
(run-at-time lazy-highlight-interval nil
@@ -3198,6 +3238,7 @@ isearch-resume
(setq isearch-string string
isearch-message message
isearch-case-fold-search case-fold)
+ (isearch-call-message nil t)
(isearch-search)
(isearch-update))
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 13:52 ` Alan Mackenzie
@ 2015-11-01 16:50 ` Eli Zaretskii
2015-11-01 18:27 ` Alan Mackenzie
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-01 16:50 UTC (permalink / raw
To: Alan Mackenzie; +Cc: bruce.connor.am, emacs-devel
> Date: Sun, 1 Nov 2015 13:52:53 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: emacs-devel <emacs-devel@gnu.org>
>
> No problem! The patch below doesn't actually apply at the moment, due
> to some (?very) recent change in isearch.el. But it wouldn't run
> anyway, because window*-start and friends aren't there yet.
>
>
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 4fc9b38..07ec534 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
How is this "not having isearch know about Follow mode"? I see that
knowledge on every step of this patch, whenever you call the new
functions.
So maybe I'm missing something, but I really don't see why this
variant is significantly better than the one Stefan didn't like. Can
you explain what am I missing here?
I thought you will come up with some more generic framework for
commands to "scroll" the display by switching to the next window (when
under Follow mode), if possible. But unless I'm missing something
very important, this isn't that framework, is it?
Btw, I see no reason to introduce new functions. Instead, we could
have the original ones accept an additional optional argument.
As yet another comment, wrt this exchange between you and John:
> > What is the reason for having separate functions such as window*-start,
> > instead of just taking the car of a list of windows? I may be missing some
> > context here, but this sounds like special-casing general behavior, and I'm
> > wondering why it's necessary...
> [...]
> The fact that the "group" of windows is represented by a list is an
> implementation detail to be encapsulated within follow.el. In the
> (fairly distant) future, this might perhaps be superseded by code in
> redisplay. Perhaps.
You are right about not relying on the list, but window-next-sibling
and window-prev-sibling are available, and always will be, so you can
"trivially" use them instead of relying on a list. If you agree, then
John's question still stands, I think.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 16:50 ` Eli Zaretskii
@ 2015-11-01 18:27 ` Alan Mackenzie
2015-11-01 19:46 ` Artur Malabarba
2015-11-01 20:54 ` Eli Zaretskii
0 siblings, 2 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-01 18:27 UTC (permalink / raw
To: Eli Zaretskii; +Cc: bruce.connor.am, emacs-devel
Hello, Eli.
On Sun, Nov 01, 2015 at 06:50:00PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 1 Nov 2015 13:52:53 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: emacs-devel <emacs-devel@gnu.org>
> >
> > No problem! The patch below doesn't actually apply at the moment, due
> > to some (?very) recent change in isearch.el. But it wouldn't run
> > anyway, because window*-start and friends aren't there yet.
> >
> >
> >
> > diff --git a/lisp/isearch.el b/lisp/isearch.el
> > index 4fc9b38..07ec534 100644
> > --- a/lisp/isearch.el
> > +++ b/lisp/isearch.el
> How is this "not having isearch know about Follow mode"? I see that
> knowledge on every step of this patch, whenever you call the new
> functions.
It has no knowledge of the internals of Follow Mode. window*-start is
merely an abstract interface, currently with two implementations
associated with it, a default one for normal windows and a Follow Mode
one. More implementations could be added, the Follow Mode one could be
taken away.
I'm not sure I really understand your question. At several points in
isearch, functions need to be called which eventually end up calling
Follow Mode functions.
> So maybe I'm missing something, but I really don't see why this
> variant is significantly better than the one Stefan didn't like. Can
> you explain what am I missing here?
The new patch doesn't insert things like this into isearch.el:
+ (if (and (boundp 'follow-mode) follow-mode)
+ (progn (follow-adjust-window (selected-window))
+ t)
, though the old one from eighteen months ago did. The new patch
doesn't use any FM internal interfaces, indeed doesn't do anything
directly with FM, beyond temporarily disabling it when a "small window"
comes into effect because of a "slow terminal".
> I thought you will come up with some more generic framework for
> commands to "scroll" the display by switching to the next window (when
> under Follow mode), if possible. But unless I'm missing something
> very important, this isn't that framework, is it?
Follow Mode itself choses which window to leave point in. For example,
if isearch started in the left hand window, FM will place point in the
RH window if the first match is there.
window*-start and friends do constitute a generic framework. Any Elisp
library can replace window-start by window*-start, etc., and it then
operates on the entire group of FM windows rather than just one window.
> Btw, I see no reason to introduce new functions. Instead, we could
> have the original ones accept an additional optional argument.
We could. But that might make these functions less "pure": as well as
doing what they do, they would also have to execute some call-out to
Follow Mode in some fashion. The Follow Mode hook would then tell
window-start which window it _really_ should return the start of. Maybe
this would be better, maybe it would be more messy.
> As yet another comment, wrt this exchange between you and John:
> > > What is the reason for having separate functions such as window*-start,
> > > instead of just taking the car of a list of windows? I may be missing some
> > > context here, but this sounds like special-casing general behavior, and I'm
> > > wondering why it's necessary...
> > [...]
> > The fact that the "group" of windows is represented by a list is an
> > implementation detail to be encapsulated within follow.el. In the
> > (fairly distant) future, this might perhaps be superseded by code in
> > redisplay. Perhaps.
> You are right about not relying on the list, but window-next-sibling
> and window-prev-sibling are available, and always will be, so you can
> "trivially" use them instead of relying on a list.
The next/previous sibling might be displaying a different buffer. This
happens quite frequently when you look up a tag with M-.. But to use
window-next/previous-sibling properly would mean some fairly extensive
changes to the innards of Follow Mode - FM has its own left-to-right,
top-to-bottom algorithm for ordering its windows. (I'm not defending
this, by the way - it's just the way things are at the moment.)
> If you agree, then John's question still stands, I think.
The "car of a list of windows" he was talking about means that list of
windows needs to come from somewhere. The list is maintained by Follow
Mode, really only available using an internal FM function. As soon as
we use this, we have coupled isearch with Follow Mode more tightly.
This is undesirable.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 18:27 ` Alan Mackenzie
@ 2015-11-01 19:46 ` Artur Malabarba
2015-11-01 20:15 ` Alan Mackenzie
2015-11-01 20:42 ` Artur Malabarba
2015-11-01 20:54 ` Eli Zaretskii
1 sibling, 2 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 19:46 UTC (permalink / raw
To: Alan Mackenzie; +Cc: Eli Zaretskii, emacs-devel
>> How is this "not having isearch know about Follow mode"? I see that
>> knowledge on every step of this patch, whenever you call the new
>> functions.
>
> It has no knowledge of the internals of Follow Mode. window*-start is
> merely an abstract interface, currently with two implementations
> associated with it, a default one for normal windows and a Follow Mode
> one. [...]
> I'm not sure I really understand your question. At several points in
> isearch, functions need to be called which eventually end up calling
> Follow Mode functions.
This is not about the window*- functions.
There are several places where your patch checks whether follow-mode
is on, turns it off, or turns it back on. I think this is still too
attached to follow mode.
Isearch really shouldn't worry about disabling or enabling other libraries.
The idiomatic Emacs way to do this is to add hooks to isearch, and run
them at useful places. Then packages like follow-mode can add stuff to
these hooks and do what they want (like, enable/disable itself).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 19:46 ` Artur Malabarba
@ 2015-11-01 20:15 ` Alan Mackenzie
2015-11-01 21:37 ` Artur Malabarba
2015-11-01 20:42 ` Artur Malabarba
1 sibling, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-01 20:15 UTC (permalink / raw
To: Artur Malabarba; +Cc: emacs-devel
Hello, Artur.
On Sun, Nov 01, 2015 at 07:46:12PM +0000, Artur Malabarba wrote:
> >> How is this "not having isearch know about Follow mode"? I see that
> >> knowledge on every step of this patch, whenever you call the new
> >> functions.
> >
> > It has no knowledge of the internals of Follow Mode. window*-start is
> > merely an abstract interface, currently with two implementations
> > associated with it, a default one for normal windows and a Follow Mode
> > one. [...]
> > I'm not sure I really understand your question. At several points in
> > isearch, functions need to be called which eventually end up calling
> > Follow Mode functions.
> This is not about the window*- functions.
> There are several places where your patch checks whether follow-mode
> is on, turns it off, or turns it back on. I think this is still too
> attached to follow mode.
> Isearch really shouldn't worry about disabling or enabling other libraries.
Have a look more closely at that point. It is where an old (possibly
obsolete) facility of the "small window" is used. The idea is that if
a terminal's comms line is very slow, isearch will just display a single
line of text instead of scrolling an entire large window.
However, if Follow Mode is enabled, it would completely negate the point
of the "small window" since it would scroll all the other windows
displaying the buffer anyway.
Now, it's highly unlikely anybody on a 1200 Baud terminal is going to be
using Follow Mode anyway, but if they are, they'll still get the benefit
of the "small window".
Set your `baud-rate' to 100, and try this facility out!
> The idiomatic Emacs way to do this is to add hooks to isearch, and run
> them at useful places. Then packages like follow-mode can add stuff to
> these hooks and do what they want (like, enable/disable itself).
But the "small window" facility is hard coded, and probably has been for
30 years. Just up until now, nobody even considered the interaction
between "small window" and Follow Mode.
Still, this part of my patch is hardly essential to the main thrust. So
if people really think it inappropriate, I can just take this bit out.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 19:46 ` Artur Malabarba
2015-11-01 20:15 ` Alan Mackenzie
@ 2015-11-01 20:42 ` Artur Malabarba
1 sibling, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 20:42 UTC (permalink / raw
To: Alan Mackenzie; +Cc: Eli Zaretskii, emacs-devel
2015-11-01 19:46 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> The idiomatic Emacs way to do this is to add hooks to isearch, and run
> them at useful places. Then packages like follow-mode can add stuff to
> these hooks and do what they want (like, enable/disable itself).
After playing around a bit, here's a quick solution that works for me.
The patch adds 4 lines to isearch.el, and defines a function in
follow-mode.
I only tried it supperficially, so you might find issues with this
solution that I couldn't
---
lisp/follow.el | 14 ++++++++++++++
lisp/isearch.el | 4 ++++
2 files changed, 18 insertions(+)
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..e9a60e5 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -203,6 +203,8 @@
(require 'easymenu)
(eval-when-compile (require 'cl-lib))
+(require 'seq)
+(require 'subr-x)
;;; Variables
@@ -419,11 +421,13 @@ follow-mode
:keymap follow-mode-map
(if follow-mode
(progn
+ (setq-local isearch-search-fun-function #'follow--search-function)
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
;; Remove globally-installed hook functions only if there is no
;; other Follow mode buffer.
+ (setq-local isearch-search-fun-function #'isearch-search-fun-default)
(let ((buffers (buffer-list))
following)
(while (and (not following) buffers)
@@ -434,6 +438,16 @@ follow-mode
(remove-hook 'window-size-change-functions 'follow-window-size-change)))
(remove-hook 'compilation-filter-hook
'follow-align-compilation-windows t)))
+(defvar isearch-lazy-highlight-ongoing-search)
+(defun follow--search-function ()
+ (lambda (&rest args)
+ (prog1 (apply (isearch-search-fun-default) args)
+ (unless (or isearch-lazy-highlight-ongoing-search
+ (pos-visible-in-window-p))
+ (when-let ((win (seq-find (lambda (w)
(pos-visible-in-window-p (point) w))
+ (get-buffer-window-list))))
+ (select-window win))))))
+
(defun follow-find-file-hook ()
"Find-file hook for Follow mode. See the variable `follow-auto'."
(if follow-auto (follow-mode 1)))
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..daecdb5 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3075,11 +3075,15 @@ isearch-lazy-highlight-new-loop
(run-with-idle-timer lazy-highlight-initial-delay nil
'isearch-lazy-highlight-update)))))
+(defvar isearch-lazy-highlight-ongoing-search nil
+ "Bound to t while doing a lazy-highlight search.")
+
(defun isearch-lazy-highlight-search ()
"Search ahead for the next or previous match, for lazy highlighting.
Attempt to do the search exactly the way the pending Isearch would."
(condition-case nil
(let ((case-fold-search isearch-lazy-highlight-case-fold-search)
+ (isearch-lazy-highlight-ongoing-search t)
(isearch-regexp isearch-lazy-highlight-regexp)
(isearch-regexp-function isearch-lazy-highlight-regexp-function)
(isearch-lax-whitespace
--
2.5.3
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 18:27 ` Alan Mackenzie
2015-11-01 19:46 ` Artur Malabarba
@ 2015-11-01 20:54 ` Eli Zaretskii
2015-11-01 22:19 ` Alan Mackenzie
1 sibling, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-01 20:54 UTC (permalink / raw
To: Alan Mackenzie; +Cc: bruce.connor.am, emacs-devel
> Date: Sun, 1 Nov 2015 18:27:33 +0000
> Cc: bruce.connor.am@gmail.com, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
>
> > How is this "not having isearch know about Follow mode"? I see that
> > knowledge on every step of this patch, whenever you call the new
> > functions.
>
> It has no knowledge of the internals of Follow Mode.
But the window*-* functions do. And any code that calls them _knows_
about Follow mode.
Maybe it's just me, but I see no real difference in having the Follow
mode explicitly called out in a separate function, instead in
isearch.el itself.
> The new patch doesn't insert things like this into isearch.el:
>
> + (if (and (boundp 'follow-mode) follow-mode)
> + (progn (follow-adjust-window (selected-window))
> + t)
It inserts them indirectly via calling special functions. Which means
a package cannot really be oblivious to Follow mode's existence.
> > I thought you will come up with some more generic framework for
> > commands to "scroll" the display by switching to the next window (when
> > under Follow mode), if possible. But unless I'm missing something
> > very important, this isn't that framework, is it?
>
> Follow Mode itself choses which window to leave point in. For example,
> if isearch started in the left hand window, FM will place point in the
> RH window if the first match is there.
That's not what I see. What I see is that starting Isearch in the
left window scrolls that window, and then the right window scrolls
accordingly. What I'd expect (if I were a user of Follow mode) us an
automatic switch to the right window, and scroll only when that window
is also exhausted. That doesn't happen, at least not with the current
master.
In any case, what I thought you'd do is introduce a bunch of
variables, such as window-selection-for-scroll-function, that will
allow Follow mode to override the default (trivial) value and dictate
its own decisions to its clients. Or something like that.
> window*-start and friends do constitute a generic framework.
No, a generic framework would be, for example, to have a variable
whose value is a function, and let Follow mode override the default
value.
> > Btw, I see no reason to introduce new functions. Instead, we could
> > have the original ones accept an additional optional argument.
>
> We could. But that might make these functions less "pure": as well as
> doing what they do, they would also have to execute some call-out to
> Follow Mode in some fashion.
Isn't that what window*-* functions do anyway? Where's the benefit?
> > You are right about not relying on the list, but window-next-sibling
> > and window-prev-sibling are available, and always will be, so you can
> > "trivially" use them instead of relying on a list.
>
> The next/previous sibling might be displaying a different buffer.
But that's very easy to test, no?
> to use window-next/previous-sibling properly would mean some fairly
> extensive changes to the innards of Follow Mode - FM has its own
> left-to-right, top-to-bottom algorithm for ordering its windows.
What algorithm could that be, if not something that traverses the
window tree in some order?
> > If you agree, then John's question still stands, I think.
>
> The "car of a list of windows" he was talking about means that list of
> windows needs to come from somewhere. The list is maintained by Follow
> Mode, really only available using an internal FM function. As soon as
> we use this, we have coupled isearch with Follow Mode more tightly.
> This is undesirable.
Not if you go through a variable whose default value Follow mode can
override. Or some other decoupling technique like that, I'm sure
there are more, perhaps more elegant ones.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 20:15 ` Alan Mackenzie
@ 2015-11-01 21:37 ` Artur Malabarba
0 siblings, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-01 21:37 UTC (permalink / raw
To: Alan Mackenzie; +Cc: emacs-devel
2015-11-01 20:15 GMT+00:00 Alan Mackenzie <acm@muc.de>:
>> There are several places where your patch checks whether follow-mode
>> is on, turns it off, or turns it back on. [...] >
> Have a look more closely at that point. It is where an old (possibly
> obsolete) facility of the "small window" is used. [...] > Set your `baud-rate' to 100, and try this facility out
You learn something every day, I had no idea this facility existed. I
tried it and it's pretty neat. :-) Still...
> [...] > Still, this part of my patch is hardly essential to the main thrust. So
> if people really think it inappropriate, I can just take this bit out.
Yes, I think that's best. It's quite a bit of complexity to add just
to slightly improve the interplay with follow-mode on very old
terminals.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-01 20:54 ` Eli Zaretskii
@ 2015-11-01 22:19 ` Alan Mackenzie
0 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-01 22:19 UTC (permalink / raw
To: Eli Zaretskii; +Cc: bruce.connor.am, emacs-devel
Hello, Eli.
On Sun, Nov 01, 2015 at 10:54:21PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 1 Nov 2015 18:27:33 +0000
> > Cc: bruce.connor.am@gmail.com, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>
> > > How is this "not having isearch know about Follow mode"? I see that
> > > knowledge on every step of this patch, whenever you call the new
> > > functions.
> >
> > It has no knowledge of the internals of Follow Mode.
> > Follow Mode itself choses which window to leave point in. For example,
> > if isearch started in the left hand window, FM will place point in the
> > RH window if the first match is there.
> That's not what I see. What I see is that starting Isearch in the
> left window scrolls that window, and then the right window scrolls
> accordingly.
That is one of the bugs that my patch to isearch.el solves. But I think
Juri has knocked that one down already. It is caused by a glitch in
lazy highlighting. Disable lazy highlighting, and try it again. Then
C-s will move onto the next window rather than scrolling the one it
started on.
> In any case, what I thought you'd do is introduce a bunch of
> variables, such as window-selection-for-scroll-function, that will
> allow Follow mode to override the default (trivial) value and dictate
> its own decisions to its clients. Or something like that.
> > window*-start and friends do constitute a generic framework.
> No, a generic framework would be, for example, to have a variable
> whose value is a function, and let Follow mode override the default
> value.
That's precisely what I've done (but haven't submitted for review, yet).
In window.el, I've got code like this:
(defvar window*-start-function #'window-start)
(make-variable-buffer-local 'window*-start-function)
(put 'window*-start-function 'permanent-local t)
(defun window*-start (&optional window)
"Return position at which display currently starts in the group of
windows containing WINDOW. When a grouping mode (such as Follow Mode)
is not active, this function is identical to `window-start'.
WINDOW must be a live window and defaults to the selected one.
This is updated by redisplay or by calling `set-window*-start'."
(funcall window*-start-function window))
, and `follow-mode' sets window*-start-function to its own value when FM
is started, and kills the buffer local value when FM is disabled.
I think there's been some miscommunication over our last few posts.
Sorry about that.
> > > Btw, I see no reason to introduce new functions. Instead, we could
> > > have the original ones accept an additional optional argument.
> > We could. But that might make these functions less "pure": as well as
> > doing what they do, they would also have to execute some call-out to
> > Follow Mode in some fashion.
> Isn't that what window*-* functions do anyway? Where's the benefit?
Maybe there isn't really any. I don't feel too strongly about it. But
what would the optional extra parameter to window-start be called?
"group"?
It would mean the facility having to be coded in C rather than Lisp. I
suppose that's not too difficult. It would save having to have the
rather ugly names with "window*" in them.
> > > You are right about not relying on the list, but window-next-sibling
> > > and window-prev-sibling are available, and always will be, so you can
> > > "trivially" use them instead of relying on a list.
> > The next/previous sibling might be displaying a different buffer.
> But that's very easy to test, no?
Yes, but it would be duplicating what Follow Mode has already done in
one of its hooks (post-command-hook, I think). FM caters for any
arrangement of windows in a frame (not only side by side, but over
eachother, or a mixture) just like Emacs does. Maybe they have the same
notion of order of windows, maybe they don't. In the general case
(which surely doesn't happen in practice much), we'd have to scan the
frame's window tree more carefully.
> > to use window-next/previous-sibling properly would mean some fairly
> > extensive changes to the innards of Follow Mode - FM has its own
> > left-to-right, top-to-bottom algorithm for ordering its windows.
> What algorithm could that be, if not something that traverses the
> window tree in some order?
IIRC, FM takes the coordinates of the top left corner of each pertinent
window, and sorts them top to bottom first, then left to right for any
windows with the same Y coordinate. Or something like that.
> > > If you agree, then John's question still stands, I think.
> > The "car of a list of windows" he was talking about means that list of
> > windows needs to come from somewhere. The list is maintained by Follow
> > Mode, really only available using an internal FM function. As soon as
> > we use this, we have coupled isearch with Follow Mode more tightly.
> > This is undesirable.
> Not if you go through a variable whose default value Follow mode can
> override. Or some other decoupling technique like that, I'm sure
> there are more, perhaps more elegant ones.
I suppose so. But why should isearch have to know that it is the car of
the list which is the "first" window (for window*-start)?
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:56 ` Alan Mackenzie
2015-11-02 0:14 ` Juri Linkov
@ 2015-11-02 0:14 ` Juri Linkov
1 sibling, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 0:14 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>> >>From another perspective, settings lazy-highlight-buffer to t
>> (implemented in bug#21092) and removing the current restriction of
>> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
>> in all follow windows with no effort.
>
> I wasn't actually aware of that fix.
>
> There were three main problems my patch fixed:
> 1) Searching commands were restricted to a single follow window. This
> was caused by the lazy highlighting mechanism, as you say.
> 2) Lazy highlighting was only being done in a single window.
> 3) In scrolling commands, point was restricted to the singled window,
> rather than being able to move freely throughout all the windows.
IIUC, lazy highlighting is not a problem anymore, so the remaining
problem is how to switch windows when the next search position
happens to be in an adjacent window. Since there is no other
core library that require so much changes to adapt to follow-mode,
and there are already some mode-specific hooks in follow.el like
compilation-filter-hook, indicates that it's the responsibility of follow.el
to support isearch in follow-mode. So I believe the right way to do
this is like Artur presented in a short patch, and maybe it's possible
to simplify it even more by using isearch-update-post-hook with
a follow-align-compilation-windows like hook. I mean something like
(add-hook 'isearch-update-post-hook follow-align-isearch-windows t t)
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-10-31 23:56 ` Alan Mackenzie
@ 2015-11-02 0:14 ` Juri Linkov
2015-11-02 3:35 ` Eli Zaretskii
` (2 more replies)
2015-11-02 0:14 ` Juri Linkov
1 sibling, 3 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 0:14 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>> >>From another perspective, settings lazy-highlight-buffer to t
>> (implemented in bug#21092) and removing the current restriction of
>> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
>> in all follow windows with no effort.
>
> I wasn't actually aware of that fix.
>
> There were three main problems my patch fixed:
> 1) Searching commands were restricted to a single follow window. This
> was caused by the lazy highlighting mechanism, as you say.
> 2) Lazy highlighting was only being done in a single window.
> 3) In scrolling commands, point was restricted to the singled window,
> rather than being able to move freely throughout all the windows.
IIUC, lazy highlighting is not a problem anymore, so the remaining
problem is how to switch windows when the next search position
happens to be in an adjacent window. Since there is no other
core library that require so much changes to adapt to follow-mode,
and there are already some mode-specific hooks in follow.el like
compilation-filter-hook, indicates that it's the responsibility of follow.el
to support isearch in follow-mode. So I believe the right way to do
this is like Artur presented in a short patch, and maybe it's possible
to simplify it even more by using isearch-update-post-hook with
a follow-align-compilation-windows like hook. I mean something like
(add-hook 'isearch-update-post-hook follow-align-isearch-windows t t)
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 0:14 ` Juri Linkov
2015-11-02 3:35 ` Eli Zaretskii
@ 2015-11-02 3:35 ` Eli Zaretskii
2015-11-02 9:28 ` Alan Mackenzie
2 siblings, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-02 3:35 UTC (permalink / raw
To: Juri Linkov; +Cc: acm, 17453, monnier, emacs-devel
> From: Juri Linkov <juri@linkov.net>
> Date: Mon, 02 Nov 2015 02:14:47 +0200
> Cc: 17453@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
> emacs-devel@gnu.org
>
> Since there is no other core library that require so much changes to
> adapt to follow-mode, and there are already some mode-specific hooks
> in follow.el like compilation-filter-hook, indicates that it's the
> responsibility of follow.el to support isearch in follow-mode. So I
> believe the right way to do this is like Artur presented in a short
> patch, and maybe it's possible to simplify it even more by using
> isearch-update-post-hook with a follow-align-compilation-windows
> like hook. I mean something like (add-hook
> 'isearch-update-post-hook follow-align-isearch-windows t t)
I agree.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 0:14 ` Juri Linkov
@ 2015-11-02 3:35 ` Eli Zaretskii
2015-11-02 3:35 ` Eli Zaretskii
2015-11-02 9:28 ` Alan Mackenzie
2 siblings, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-02 3:35 UTC (permalink / raw
To: Juri Linkov; +Cc: acm, 17453, monnier, emacs-devel
> From: Juri Linkov <juri@linkov.net>
> Date: Mon, 02 Nov 2015 02:14:47 +0200
> Cc: 17453@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
> emacs-devel@gnu.org
>
> Since there is no other core library that require so much changes to
> adapt to follow-mode, and there are already some mode-specific hooks
> in follow.el like compilation-filter-hook, indicates that it's the
> responsibility of follow.el to support isearch in follow-mode. So I
> believe the right way to do this is like Artur presented in a short
> patch, and maybe it's possible to simplify it even more by using
> isearch-update-post-hook with a follow-align-compilation-windows
> like hook. I mean something like (add-hook
> 'isearch-update-post-hook follow-align-isearch-windows t t)
I agree.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 0:14 ` Juri Linkov
2015-11-02 3:35 ` Eli Zaretskii
2015-11-02 3:35 ` Eli Zaretskii
@ 2015-11-02 9:28 ` Alan Mackenzie
2015-11-02 11:53 ` Artur Malabarba
` (3 more replies)
2 siblings, 4 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 9:28 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Stefan Monnier, emacs-devel
Hi, Juri.
On Mon, Nov 02, 2015 at 02:14:47AM +0200, Juri Linkov wrote:
> >> >>From another perspective, settings lazy-highlight-buffer to t
> >> (implemented in bug#21092) and removing the current restriction of
> >> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
> >> in all follow windows with no effort.
> > I wasn't actually aware of that fix.
> > There were three main problems my patch fixed:
> > 1) Searching commands were restricted to a single follow window. This
> > was caused by the lazy highlighting mechanism, as you say.
> > 2) Lazy highlighting was only being done in a single window.
> > 3) In scrolling commands, point was restricted to the singled window,
> > rather than being able to move freely throughout all the windows.
> IIUC, lazy highlighting is not a problem anymore, ....
It is. With Follow Mode enabled, lazy highlighting is only occurring in
the one window, not all of them.
> .... so the remaining problem is how to switch windows when the next
> search position happens to be in an adjacent window.
That problem only shows itself when lazy-highlighting is enabled. This
suggests that the solution is to fix the lazy-highliting code so as not
to mess up the window arrangement.
The other problem which still exists is that when isearch-allow-scroll
is enabled, point cannot be scrolled into a neighbouring Follow Mode
window.
> Since there is no other core library that require so much changes to
> adapt to follow-mode, ....
I don't think we know this.
> .... and there are already some mode-specific hooks in follow.el like
> compilation-filter-hook, indicates that it's the responsibility of
> follow.el to support isearch in follow-mode.
There is nothing Follow Mode can do about isearch restricting its
lazy-highlighting and bounds for scrolling to one single window.
Isearch itself must be adapted for Follow Mode to work with it.
> So I believe the right way to do this is like Artur presented in a
> short patch, ....
I don't actually understand that patch, I'll need to study the wierd/new
constructs used in it, like `when-let'.
However, I'm certain that that patch will not fix all the problems
discussed in this post. One way or another, isearch MUST work with the
window boundaries of the entire Follow Mode group.
> .... and maybe it's possible to simplify it even more by using
> isearch-update-post-hook with a follow-align-compilation-windows like
> hook. I mean something like (add-hook 'isearch-update-post-hook
> follow-align-isearch-windows t t)
Hmm. I see that more as complicatifying rather than simplifying. The
imperative, set out by Stefan 18 months ago, was to keep internal Follow
Mode stuff out of isearch.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 9:28 ` Alan Mackenzie
2015-11-02 11:53 ` Artur Malabarba
@ 2015-11-02 11:53 ` Artur Malabarba
2015-11-02 23:33 ` Juri Linkov
2015-11-02 23:33 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 11:53 UTC (permalink / raw
To: Alan Mackenzie; +Cc: Juri Linkov, 17453, Stefan Monnier, emacs-devel
> I don't actually understand that patch, I'll need to study the wierd/new
> constructs used in it, like `when-let'.
Here's a more thoroughly explained version of this function, that
doesn't use when-let.
seq-find is equivalent to cl-find-if, it returns the first element
that matches the provided predicate.
(defun follow--search-function ()
(lambda (&rest args)
(let ((search-function (isearch-search-fun-default))
(matched (apply search-function args)))
;; If this is a proper user-triggered search (and not just a
;; lazy-highlight search), and if the search matched, and if the
;; match is not visible on this window...
(when (and matched
(not isearch-lazy-highlight-ongoing-search)
(not (and (pos-visible-in-window-p (match-beginning 0))
(pos-visible-in-window-p (match-end 0)))))
;; ... see if the match is visible on another window.
(let ((win (seq-find (lambda (w)
(and (pos-visible-in-window-p
(match-beginning 0) w)
(pos-visible-in-window-p (match-end 0) w)))
(follow-all-followers))))
;; If so, select it.
(when win
(select-window win))))
matched)))
I also changed it to use (follow-all-followers).
> However, I'm certain that that patch will not fix all the problems
> discussed in this post.
The patch as provided doesn't fix the “highlighting matches on all
windows” issue. But that's trivial to solve by removing that
`(overlay-put ov 'window (selected-window))' line. (which I have half
a mind to do right now because I just think it's a generally useful
improvement.)
> One way or another, isearch MUST work with the
> window boundaries of the entire Follow Mode group.
Maybe I missed part of the issue. I thought you wanted Isearch to
switch to another window if that window contains the next match
(instead of scrolling the current window). For that, you only need
pos-visible-in-window-p, you don't need to mess with boundaries.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 9:28 ` Alan Mackenzie
@ 2015-11-02 11:53 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
` (3 more replies)
2015-11-02 11:53 ` Artur Malabarba
` (2 subsequent siblings)
3 siblings, 4 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 11:53 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, emacs-devel, Stefan Monnier, Juri Linkov
> I don't actually understand that patch, I'll need to study the wierd/new
> constructs used in it, like `when-let'.
Here's a more thoroughly explained version of this function, that
doesn't use when-let.
seq-find is equivalent to cl-find-if, it returns the first element
that matches the provided predicate.
(defun follow--search-function ()
(lambda (&rest args)
(let ((search-function (isearch-search-fun-default))
(matched (apply search-function args)))
;; If this is a proper user-triggered search (and not just a
;; lazy-highlight search), and if the search matched, and if the
;; match is not visible on this window...
(when (and matched
(not isearch-lazy-highlight-ongoing-search)
(not (and (pos-visible-in-window-p (match-beginning 0))
(pos-visible-in-window-p (match-end 0)))))
;; ... see if the match is visible on another window.
(let ((win (seq-find (lambda (w)
(and (pos-visible-in-window-p
(match-beginning 0) w)
(pos-visible-in-window-p (match-end 0) w)))
(follow-all-followers))))
;; If so, select it.
(when win
(select-window win))))
matched)))
I also changed it to use (follow-all-followers).
> However, I'm certain that that patch will not fix all the problems
> discussed in this post.
The patch as provided doesn't fix the “highlighting matches on all
windows” issue. But that's trivial to solve by removing that
`(overlay-put ov 'window (selected-window))' line. (which I have half
a mind to do right now because I just think it's a generally useful
improvement.)
> One way or another, isearch MUST work with the
> window boundaries of the entire Follow Mode group.
Maybe I missed part of the issue. I thought you wanted Isearch to
switch to another window if that window contains the next match
(instead of scrolling the current window). For that, you only need
pos-visible-in-window-p, you don't need to mess with boundaries.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 11:53 ` Artur Malabarba
@ 2015-11-02 12:14 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
` (2 subsequent siblings)
3 siblings, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 12:14 UTC (permalink / raw
To: Alan Mackenzie; +Cc: Juri Linkov, 17453, Stefan Monnier, emacs-devel
>> One way or another, isearch MUST work with the
>> window boundaries of the entire Follow Mode group.
>
> Maybe I missed part of the issue. I thought you wanted Isearch to
> switch to another window if that window contains the next match
> (instead of scrolling the current window). For that, you only need
> pos-visible-in-window-p, you don't need to mess with boundaries.
I think I see what I'm missing. Lazy-highlighting stops at the end of
the window, so the matches on the second window won't be highlighted
until we switch to it, right?
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 11:53 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
@ 2015-11-02 12:14 ` Artur Malabarba
2015-11-02 12:39 ` Alan Mackenzie
2015-11-02 12:39 ` Alan Mackenzie
2015-11-02 12:35 ` Alan Mackenzie
2015-11-02 12:35 ` Alan Mackenzie
3 siblings, 2 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 12:14 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, emacs-devel, Stefan Monnier, Juri Linkov
>> One way or another, isearch MUST work with the
>> window boundaries of the entire Follow Mode group.
>
> Maybe I missed part of the issue. I thought you wanted Isearch to
> switch to another window if that window contains the next match
> (instead of scrolling the current window). For that, you only need
> pos-visible-in-window-p, you don't need to mess with boundaries.
I think I see what I'm missing. Lazy-highlighting stops at the end of
the window, so the matches on the second window won't be highlighted
until we switch to it, right?
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 11:53 ` Artur Malabarba
` (2 preceding siblings ...)
2015-11-02 12:35 ` Alan Mackenzie
@ 2015-11-02 12:35 ` Alan Mackenzie
3 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 12:35 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, emacs-devel, Stefan Monnier, Juri Linkov
Hello, Artur.
On Mon, Nov 02, 2015 at 11:53:10AM +0000, Artur Malabarba wrote:
> > I don't actually understand that patch, I'll need to study the wierd/new
> > constructs used in it, like `when-let'.
> Here's a more thoroughly explained version of this function, that
> doesn't use when-let.
> seq-find is equivalent to cl-find-if, it returns the first element
> that matches the provided predicate.
OK.
[ .... ]
> I also changed it to use (follow-all-followers).
That's unwanted. ;-) Isearch should not be using internal Follow Mode
functions. Some more abstraction would be needed.
> > However, I'm certain that that patch will not fix all the problems
> > discussed in this post.
> The patch as provided doesn't fix the “highlighting matches on all
> windows” issue. But that's trivial to solve by removing that
> `(overlay-put ov 'window (selected-window))' line. (which I have half
> a mind to do right now because I just think it's a generally useful
> improvement.)
Lazy highlighting searches for matches on the current window. It must
be extended to search for matches on the Follow Mode group of windows.
For that, it needs the details of the "window*-start" and "window*-end".
> > One way or another, isearch MUST work with the
> > window boundaries of the entire Follow Mode group.
> Maybe I missed part of the issue. I thought you wanted Isearch to
> switch to another window if that window contains the next match
> (instead of scrolling the current window). For that, you only need
> pos-visible-in-window-p, you don't need to mess with boundaries.
What is causing the unwanted scrolling rather than moving to the next
window, is the form "(sit-for 0)" near the start of
isearch-lazy-highlight-new-loop. When point is outside the window, this
form causes redisplay, which scrolls point back into the window -
without Follow Mode getting a look in. In my patch, I replaced this
with "(sit*-for 0)", where Follow Mode can do its thing before the
redisplay happens.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 11:53 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
@ 2015-11-02 12:35 ` Alan Mackenzie
2015-11-02 13:10 ` Artur Malabarba
` (3 more replies)
2015-11-02 12:35 ` Alan Mackenzie
3 siblings, 4 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 12:35 UTC (permalink / raw
To: Artur Malabarba; +Cc: Juri Linkov, 17453, Stefan Monnier, emacs-devel
Hello, Artur.
On Mon, Nov 02, 2015 at 11:53:10AM +0000, Artur Malabarba wrote:
> > I don't actually understand that patch, I'll need to study the wierd/new
> > constructs used in it, like `when-let'.
> Here's a more thoroughly explained version of this function, that
> doesn't use when-let.
> seq-find is equivalent to cl-find-if, it returns the first element
> that matches the provided predicate.
OK.
[ .... ]
> I also changed it to use (follow-all-followers).
That's unwanted. ;-) Isearch should not be using internal Follow Mode
functions. Some more abstraction would be needed.
> > However, I'm certain that that patch will not fix all the problems
> > discussed in this post.
> The patch as provided doesn't fix the “highlighting matches on all
> windows” issue. But that's trivial to solve by removing that
> `(overlay-put ov 'window (selected-window))' line. (which I have half
> a mind to do right now because I just think it's a generally useful
> improvement.)
Lazy highlighting searches for matches on the current window. It must
be extended to search for matches on the Follow Mode group of windows.
For that, it needs the details of the "window*-start" and "window*-end".
> > One way or another, isearch MUST work with the
> > window boundaries of the entire Follow Mode group.
> Maybe I missed part of the issue. I thought you wanted Isearch to
> switch to another window if that window contains the next match
> (instead of scrolling the current window). For that, you only need
> pos-visible-in-window-p, you don't need to mess with boundaries.
What is causing the unwanted scrolling rather than moving to the next
window, is the form "(sit-for 0)" near the start of
isearch-lazy-highlight-new-loop. When point is outside the window, this
form causes redisplay, which scrolls point back into the window -
without Follow Mode getting a look in. In my patch, I replaced this
with "(sit*-for 0)", where Follow Mode can do its thing before the
redisplay happens.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 12:14 ` Artur Malabarba
2015-11-02 12:39 ` Alan Mackenzie
@ 2015-11-02 12:39 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 12:39 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, emacs-devel, Stefan Monnier, Juri Linkov
Hello, Artur.
On Mon, Nov 02, 2015 at 12:14:12PM +0000, Artur Malabarba wrote:
> >> One way or another, isearch MUST work with the
> >> window boundaries of the entire Follow Mode group.
> > Maybe I missed part of the issue. I thought you wanted Isearch to
> > switch to another window if that window contains the next match
> > (instead of scrolling the current window). For that, you only need
> > pos-visible-in-window-p, you don't need to mess with boundaries.
> I think I see what I'm missing. Lazy-highlighting stops at the end of
> the window, so the matches on the second window won't be highlighted
> until we switch to it, right?
Absolutely! Not forgetting, of course, matches which start near the
bottom of one window, and carry on at the top of the next window.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 12:14 ` Artur Malabarba
@ 2015-11-02 12:39 ` Alan Mackenzie
2015-11-02 12:39 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 12:39 UTC (permalink / raw
To: Artur Malabarba; +Cc: Juri Linkov, 17453, Stefan Monnier, emacs-devel
Hello, Artur.
On Mon, Nov 02, 2015 at 12:14:12PM +0000, Artur Malabarba wrote:
> >> One way or another, isearch MUST work with the
> >> window boundaries of the entire Follow Mode group.
> > Maybe I missed part of the issue. I thought you wanted Isearch to
> > switch to another window if that window contains the next match
> > (instead of scrolling the current window). For that, you only need
> > pos-visible-in-window-p, you don't need to mess with boundaries.
> I think I see what I'm missing. Lazy-highlighting stops at the end of
> the window, so the matches on the second window won't be highlighted
> until we switch to it, right?
Absolutely! Not forgetting, of course, matches which start near the
bottom of one window, and carry on at the top of the next window.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 12:35 ` Alan Mackenzie
2015-11-02 13:10 ` Artur Malabarba
@ 2015-11-02 13:10 ` Artur Malabarba
2015-11-02 15:46 ` Eli Zaretskii
[not found] ` <<831tc8xv39.fsf@gnu.org>
3 siblings, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 13:10 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, emacs-devel, Stefan Monnier, Juri Linkov
>> I also changed it to use (follow-all-followers).
>
> That's unwanted. ;-) Isearch should not be using internal Follow Mode
> functions. Some more abstraction would be needed.
That function goes in follow.el. ;-)
> Lazy highlighting searches for matches on the current window. It must
> be extended to search for matches on the Follow Mode group of windows.
> For that, it needs the details of the "window*-start" and "window*-end".
Yes, you're right, I see that now. This might still be solvable via
setting isearch-search-fun-function, I'll need to think on that for a
moment.
I see nothing wrong with the window*-start/end functionality you
suggest, but if we can solve this problem via configuration points
that are already provided (like isearch-search-fun-function) then all
the better.
>> > One way or another, isearch MUST work with the
>> > window boundaries of the entire Follow Mode group.
>
>> Maybe I missed part of the issue. I thought you wanted Isearch to
>> switch to another window if that window contains the next match
>> (instead of scrolling the current window). For that, you only need
>> pos-visible-in-window-p, you don't need to mess with boundaries.
>
> What is causing the unwanted scrolling rather than moving to the next
> window, is the form "(sit-for 0)" near the start of
> isearch-lazy-highlight-new-loop.
I think the patch I provided switches to the second (or third or
whatever) window before that sit-for runs. So I believe it solves this
problem.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 12:35 ` Alan Mackenzie
@ 2015-11-02 13:10 ` Artur Malabarba
2015-11-02 14:18 ` Artur Malabarba
2015-11-02 14:18 ` Artur Malabarba
2015-11-02 13:10 ` Artur Malabarba
` (2 subsequent siblings)
3 siblings, 2 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 13:10 UTC (permalink / raw
To: Alan Mackenzie; +Cc: Juri Linkov, 17453, Stefan Monnier, emacs-devel
>> I also changed it to use (follow-all-followers).
>
> That's unwanted. ;-) Isearch should not be using internal Follow Mode
> functions. Some more abstraction would be needed.
That function goes in follow.el. ;-)
> Lazy highlighting searches for matches on the current window. It must
> be extended to search for matches on the Follow Mode group of windows.
> For that, it needs the details of the "window*-start" and "window*-end".
Yes, you're right, I see that now. This might still be solvable via
setting isearch-search-fun-function, I'll need to think on that for a
moment.
I see nothing wrong with the window*-start/end functionality you
suggest, but if we can solve this problem via configuration points
that are already provided (like isearch-search-fun-function) then all
the better.
>> > One way or another, isearch MUST work with the
>> > window boundaries of the entire Follow Mode group.
>
>> Maybe I missed part of the issue. I thought you wanted Isearch to
>> switch to another window if that window contains the next match
>> (instead of scrolling the current window). For that, you only need
>> pos-visible-in-window-p, you don't need to mess with boundaries.
>
> What is causing the unwanted scrolling rather than moving to the next
> window, is the form "(sit-for 0)" near the start of
> isearch-lazy-highlight-new-loop.
I think the patch I provided switches to the second (or third or
whatever) window before that sit-for runs. So I believe it solves this
problem.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 13:10 ` Artur Malabarba
2015-11-02 14:18 ` Artur Malabarba
@ 2015-11-02 14:18 ` Artur Malabarba
1 sibling, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 14:18 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, emacs-devel, Stefan Monnier, Juri Linkov
[-- Attachment #1: Type: text/plain, Size: 828 bytes --]
> > Lazy highlighting searches for matches on the current window. It must
> > be extended to search for matches on the Follow Mode group of windows.
> > For that, it needs the details of the "window*-start" and "window*-end".
>
> Yes, you're right, I see that now. This might still be solvable via
> setting isearch-search-fun-function, I'll need to think on that for a
> moment.
Here's a new version, split into 3 short patches to make it easier to
understand.
This makes 2 small changes to isearch (which I think are even
justifiable on their own, regardless of follow-mode), and defines a
new function in follow-mode which takes care of two things:
Matches are highlighted on all windows
When the user types something, and the next match is on another
window, switch to that window instead of scrolling the current one.
[-- Attachment #2: 0003-lisp-follow.el-Improve-isearch-compatibility.patch --]
[-- Type: text/x-patch, Size: 2951 bytes --]
From caf5adfc0b7caf07dc74813242f9ecc664babc13 Mon Sep 17 00:00:00 2001
From: Artur Malabarba <bruce.connor.am@gmail.com>
Date: Mon, 2 Nov 2015 14:01:18 +0000
Subject: [PATCH 3/3] * lisp/follow.el: Improve isearch compatibility
(follow--search-function): New function.
(follow-mode): Configure `isearch-lazy-highlight' and
`isearch-search-fun-function'.
---
lisp/follow.el | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..0b80f04 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -419,6 +419,9 @@ follow-mode
:keymap follow-mode-map
(if follow-mode
(progn
+ (setq-local isearch-search-fun-function #'follow--search-function)
+ (when isearch-lazy-highlight
+ (setq-local isearch-lazy-highlight 'all-windows))
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
@@ -434,6 +437,36 @@ follow-mode
(remove-hook 'window-size-change-functions 'follow-window-size-change)))
(remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
+(defun follow--search-function ()
+ "Return a function suitable for `isearch-search-fun-function'."
+ (lambda (string &optional bound noerror count)
+ (let* ((search-function (isearch-search-fun-default))
+ (all-followers (follow-all-followers))
+ ;; If bound at the edge of the window, extend it to the
+ ;; edges know by `follow-mode'.
+ (bound (cond ((equal bound (window-end))
+ (window-end (car (last all-followers))))
+ ((equal bound (window-start))
+ (window-start (car all-followers)))
+ (t bound)))
+ (matched (funcall search-function string bound noerror count)))
+ ;; If this is a proper user-triggered search (and not just a
+ ;; lazy-highlight search), and if the search matched, and if the
+ ;; match is not visible on this window...
+ (when (and matched
+ (not isearch-lazy-highlight-ongoing-search)
+ (not (and (pos-visible-in-window-p (match-beginning 0))
+ (pos-visible-in-window-p (match-end 0)))))
+ ;; ... see if the match is visible on another window.
+ (let ((win (seq-find (lambda (w)
+ (and (pos-visible-in-window-p (match-beginning 0) w)
+ (pos-visible-in-window-p (match-end 0) w)))
+ all-followers)))
+ ;; If so, select it.
+ (when win
+ (select-window win))))
+ matched)))
+
(defun follow-find-file-hook ()
"Find-file hook for Follow mode. See the variable `follow-auto'."
(if follow-auto (follow-mode 1)))
--
2.6.2
[-- Attachment #3: 0002-lisp-isearch.el-Bind-a-variable-while-lazy-highlight.patch --]
[-- Type: text/x-patch, Size: 1458 bytes --]
From 8a52f12872d2b99acb2ca5a2077ccd87779309fe Mon Sep 17 00:00:00 2001
From: Artur Malabarba <bruce.connor.am@gmail.com>
Date: Mon, 2 Nov 2015 13:54:15 +0000
Subject: [PATCH 2/3] * lisp/isearch.el: Bind a variable while
lazy-highlighting
(isearch-lazy-highlight-ongoing-search): New variable.
(isearch-lazy-highlight-search): Use it.
---
lisp/isearch.el | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 6442166..ef49650 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3080,11 +3080,18 @@ isearch-lazy-highlight-new-loop
(run-with-idle-timer lazy-highlight-initial-delay nil
'isearch-lazy-highlight-update)))))
+(defvar isearch-lazy-highlight-ongoing-search nil
+ "Dynamically bound to t in `isearch-lazy-highlight-search'.
+This can be used by `isearch-search-fun-function' to detect
+whether its return value is being run for a proper user search or
+a lazy highlight search.")
+
(defun isearch-lazy-highlight-search ()
"Search ahead for the next or previous match, for lazy highlighting.
Attempt to do the search exactly the way the pending Isearch would."
(condition-case nil
(let ((case-fold-search isearch-lazy-highlight-case-fold-search)
+ (isearch-lazy-highlight-ongoing-search t)
(isearch-regexp isearch-lazy-highlight-regexp)
(isearch-regexp-function isearch-lazy-highlight-regexp-function)
(isearch-lax-whitespace
--
2.6.2
[-- Attachment #4: 0001-lisp-isearch.el-Allow-lazy-highlighting-of-all-windo.patch --]
[-- Type: text/x-patch, Size: 1763 bytes --]
From 0c815fa21001b4a29f124e8eda58f7889560d701 Mon Sep 17 00:00:00 2001
From: Artur Malabarba <bruce.connor.am@gmail.com>
Date: Mon, 2 Nov 2015 12:09:16 +0000
Subject: [PATCH 1/3] * lisp/isearch.el: Allow lazy-highlighting of all windows
(isearch-lazy-highlight): New possible value.
(isearch-lazy-highlight-update): Use it.
---
lisp/isearch.el | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..6442166 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -279,8 +279,13 @@ isearch-lazy-highlight
"Controls the lazy-highlighting during incremental search.
When non-nil, all text in the buffer matching the current search
string is highlighted lazily (see `lazy-highlight-initial-delay'
-and `lazy-highlight-interval')."
- :type 'boolean
+and `lazy-highlight-interval').
+
+When multiple windows display the current buffer, the
+highlighting is displayed only on the selected window, unless
+this variable is set to the symbol `all-windows'."
+ :type '(choice boolean
+ (const :tag "On, and applied to all windows" all-windows))
:group 'lazy-highlight
:group 'isearch)
@@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
;; but lower than isearch main overlay's 1001
(overlay-put ov 'priority 1000)
(overlay-put ov 'face lazy-highlight-face)
- (overlay-put ov 'window (selected-window))))
+ (unless (eq isearch-lazy-highlight 'all-windows)
+ (overlay-put ov 'window (selected-window)))))
;; Remember the current position of point for
;; the next call of `isearch-lazy-highlight-update'
;; when `lazy-highlight-max-at-a-time' is too small.
--
2.6.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 13:10 ` Artur Malabarba
@ 2015-11-02 14:18 ` Artur Malabarba
2015-11-02 15:44 ` Alan Mackenzie
` (3 more replies)
2015-11-02 14:18 ` Artur Malabarba
1 sibling, 4 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 14:18 UTC (permalink / raw
To: Alan Mackenzie; +Cc: Juri Linkov, 17453, Stefan Monnier, emacs-devel
[-- Attachment #1: Type: text/plain, Size: 828 bytes --]
> > Lazy highlighting searches for matches on the current window. It must
> > be extended to search for matches on the Follow Mode group of windows.
> > For that, it needs the details of the "window*-start" and "window*-end".
>
> Yes, you're right, I see that now. This might still be solvable via
> setting isearch-search-fun-function, I'll need to think on that for a
> moment.
Here's a new version, split into 3 short patches to make it easier to
understand.
This makes 2 small changes to isearch (which I think are even
justifiable on their own, regardless of follow-mode), and defines a
new function in follow-mode which takes care of two things:
Matches are highlighted on all windows
When the user types something, and the next match is on another
window, switch to that window instead of scrolling the current one.
[-- Attachment #2: 0003-lisp-follow.el-Improve-isearch-compatibility.patch --]
[-- Type: text/x-patch, Size: 2951 bytes --]
From caf5adfc0b7caf07dc74813242f9ecc664babc13 Mon Sep 17 00:00:00 2001
From: Artur Malabarba <bruce.connor.am@gmail.com>
Date: Mon, 2 Nov 2015 14:01:18 +0000
Subject: [PATCH 3/3] * lisp/follow.el: Improve isearch compatibility
(follow--search-function): New function.
(follow-mode): Configure `isearch-lazy-highlight' and
`isearch-search-fun-function'.
---
lisp/follow.el | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..0b80f04 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -419,6 +419,9 @@ follow-mode
:keymap follow-mode-map
(if follow-mode
(progn
+ (setq-local isearch-search-fun-function #'follow--search-function)
+ (when isearch-lazy-highlight
+ (setq-local isearch-lazy-highlight 'all-windows))
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
@@ -434,6 +437,36 @@ follow-mode
(remove-hook 'window-size-change-functions 'follow-window-size-change)))
(remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
+(defun follow--search-function ()
+ "Return a function suitable for `isearch-search-fun-function'."
+ (lambda (string &optional bound noerror count)
+ (let* ((search-function (isearch-search-fun-default))
+ (all-followers (follow-all-followers))
+ ;; If bound at the edge of the window, extend it to the
+ ;; edges know by `follow-mode'.
+ (bound (cond ((equal bound (window-end))
+ (window-end (car (last all-followers))))
+ ((equal bound (window-start))
+ (window-start (car all-followers)))
+ (t bound)))
+ (matched (funcall search-function string bound noerror count)))
+ ;; If this is a proper user-triggered search (and not just a
+ ;; lazy-highlight search), and if the search matched, and if the
+ ;; match is not visible on this window...
+ (when (and matched
+ (not isearch-lazy-highlight-ongoing-search)
+ (not (and (pos-visible-in-window-p (match-beginning 0))
+ (pos-visible-in-window-p (match-end 0)))))
+ ;; ... see if the match is visible on another window.
+ (let ((win (seq-find (lambda (w)
+ (and (pos-visible-in-window-p (match-beginning 0) w)
+ (pos-visible-in-window-p (match-end 0) w)))
+ all-followers)))
+ ;; If so, select it.
+ (when win
+ (select-window win))))
+ matched)))
+
(defun follow-find-file-hook ()
"Find-file hook for Follow mode. See the variable `follow-auto'."
(if follow-auto (follow-mode 1)))
--
2.6.2
[-- Attachment #3: 0002-lisp-isearch.el-Bind-a-variable-while-lazy-highlight.patch --]
[-- Type: text/x-patch, Size: 1458 bytes --]
From 8a52f12872d2b99acb2ca5a2077ccd87779309fe Mon Sep 17 00:00:00 2001
From: Artur Malabarba <bruce.connor.am@gmail.com>
Date: Mon, 2 Nov 2015 13:54:15 +0000
Subject: [PATCH 2/3] * lisp/isearch.el: Bind a variable while
lazy-highlighting
(isearch-lazy-highlight-ongoing-search): New variable.
(isearch-lazy-highlight-search): Use it.
---
lisp/isearch.el | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 6442166..ef49650 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3080,11 +3080,18 @@ isearch-lazy-highlight-new-loop
(run-with-idle-timer lazy-highlight-initial-delay nil
'isearch-lazy-highlight-update)))))
+(defvar isearch-lazy-highlight-ongoing-search nil
+ "Dynamically bound to t in `isearch-lazy-highlight-search'.
+This can be used by `isearch-search-fun-function' to detect
+whether its return value is being run for a proper user search or
+a lazy highlight search.")
+
(defun isearch-lazy-highlight-search ()
"Search ahead for the next or previous match, for lazy highlighting.
Attempt to do the search exactly the way the pending Isearch would."
(condition-case nil
(let ((case-fold-search isearch-lazy-highlight-case-fold-search)
+ (isearch-lazy-highlight-ongoing-search t)
(isearch-regexp isearch-lazy-highlight-regexp)
(isearch-regexp-function isearch-lazy-highlight-regexp-function)
(isearch-lax-whitespace
--
2.6.2
[-- Attachment #4: 0001-lisp-isearch.el-Allow-lazy-highlighting-of-all-windo.patch --]
[-- Type: text/x-patch, Size: 1763 bytes --]
From 0c815fa21001b4a29f124e8eda58f7889560d701 Mon Sep 17 00:00:00 2001
From: Artur Malabarba <bruce.connor.am@gmail.com>
Date: Mon, 2 Nov 2015 12:09:16 +0000
Subject: [PATCH 1/3] * lisp/isearch.el: Allow lazy-highlighting of all windows
(isearch-lazy-highlight): New possible value.
(isearch-lazy-highlight-update): Use it.
---
lisp/isearch.el | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..6442166 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -279,8 +279,13 @@ isearch-lazy-highlight
"Controls the lazy-highlighting during incremental search.
When non-nil, all text in the buffer matching the current search
string is highlighted lazily (see `lazy-highlight-initial-delay'
-and `lazy-highlight-interval')."
- :type 'boolean
+and `lazy-highlight-interval').
+
+When multiple windows display the current buffer, the
+highlighting is displayed only on the selected window, unless
+this variable is set to the symbol `all-windows'."
+ :type '(choice boolean
+ (const :tag "On, and applied to all windows" all-windows))
:group 'lazy-highlight
:group 'isearch)
@@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
;; but lower than isearch main overlay's 1001
(overlay-put ov 'priority 1000)
(overlay-put ov 'face lazy-highlight-face)
- (overlay-put ov 'window (selected-window))))
+ (unless (eq isearch-lazy-highlight 'all-windows)
+ (overlay-put ov 'window (selected-window)))))
;; Remember the current position of point for
;; the next call of `isearch-lazy-highlight-update'
;; when `lazy-highlight-max-at-a-time' is too small.
--
2.6.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 14:18 ` Artur Malabarba
@ 2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 15:44 ` Alan Mackenzie
` (2 subsequent siblings)
3 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 15:44 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, emacs-devel, Juri Linkov
Hello again, Artur
On Mon, Nov 02, 2015 at 02:18:59PM +0000, Artur Malabarba wrote:
> > > Lazy highlighting searches for matches on the current window. It must
> > > be extended to search for matches on the Follow Mode group of windows.
> > > For that, it needs the details of the "window*-start" and "window*-end".
> > Yes, you're right, I see that now. This might still be solvable via
> > setting isearch-search-fun-function, I'll need to think on that for a
> > moment.
> Here's a new version, split into 3 short patches to make it easier to
> understand.
Thanks. I understand what you're suggesting, now, and I think it's a
good idea, on the whole.
However, about the bug where, with lazy highlighting active, isearch
scrolls the window its on rather than moving to the next one: as I said,
this is caused by the "(sit-for 0)" near the beginning of
isearch-lazy-highlight-new-loop. The purpose of this is to scroll the
display, in case point has moved off of the window.
The bug cause is now clear: ..-lazy-highlight-new-loop first scrolls the
window, then checks whether it needs to restart its lazy h. loop. If it
does, it starts the timer which triggers ..-lazy-highlight-update after
the delay.
The bug is that all this checking happens in ...-lazy-h-new-loop rather
than in ...-lazy-h-update. If we moved all the checking to
..l-h-update, then there would be no need for "(sit-for 0)" - the
command loop redisplay would already have scrolled the windows, if
necessary; and Follow Mode would already have shifted point to the right
window.
So how about us just moving all these checks to where they really
belong, in isearch-lazy-highlight-update? I've a feeling that if we do
this, then your function follow--search-function becomes unneeded.
Juri?
#########################################################################
A related point in one of your patches below. It explicitly selects one
of the Follow Mode windows. This is bad. Follow Mode itself choses the
right window in its post-command-hook. Second-guessing it within the
command processing is liable to lead to confusion, maintenance
difficulties, and general pain.
Another point. It is better to use follow-all-followers to get at the
windows rather than get-buffer-window-list. The first of these is
guaranteed to get the windows in the right order, and also allows for
a future enhancement whereby only some of the windows displaying
<buffer> are part of the Follow Mode group. Of course, anything using
follow-all-followers needs to be textually in follow.el.
> This makes 2 small changes to isearch (which I think are even
> justifiable on their own, regardless of follow-mode), and defines a
> new function in follow-mode which takes care of two things:
> Matches are highlighted on all windows
Here, the matches should be highlighted on all the windows in the FM
group without exception. I think a new option you're providing allows
lazy highlighting to happen only in the current window. This is a bad
idea when FM is active.
> When the user types something, and the next match is on another
> window, switch to that window instead of scrolling the current one.
> From caf5adfc0b7caf07dc74813242f9ecc664babc13 Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 14:01:18 +0000
> Subject: [PATCH 3/3] * lisp/follow.el: Improve isearch compatibility
>
> (follow--search-function): New function.
> (follow-mode): Configure `isearch-lazy-highlight' and
> `isearch-search-fun-function'.
> ---
> lisp/follow.el | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..0b80f04 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -419,6 +419,9 @@ follow-mode
> :keymap follow-mode-map
> (if follow-mode
> (progn
> + (setq-local isearch-search-fun-function #'follow--search-function)
> + (when isearch-lazy-highlight
> + (setq-local isearch-lazy-highlight 'all-windows))
> (add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
> (add-hook 'post-command-hook 'follow-post-command-hook t)
> (add-hook 'window-size-change-functions 'follow-window-size-change t))
> @@ -434,6 +437,36 @@ follow-mode
> (remove-hook 'window-size-change-functions 'follow-window-size-change)))
> (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
>
> +(defun follow--search-function ()
> + "Return a function suitable for `isearch-search-fun-function'."
> + (lambda (string &optional bound noerror count)
> + (let* ((search-function (isearch-search-fun-default))
> + (all-followers (follow-all-followers))
> + ;; If bound at the edge of the window, extend it to the
> + ;; edges know by `follow-mode'.
> + (bound (cond ((equal bound (window-end))
> + (window-end (car (last all-followers))))
> + ((equal bound (window-start))
> + (window-start (car all-followers)))
> + (t bound)))
> + (matched (funcall search-function string bound noerror count)))
> + ;; If this is a proper user-triggered search (and not just a
> + ;; lazy-highlight search), and if the search matched, and if the
> + ;; match is not visible on this window...
> + (when (and matched
> + (not isearch-lazy-highlight-ongoing-search)
> + (not (and (pos-visible-in-window-p (match-beginning 0))
> + (pos-visible-in-window-p (match-end 0)))))
> + ;; ... see if the match is visible on another window.
> + (let ((win (seq-find (lambda (w)
> + (and (pos-visible-in-window-p (match-beginning 0) w)
> + (pos-visible-in-window-p (match-end 0) w)))
> + all-followers)))
> + ;; If so, select it.
> + (when win
> + (select-window win)))) <===========================
> + matched)))
> +
> (defun follow-find-file-hook ()
> "Find-file hook for Follow mode. See the variable `follow-auto'."
> (if follow-auto (follow-mode 1)))
> --
> 2.6.2
>
> From 8a52f12872d2b99acb2ca5a2077ccd87779309fe Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 13:54:15 +0000
> Subject: [PATCH 2/3] * lisp/isearch.el: Bind a variable while
> lazy-highlighting
>
> (isearch-lazy-highlight-ongoing-search): New variable.
> (isearch-lazy-highlight-search): Use it.
> ---
> lisp/isearch.el | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 6442166..ef49650 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3080,11 +3080,18 @@ isearch-lazy-highlight-new-loop
> (run-with-idle-timer lazy-highlight-initial-delay nil
> 'isearch-lazy-highlight-update)))))
>
> +(defvar isearch-lazy-highlight-ongoing-search nil
> + "Dynamically bound to t in `isearch-lazy-highlight-search'.
> +This can be used by `isearch-search-fun-function' to detect
> +whether its return value is being run for a proper user search or
> +a lazy highlight search.")
> +
> (defun isearch-lazy-highlight-search ()
> "Search ahead for the next or previous match, for lazy highlighting.
> Attempt to do the search exactly the way the pending Isearch would."
> (condition-case nil
> (let ((case-fold-search isearch-lazy-highlight-case-fold-search)
> + (isearch-lazy-highlight-ongoing-search t)
> (isearch-regexp isearch-lazy-highlight-regexp)
> (isearch-regexp-function isearch-lazy-highlight-regexp-function)
> (isearch-lax-whitespace
> --
> 2.6.2
>
> From 0c815fa21001b4a29f124e8eda58f7889560d701 Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 12:09:16 +0000
> Subject: [PATCH 1/3] * lisp/isearch.el: Allow lazy-highlighting of all windows
>
> (isearch-lazy-highlight): New possible value.
> (isearch-lazy-highlight-update): Use it.
> ---
> lisp/isearch.el | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..6442166 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -279,8 +279,13 @@ isearch-lazy-highlight
> "Controls the lazy-highlighting during incremental search.
> When non-nil, all text in the buffer matching the current search
> string is highlighted lazily (see `lazy-highlight-initial-delay'
> -and `lazy-highlight-interval')."
> - :type 'boolean
> +and `lazy-highlight-interval').
> +
> +When multiple windows display the current buffer, the
> +highlighting is displayed only on the selected window, unless
> +this variable is set to the symbol `all-windows'."
> + :type '(choice boolean
> + (const :tag "On, and applied to all windows" all-windows))
> :group 'lazy-highlight
> :group 'isearch)
>
> @@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
> ;; but lower than isearch main overlay's 1001
> (overlay-put ov 'priority 1000)
> (overlay-put ov 'face lazy-highlight-face)
> - (overlay-put ov 'window (selected-window))))
> + (unless (eq isearch-lazy-highlight 'all-windows)
> + (overlay-put ov 'window (selected-window)))))
> ;; Remember the current position of point for
> ;; the next call of `isearch-lazy-highlight-update'
> ;; when `lazy-highlight-max-at-a-time' is too small.
> --
> 2.6.2
>
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 14:18 ` Artur Malabarba
2015-11-02 15:44 ` Alan Mackenzie
@ 2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 16:26 ` Artur Malabarba
` (3 more replies)
2015-11-02 23:28 ` Juri Linkov
2015-11-02 23:28 ` Juri Linkov
3 siblings, 4 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 15:44 UTC (permalink / raw
To: Artur Malabarba; +Cc: Juri Linkov, 17453, emacs-devel
Hello again, Artur
On Mon, Nov 02, 2015 at 02:18:59PM +0000, Artur Malabarba wrote:
> > > Lazy highlighting searches for matches on the current window. It must
> > > be extended to search for matches on the Follow Mode group of windows.
> > > For that, it needs the details of the "window*-start" and "window*-end".
> > Yes, you're right, I see that now. This might still be solvable via
> > setting isearch-search-fun-function, I'll need to think on that for a
> > moment.
> Here's a new version, split into 3 short patches to make it easier to
> understand.
Thanks. I understand what you're suggesting, now, and I think it's a
good idea, on the whole.
However, about the bug where, with lazy highlighting active, isearch
scrolls the window its on rather than moving to the next one: as I said,
this is caused by the "(sit-for 0)" near the beginning of
isearch-lazy-highlight-new-loop. The purpose of this is to scroll the
display, in case point has moved off of the window.
The bug cause is now clear: ..-lazy-highlight-new-loop first scrolls the
window, then checks whether it needs to restart its lazy h. loop. If it
does, it starts the timer which triggers ..-lazy-highlight-update after
the delay.
The bug is that all this checking happens in ...-lazy-h-new-loop rather
than in ...-lazy-h-update. If we moved all the checking to
..l-h-update, then there would be no need for "(sit-for 0)" - the
command loop redisplay would already have scrolled the windows, if
necessary; and Follow Mode would already have shifted point to the right
window.
So how about us just moving all these checks to where they really
belong, in isearch-lazy-highlight-update? I've a feeling that if we do
this, then your function follow--search-function becomes unneeded.
Juri?
#########################################################################
A related point in one of your patches below. It explicitly selects one
of the Follow Mode windows. This is bad. Follow Mode itself choses the
right window in its post-command-hook. Second-guessing it within the
command processing is liable to lead to confusion, maintenance
difficulties, and general pain.
Another point. It is better to use follow-all-followers to get at the
windows rather than get-buffer-window-list. The first of these is
guaranteed to get the windows in the right order, and also allows for
a future enhancement whereby only some of the windows displaying
<buffer> are part of the Follow Mode group. Of course, anything using
follow-all-followers needs to be textually in follow.el.
> This makes 2 small changes to isearch (which I think are even
> justifiable on their own, regardless of follow-mode), and defines a
> new function in follow-mode which takes care of two things:
> Matches are highlighted on all windows
Here, the matches should be highlighted on all the windows in the FM
group without exception. I think a new option you're providing allows
lazy highlighting to happen only in the current window. This is a bad
idea when FM is active.
> When the user types something, and the next match is on another
> window, switch to that window instead of scrolling the current one.
> From caf5adfc0b7caf07dc74813242f9ecc664babc13 Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 14:01:18 +0000
> Subject: [PATCH 3/3] * lisp/follow.el: Improve isearch compatibility
>
> (follow--search-function): New function.
> (follow-mode): Configure `isearch-lazy-highlight' and
> `isearch-search-fun-function'.
> ---
> lisp/follow.el | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..0b80f04 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -419,6 +419,9 @@ follow-mode
> :keymap follow-mode-map
> (if follow-mode
> (progn
> + (setq-local isearch-search-fun-function #'follow--search-function)
> + (when isearch-lazy-highlight
> + (setq-local isearch-lazy-highlight 'all-windows))
> (add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
> (add-hook 'post-command-hook 'follow-post-command-hook t)
> (add-hook 'window-size-change-functions 'follow-window-size-change t))
> @@ -434,6 +437,36 @@ follow-mode
> (remove-hook 'window-size-change-functions 'follow-window-size-change)))
> (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
>
> +(defun follow--search-function ()
> + "Return a function suitable for `isearch-search-fun-function'."
> + (lambda (string &optional bound noerror count)
> + (let* ((search-function (isearch-search-fun-default))
> + (all-followers (follow-all-followers))
> + ;; If bound at the edge of the window, extend it to the
> + ;; edges know by `follow-mode'.
> + (bound (cond ((equal bound (window-end))
> + (window-end (car (last all-followers))))
> + ((equal bound (window-start))
> + (window-start (car all-followers)))
> + (t bound)))
> + (matched (funcall search-function string bound noerror count)))
> + ;; If this is a proper user-triggered search (and not just a
> + ;; lazy-highlight search), and if the search matched, and if the
> + ;; match is not visible on this window...
> + (when (and matched
> + (not isearch-lazy-highlight-ongoing-search)
> + (not (and (pos-visible-in-window-p (match-beginning 0))
> + (pos-visible-in-window-p (match-end 0)))))
> + ;; ... see if the match is visible on another window.
> + (let ((win (seq-find (lambda (w)
> + (and (pos-visible-in-window-p (match-beginning 0) w)
> + (pos-visible-in-window-p (match-end 0) w)))
> + all-followers)))
> + ;; If so, select it.
> + (when win
> + (select-window win)))) <===========================
> + matched)))
> +
> (defun follow-find-file-hook ()
> "Find-file hook for Follow mode. See the variable `follow-auto'."
> (if follow-auto (follow-mode 1)))
> --
> 2.6.2
>
> From 8a52f12872d2b99acb2ca5a2077ccd87779309fe Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 13:54:15 +0000
> Subject: [PATCH 2/3] * lisp/isearch.el: Bind a variable while
> lazy-highlighting
>
> (isearch-lazy-highlight-ongoing-search): New variable.
> (isearch-lazy-highlight-search): Use it.
> ---
> lisp/isearch.el | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 6442166..ef49650 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3080,11 +3080,18 @@ isearch-lazy-highlight-new-loop
> (run-with-idle-timer lazy-highlight-initial-delay nil
> 'isearch-lazy-highlight-update)))))
>
> +(defvar isearch-lazy-highlight-ongoing-search nil
> + "Dynamically bound to t in `isearch-lazy-highlight-search'.
> +This can be used by `isearch-search-fun-function' to detect
> +whether its return value is being run for a proper user search or
> +a lazy highlight search.")
> +
> (defun isearch-lazy-highlight-search ()
> "Search ahead for the next or previous match, for lazy highlighting.
> Attempt to do the search exactly the way the pending Isearch would."
> (condition-case nil
> (let ((case-fold-search isearch-lazy-highlight-case-fold-search)
> + (isearch-lazy-highlight-ongoing-search t)
> (isearch-regexp isearch-lazy-highlight-regexp)
> (isearch-regexp-function isearch-lazy-highlight-regexp-function)
> (isearch-lax-whitespace
> --
> 2.6.2
>
> From 0c815fa21001b4a29f124e8eda58f7889560d701 Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 12:09:16 +0000
> Subject: [PATCH 1/3] * lisp/isearch.el: Allow lazy-highlighting of all windows
>
> (isearch-lazy-highlight): New possible value.
> (isearch-lazy-highlight-update): Use it.
> ---
> lisp/isearch.el | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..6442166 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -279,8 +279,13 @@ isearch-lazy-highlight
> "Controls the lazy-highlighting during incremental search.
> When non-nil, all text in the buffer matching the current search
> string is highlighted lazily (see `lazy-highlight-initial-delay'
> -and `lazy-highlight-interval')."
> - :type 'boolean
> +and `lazy-highlight-interval').
> +
> +When multiple windows display the current buffer, the
> +highlighting is displayed only on the selected window, unless
> +this variable is set to the symbol `all-windows'."
> + :type '(choice boolean
> + (const :tag "On, and applied to all windows" all-windows))
> :group 'lazy-highlight
> :group 'isearch)
>
> @@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
> ;; but lower than isearch main overlay's 1001
> (overlay-put ov 'priority 1000)
> (overlay-put ov 'face lazy-highlight-face)
> - (overlay-put ov 'window (selected-window))))
> + (unless (eq isearch-lazy-highlight 'all-windows)
> + (overlay-put ov 'window (selected-window)))))
> ;; Remember the current position of point for
> ;; the next call of `isearch-lazy-highlight-update'
> ;; when `lazy-highlight-max-at-a-time' is too small.
> --
> 2.6.2
>
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 12:35 ` Alan Mackenzie
2015-11-02 13:10 ` Artur Malabarba
2015-11-02 13:10 ` Artur Malabarba
@ 2015-11-02 15:46 ` Eli Zaretskii
2015-11-02 16:09 ` Alan Mackenzie
[not found] ` <<831tc8xv39.fsf@gnu.org>
3 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-02 15:46 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, monnier, bruce.connor.am, juri
Lets' take emacs-devel out of this shall we?
> Date: Mon, 2 Nov 2015 12:35:12 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: Juri Linkov <juri@linkov.net>, 17453@debbugs.gnu.org,
> Stefan Monnier <monnier@iro.umontreal.ca>,
> emacs-devel <emacs-devel@gnu.org>
>
> > Maybe I missed part of the issue. I thought you wanted Isearch to
> > switch to another window if that window contains the next match
> > (instead of scrolling the current window). For that, you only need
> > pos-visible-in-window-p, you don't need to mess with boundaries.
>
> What is causing the unwanted scrolling rather than moving to the next
> window, is the form "(sit-for 0)" near the start of
> isearch-lazy-highlight-new-loop. When point is outside the window, this
> form causes redisplay, which scrolls point back into the window -
> without Follow Mode getting a look in. In my patch, I replaced this
> with "(sit*-for 0)", where Follow Mode can do its thing before the
> redisplay happens.
If this means that sit*-for does something other than redisplay and
wait, like switch to another window, I'd really suggest to rethink
that. It is entirely counter-intuitive to have a sit-for family of
functions do anything other than some kind of redisplay and some kind
of waiting.
I think it is a better idea to have Isearch switch to another window
when the next hit is there, via some specialized movement command that
Follow mode could customize. That'd be something expectable.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
[not found] ` <<831tc8xv39.fsf@gnu.org>
@ 2015-11-02 16:05 ` Drew Adams
0 siblings, 0 replies; 96+ messages in thread
From: Drew Adams @ 2015-11-02 16:05 UTC (permalink / raw
To: Eli Zaretskii, Alan Mackenzie; +Cc: 17453, monnier, bruce.connor.am, juri
> Lets' take emacs-devel out of this shall we?
Enfin !
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 15:46 ` Eli Zaretskii
@ 2015-11-02 16:09 ` Alan Mackenzie
2015-11-02 17:49 ` Eli Zaretskii
0 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 16:09 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 17453, monnier, bruce.connor.am, juri
Hello, Eli.
On Mon, Nov 02, 2015 at 05:46:34PM +0200, Eli Zaretskii wrote:
> Lets' take emacs-devel out of this shall we?
OK.
> > Date: Mon, 2 Nov 2015 12:35:12 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: Juri Linkov <juri@linkov.net>, 17453@debbugs.gnu.org,
> > Stefan Monnier <monnier@iro.umontreal.ca>,
> > emacs-devel <emacs-devel@gnu.org>
> > > Maybe I missed part of the issue. I thought you wanted Isearch to
> > > switch to another window if that window contains the next match
> > > (instead of scrolling the current window). For that, you only need
> > > pos-visible-in-window-p, you don't need to mess with boundaries.
> > What is causing the unwanted scrolling rather than moving to the next
> > window, is the form "(sit-for 0)" near the start of
> > isearch-lazy-highlight-new-loop. When point is outside the window, this
> > form causes redisplay, which scrolls point back into the window -
> > without Follow Mode getting a look in. In my patch, I replaced this
> > with "(sit*-for 0)", where Follow Mode can do its thing before the
> > redisplay happens.
> If this means that sit*-for does something other than redisplay and
> wait, like switch to another window, I'd really suggest to rethink
> that. It is entirely counter-intuitive to have a sit-for family of
> functions do anything other than some kind of redisplay and some kind
> of waiting.
sit*-for's synchronising windows, switching to the appropriate window,
etc., is conceptually an extension of redisplay's scrolling to get point
on screen. But I think sit*-for could well be not needed here, anyway.
> I think it is a better idea to have Isearch switch to another window
> when the next hit is there, via some specialized movement command that
> Follow mode could customize. That'd be something expectable.
No. The right fix is not to do "(sit-for 0)" at that point in the
processing. As I explained at length to Artur in another post today, if
we move lazy-highlight's checking whether window-start/end have changed,
into the function triggered by the timer, redisplay will already have
happened and so we won't need that offending sit-for.
Then Follow Mode will quite naturally select the next window (via its
post-command-hook), rather than isearch forcibly scrolling the first
one. As currently happens when lazy highlighting is disabled.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 15:44 ` Alan Mackenzie
@ 2015-11-02 16:26 ` Artur Malabarba
2015-11-02 16:35 ` Drew Adams
2015-11-02 22:09 ` Alan Mackenzie
2015-11-02 17:45 ` Eli Zaretskii
` (2 subsequent siblings)
3 siblings, 2 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 16:26 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Juri Linkov
Hi Alan,
2015-11-02 15:44 GMT+00:00 Alan Mackenzie <acm@muc.de>:
> So how about us just moving all these checks to where they really
> belong, in isearch-lazy-highlight-update? I've a feeling that if we do
> this, then your function follow--search-function becomes unneeded.
Yes, I think this might work too. And I like the idea of eliminating a
redisplay inside the command-loop.
Could you test it?
You'll probably still need a simpler version of that
follow--search-function to ensure that lazy-highlighting extends to
all windows (instead of stopping inside the selected-window), but at
least it won't need that `select-window' part that you said is bad.
:-)
>> Matches are highlighted on all windows
>
> Here, the matches should be highlighted on all the windows in the FM
> group without exception. I think a new option you're providing allows
> lazy highlighting to happen only in the current window. This is a bad
> idea when FM is active.
No, currently lazy highlighting to happens only in the current window
(with or without follow-mode). This happens for 2 reasons. (1) Isearch
only highlights between window-start and window-end, and (2) isearch
limits the highlithing overlays to only be displayed on the selected
window.
The new option I introduced to isearch allows the highlighting
overlays to be displayed on all windows (solving (2)). Furthermore,
one of features of that follow--search-function function is to make
sure that highlighting doesn't stop `window-end' (solving (1)).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 16:26 ` Artur Malabarba
@ 2015-11-02 16:35 ` Drew Adams
2015-11-02 19:18 ` Artur Malabarba
2015-11-02 22:09 ` Alan Mackenzie
1 sibling, 1 reply; 96+ messages in thread
From: Drew Adams @ 2015-11-02 16:35 UTC (permalink / raw
To: bruce.connor.am, Alan Mackenzie; +Cc: 17453, Juri Linkov
> one of features of that follow--search-function function is to make
> sure that highlighting doesn't stop `window-end' (solving (1)).
I believe this was already taken care of by work that Juri did recently.
I don't have time to look this up now, but see recent bug reports.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 16:26 ` Artur Malabarba
@ 2015-11-02 17:45 ` Eli Zaretskii
2015-11-02 23:22 ` Juri Linkov
2015-11-02 23:22 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-02 17:45 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, bruce.connor.am, juri
> Date: Mon, 2 Nov 2015 15:44:45 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: Juri Linkov <juri@linkov.net>, 17453@debbugs.gnu.org,
> emacs-devel <emacs-devel@gnu.org>
>
> A related point in one of your patches below. It explicitly selects one
> of the Follow Mode windows. This is bad. Follow Mode itself choses the
> right window in its post-command-hook. Second-guessing it within the
> command processing is liable to lead to confusion, maintenance
> difficulties, and general pain.
You could call the same code that post-command-hook calls, then it
wouldn't be second-guessing.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 16:09 ` Alan Mackenzie
@ 2015-11-02 17:49 ` Eli Zaretskii
2015-11-02 20:35 ` John Wiegley
2015-11-03 8:35 ` Alan Mackenzie
0 siblings, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-02 17:49 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, monnier, bruce.connor.am, juri
> Date: Mon, 2 Nov 2015 16:09:17 +0000
> Cc: bruce.connor.am@gmail.com, juri@linkov.net, 17453@debbugs.gnu.org,
> monnier@iro.umontreal.ca
> From: Alan Mackenzie <acm@muc.de>
>
> > If this means that sit*-for does something other than redisplay and
> > wait, like switch to another window, I'd really suggest to rethink
> > that. It is entirely counter-intuitive to have a sit-for family of
> > functions do anything other than some kind of redisplay and some kind
> > of waiting.
>
> sit*-for's synchronising windows, switching to the appropriate window,
> etc., is conceptually an extension of redisplay's scrolling to get point
> on screen.
No, redisplay doesn't necessarily scroll. It does so only if needed.
So sit-for should not be thought as a way to scroll the window, even
if sometimes it does.
> But I think sit*-for could well be not needed here, anyway.
That's okay, but my point is: please do not add to established
functions code that does on behalf of Follow mode something that
conceptually doesn't belong to those functions. OK?
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 16:35 ` Drew Adams
@ 2015-11-02 19:18 ` Artur Malabarba
2015-11-02 19:28 ` Drew Adams
0 siblings, 1 reply; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 19:18 UTC (permalink / raw
To: Drew Adams; +Cc: Alan Mackenzie, 17453, Juri Linkov
2015-11-02 16:35 GMT+00:00 Drew Adams <drew.adams@oracle.com>:
>> one of features of that follow--search-function function is to make
>> sure that highlighting doesn't stop `window-end' (solving (1)).
>
> I believe this was already taken care of by work that Juri did recently.
> I don't have time to look this up now, but see recent bug reports.
I ran a git log on isearch.el and I think you're referring to this change:
10ec0468dfbc0815a772cc46a031aca298af0985 Lazy-highlight the whole
string at point (debbugs:19353)
If so, that's just about extending the bound a little bit to ensure it
holds a whole symbol. follow-mode actually needs to extend the bound a
lot more (up 'till the `window-end' of the last window in the group).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 19:18 ` Artur Malabarba
@ 2015-11-02 19:28 ` Drew Adams
2015-11-02 23:45 ` Juri Linkov
0 siblings, 1 reply; 96+ messages in thread
From: Drew Adams @ 2015-11-02 19:28 UTC (permalink / raw
To: bruce.connor.am; +Cc: Alan Mackenzie, 17453, Juri Linkov
> > I believe this was already taken care of by work that Juri did recently.
> > I don't have time to look this up now, but see recent bug reports.
>
> I ran a git log on isearch.el and I think you're referring to this change:
>
> 10ec0468dfbc0815a772cc46a031aca298af0985 Lazy-highlight the whole
> string at point (debbugs:19353)
>
> If so, that's just about extending the bound a little bit to ensure it
> holds a whole symbol. follow-mode actually needs to extend the bound a
> lot more (up 'till the `window-end' of the last window in the group).
Please check with Juri. I'm pretty sure the discussion included
the possibility of having lazy highlighting throughout a buffer.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 17:49 ` Eli Zaretskii
@ 2015-11-02 20:35 ` John Wiegley
2015-11-03 8:35 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: John Wiegley @ 2015-11-02 20:35 UTC (permalink / raw
To: Eli Zaretskii; +Cc: Alan Mackenzie, 17453, monnier, bruce.connor.am, juri
>>>>> Eli Zaretskii <eliz@gnu.org> writes:
> That's okay, but my point is: please do not add to established functions
> code that does on behalf of Follow mode something that conceptually doesn't
> belong to those functions. OK?
I'd like to second this request.
John
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 16:26 ` Artur Malabarba
2015-11-02 16:35 ` Drew Adams
@ 2015-11-02 22:09 ` Alan Mackenzie
2015-11-02 23:00 ` Artur Malabarba
1 sibling, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-02 22:09 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, Juri Linkov
Hello again, Artur.
On Mon, Nov 02, 2015 at 04:26:23PM +0000, Artur Malabarba wrote:
> Hi Alan,
> 2015-11-02 15:44 GMT+00:00 Alan Mackenzie <acm@muc.de>:
> > So how about us just moving all these checks to where they really
> > belong, in isearch-lazy-highlight-update? I've a feeling that if we do
> > this, then your function follow--search-function becomes unneeded.
> Yes, I think this might work too. And I like the idea of eliminating a
> redisplay inside the command-loop.
> Could you test it?
Of course, nothing is ever that simple. ;-). I've got a working
version which doesn't use "(sit-for 0)", thus allowing point to move
properly over Follow Mode windows.
However, this meant the "old" highlighting not being erased until the
0.25 seconds had passed. So I tried testing the conditions for a new
lazy-highlight loop (apart from window-start/end changing) in the
command loop bit. This doesn't work brilliantly well when a C-s causes
a half-screen forward scroll: the "upper" lazy highlighting remains
there, and 0.25s later the "lower" lazy H. appears.
It struck me that instead of all this rigmarole, and instead of the
"(sit-for 0)", we could test whether or not a scroll is about to take
place. The obvious candidate for this is `pos-visible-in-window-p' - if
this returns nil, obviously a scroll is about to happen. Unfortunately,
even if p-v-i-w-p returns t, a scroll might still happen because of
`scroll-margin', and the like.
What we could do with is an interface which will tell us whether or not
redisplay would scroll if invoked immediately, with the current value of
point, window-start, etc. Obviously it would be simple, but messy to
hack a lisp function together to do this, but a more general purpose
function might be apposite here. Eli, what do you think?
All this is, just for the moment, disregarding any enhancements needed
to support Follow Mode properly. Just for the moment, that is.
If you're interested, here is a (still scrappy) patch of where I am at
the moment.
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..b59a224 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3012,13 +3012,52 @@ 'isearch-lazy-highlight-cleanup
"22.1")
(defun isearch-lazy-highlight-new-loop (&optional beg end)
- "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
-by other Emacs features."
+ "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."
+ (setq isearch-lazy-highlight-start-limit beg
+ isearch-lazy-highlight-end-limit end)
+ (when (null executing-kbd-macro)
+ (when (or (not (equal isearch-string
+ isearch-lazy-highlight-last-string))
+ (not (eq (selected-window)
+ isearch-lazy-highlight-window))
+ (not (eq isearch-lazy-highlight-case-fold-search
+ isearch-case-fold-search))
+ (not (eq isearch-lazy-highlight-regexp
+ isearch-regexp))
+ (not (eq isearch-lazy-highlight-regexp-function
+ isearch-regexp-function))
+ (not (eq isearch-lazy-highlight-lax-whitespace
+ isearch-lax-whitespace))
+ (not (eq isearch-lazy-highlight-regexp-lax-whitespace
+ isearch-regexp-lax-whitespace))
+ ;; (not (= (window-start)
+ ;; isearch-lazy-highlight-window-start))
+ ;; (not (= (window-end) ; Window may have been split/joined.
+ ;; isearch-lazy-highlight-window-end))
+ (not (eq isearch-forward
+ isearch-lazy-highlight-forward))
+ ;; In case we are recovering from an error.
+ (not (equal isearch-error
+ isearch-lazy-highlight-error)))
+ (lazy-highlight-cleanup t)) ;kill old loop & remove overlays.
+ (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 the previous `lazy-highlight' loop and begin a new one.
+Return t if we begin a loop, nil otherwise."
+;; " 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
+;; by other Emacs features."
(when (and (null executing-kbd-macro)
- (sit-for 0) ;make sure (window-start) is credible
+ ;; (sit-for 0) ;make sure (window-start) is credible
(or (not (equal isearch-string
isearch-lazy-highlight-last-string))
(not (eq (selected-window)
@@ -3048,8 +3087,8 @@ isearch-lazy-highlight-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-start-limit beg
+ ;; isearch-lazy-highlight-end-limit end)
(setq isearch-lazy-highlight-window (selected-window)
isearch-lazy-highlight-window-start (window-start)
isearch-lazy-highlight-window-end (window-end)
@@ -3070,10 +3109,12 @@ isearch-lazy-highlight-new-loop
isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace
isearch-lazy-highlight-regexp-function isearch-regexp-function
isearch-lazy-highlight-forward isearch-forward)
- (unless (equal isearch-string "")
- (setq isearch-lazy-highlight-timer
- (run-with-idle-timer lazy-highlight-initial-delay nil
- 'isearch-lazy-highlight-update)))))
+ ;; (unless (equal isearch-string "")
+ ;; (setq isearch-lazy-highlight-timer
+ ;; (run-with-idle-timer lazy-highlight-initial-delay nil
+ ;; 'isearch-lazy-highlight-update)))
+ (unless (equal isearch-string "")
+ (isearch-lazy-highlight-update))))
(defun isearch-lazy-highlight-search ()
"Search ahead for the next or previous match, for lazy highlighting.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 22:09 ` Alan Mackenzie
@ 2015-11-02 23:00 ` Artur Malabarba
2015-11-03 9:18 ` Alan Mackenzie
0 siblings, 1 reply; 96+ messages in thread
From: Artur Malabarba @ 2015-11-02 23:00 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Juri Linkov
[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]
On 2 Nov 2015 10:09 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> Of course, nothing is ever that simple. ;-). I've got a working
> version which doesn't use "(sit-for 0)", thus allowing point to move
> properly over Follow Mode windows.
>
> However, this meant the "old" highlighting not being erased until the
> 0.25 seconds had passed. So I tried testing the conditions for a new
> lazy-highlight loop (apart from window-start/end changing) in the
> command loop bit.
> It struck me that instead of all this rigmarole, and instead of the
> "(sit-for 0)", we could test whether or not a scroll is about to take
> place.
Or we can cleanup the lazy highlighting _when_ scrolling takes place.
> What we could do with is an interface which will tell us whether or not
> redisplay would scroll if invoked immediately, with the current value of
> point, window-start, etc.
I don't think that's necessary. What if isearch just added
(lazy-highlight-cleanup t) to window-scroll-functions?
I want to be very cautious about not increasing the complexity of isearch
code here, but so far these sound like good refactorings to me.
[-- Attachment #2: Type: text/html, Size: 1425 bytes --]
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 16:26 ` Artur Malabarba
2015-11-02 17:45 ` Eli Zaretskii
@ 2015-11-02 23:22 ` Juri Linkov
2015-11-02 23:22 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:22 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Artur Malabarba, emacs-devel
> So how about us just moving all these checks to where they really
> belong, in isearch-lazy-highlight-update? I've a feeling that if we do
> this, then your function follow--search-function becomes unneeded.
>
> Juri?
Right, without (sit-for 0) it's possible to switch focus to adjacent windows
with just adding 2 lines to follow.el, i.e. I get the desired behavior with:
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..0433854 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -420,6 +420,7 @@ (define-minor-mode follow-mode
(if follow-mode
(progn
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
+ (add-hook 'isearch-update-post-hook 'follow-align-compilation-windows t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
;; Remove globally-installed hook functions only if there is no
@@ -432,6 +433,7 @@ (define-minor-mode follow-mode
(unless following
(remove-hook 'post-command-hook 'follow-post-command-hook)
(remove-hook 'window-size-change-functions 'follow-window-size-change)))
+ (remove-hook 'isearch-update-post-hook 'follow-align-compilation-windows t)
(remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
(defun follow-find-file-hook ()
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..8edf8b0 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3018,7 +3018,7 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
search string to change or the window to scroll). It is also used
by other Emacs features."
(when (and (null executing-kbd-macro)
- (sit-for 0) ;make sure (window-start) is credible
+ ; (sit-for 0) ;make sure (window-start) is credible
(or (not (equal isearch-string
isearch-lazy-highlight-last-string))
(not (eq (selected-window)
So what remains to do is to fix this bug, but I don't understand the logic
you proposed: how checks could be moved to isearch-lazy-highlight-update
if isearch-lazy-highlight-update is scheduled by a timer conditionally
depending on these checks?
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 15:44 ` Alan Mackenzie
` (2 preceding siblings ...)
2015-11-02 23:22 ` Juri Linkov
@ 2015-11-02 23:22 ` Juri Linkov
2015-11-03 12:31 ` Alan Mackenzie
2015-11-03 12:31 ` Alan Mackenzie
3 siblings, 2 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:22 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Artur Malabarba, emacs-devel
> So how about us just moving all these checks to where they really
> belong, in isearch-lazy-highlight-update? I've a feeling that if we do
> this, then your function follow--search-function becomes unneeded.
>
> Juri?
Right, without (sit-for 0) it's possible to switch focus to adjacent windows
with just adding 2 lines to follow.el, i.e. I get the desired behavior with:
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..0433854 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -420,6 +420,7 @@ (define-minor-mode follow-mode
(if follow-mode
(progn
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
+ (add-hook 'isearch-update-post-hook 'follow-align-compilation-windows t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
;; Remove globally-installed hook functions only if there is no
@@ -432,6 +433,7 @@ (define-minor-mode follow-mode
(unless following
(remove-hook 'post-command-hook 'follow-post-command-hook)
(remove-hook 'window-size-change-functions 'follow-window-size-change)))
+ (remove-hook 'isearch-update-post-hook 'follow-align-compilation-windows t)
(remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
(defun follow-find-file-hook ()
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..8edf8b0 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3018,7 +3018,7 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
search string to change or the window to scroll). It is also used
by other Emacs features."
(when (and (null executing-kbd-macro)
- (sit-for 0) ;make sure (window-start) is credible
+ ; (sit-for 0) ;make sure (window-start) is credible
(or (not (equal isearch-string
isearch-lazy-highlight-last-string))
(not (eq (selected-window)
So what remains to do is to fix this bug, but I don't understand the logic
you proposed: how checks could be moved to isearch-lazy-highlight-update
if isearch-lazy-highlight-update is scheduled by a timer conditionally
depending on these checks?
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 14:18 ` Artur Malabarba
` (2 preceding siblings ...)
2015-11-02 23:28 ` Juri Linkov
@ 2015-11-02 23:28 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:28 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, 17453, Stefan Monnier, emacs-devel
> +(defvar isearch-lazy-highlight-ongoing-search nil
> + "Dynamically bound to t in `isearch-lazy-highlight-search'.
> +This can be used by `isearch-search-fun-function' to detect
> +whether its return value is being run for a proper user search or
> +a lazy highlight search.")
> +
> (defun isearch-lazy-highlight-search ()
> "Search ahead for the next or previous match, for lazy highlighting.
> Attempt to do the search exactly the way the pending Isearch would."
> (condition-case nil
> (let ((case-fold-search isearch-lazy-highlight-case-fold-search)
> + (isearch-lazy-highlight-ongoing-search t)
> (isearch-regexp isearch-lazy-highlight-regexp)
> (isearch-regexp-function isearch-lazy-highlight-regexp-function)
> (isearch-lax-whitespace
Please note that this is unnecessary because all functions that override
isearch-search-fun-function use its ‘bounds’ arg to detect whether it's
called from isearch-search or isearch-lazy-highlight-search with the value
either nil or non-nil correspondingly.
> @@ -279,8 +279,13 @@ isearch-lazy-highlight
> "Controls the lazy-highlighting during incremental search.
> When non-nil, all text in the buffer matching the current search
> string is highlighted lazily (see `lazy-highlight-initial-delay'
> -and `lazy-highlight-interval')."
> - :type 'boolean
> +and `lazy-highlight-interval').
> +
> +When multiple windows display the current buffer, the
> +highlighting is displayed only on the selected window, unless
> +this variable is set to the symbol `all-windows'."
> + :type '(choice boolean
> + (const :tag "On, and applied to all windows" all-windows))
> :group 'lazy-highlight
> :group 'isearch)
>
> @@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
> ;; but lower than isearch main overlay's 1001
> (overlay-put ov 'priority 1000)
> (overlay-put ov 'face lazy-highlight-face)
> - (overlay-put ov 'window (selected-window))))
> + (unless (eq isearch-lazy-highlight 'all-windows)
> + (overlay-put ov 'window (selected-window)))))
This is a good option that follow-mode could set (or users to customize).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 14:18 ` Artur Malabarba
2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 15:44 ` Alan Mackenzie
@ 2015-11-02 23:28 ` Juri Linkov
2015-11-02 23:28 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:28 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, 17453, Stefan Monnier, emacs-devel
> +(defvar isearch-lazy-highlight-ongoing-search nil
> + "Dynamically bound to t in `isearch-lazy-highlight-search'.
> +This can be used by `isearch-search-fun-function' to detect
> +whether its return value is being run for a proper user search or
> +a lazy highlight search.")
> +
> (defun isearch-lazy-highlight-search ()
> "Search ahead for the next or previous match, for lazy highlighting.
> Attempt to do the search exactly the way the pending Isearch would."
> (condition-case nil
> (let ((case-fold-search isearch-lazy-highlight-case-fold-search)
> + (isearch-lazy-highlight-ongoing-search t)
> (isearch-regexp isearch-lazy-highlight-regexp)
> (isearch-regexp-function isearch-lazy-highlight-regexp-function)
> (isearch-lax-whitespace
Please note that this is unnecessary because all functions that override
isearch-search-fun-function use its ‘bounds’ arg to detect whether it's
called from isearch-search or isearch-lazy-highlight-search with the value
either nil or non-nil correspondingly.
> @@ -279,8 +279,13 @@ isearch-lazy-highlight
> "Controls the lazy-highlighting during incremental search.
> When non-nil, all text in the buffer matching the current search
> string is highlighted lazily (see `lazy-highlight-initial-delay'
> -and `lazy-highlight-interval')."
> - :type 'boolean
> +and `lazy-highlight-interval').
> +
> +When multiple windows display the current buffer, the
> +highlighting is displayed only on the selected window, unless
> +this variable is set to the symbol `all-windows'."
> + :type '(choice boolean
> + (const :tag "On, and applied to all windows" all-windows))
> :group 'lazy-highlight
> :group 'isearch)
>
> @@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
> ;; but lower than isearch main overlay's 1001
> (overlay-put ov 'priority 1000)
> (overlay-put ov 'face lazy-highlight-face)
> - (overlay-put ov 'window (selected-window))))
> + (unless (eq isearch-lazy-highlight 'all-windows)
> + (overlay-put ov 'window (selected-window)))))
This is a good option that follow-mode could set (or users to customize).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 9:28 ` Alan Mackenzie
2015-11-02 11:53 ` Artur Malabarba
2015-11-02 11:53 ` Artur Malabarba
@ 2015-11-02 23:33 ` Juri Linkov
2015-11-02 23:33 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:33 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>> IIUC, lazy highlighting is not a problem anymore, ....
>
> It is. With Follow Mode enabled, lazy highlighting is only occurring in
> the one window, not all of them.
Perhaps you missed my patch in the previous message
that will highlight matches in all windows after Artur will add
a new option ‘all-windows’ to ‘isearch-lazy-highlight’ to lift
the selected-window-only restriction from lazy overlays.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 9:28 ` Alan Mackenzie
` (2 preceding siblings ...)
2015-11-02 23:33 ` Juri Linkov
@ 2015-11-02 23:33 ` Juri Linkov
3 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:33 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Stefan Monnier, emacs-devel
>> IIUC, lazy highlighting is not a problem anymore, ....
>
> It is. With Follow Mode enabled, lazy highlighting is only occurring in
> the one window, not all of them.
Perhaps you missed my patch in the previous message
that will highlight matches in all windows after Artur will add
a new option ‘all-windows’ to ‘isearch-lazy-highlight’ to lift
the selected-window-only restriction from lazy overlays.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 19:28 ` Drew Adams
@ 2015-11-02 23:45 ` Juri Linkov
0 siblings, 0 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-02 23:45 UTC (permalink / raw
To: Drew Adams; +Cc: Alan Mackenzie, 17453, bruce.connor.am
>> > I believe this was already taken care of by work that Juri did recently.
>> > I don't have time to look this up now, but see recent bug reports.
>>
>> I ran a git log on isearch.el and I think you're referring to this change:
>>
>> 10ec0468dfbc0815a772cc46a031aca298af0985 Lazy-highlight the whole
>> string at point (debbugs:19353)
>>
>> If so, that's just about extending the bound a little bit to ensure it
>> holds a whole symbol. follow-mode actually needs to extend the bound a
>> lot more (up 'till the `window-end' of the last window in the group).
>
> Please check with Juri. I'm pretty sure the discussion included
> the possibility of having lazy highlighting throughout a buffer.
Yes, what I meant is:
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..fc970bb 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -419,6 +419,7 @@ (define-minor-mode follow-mode
:keymap follow-mode-map
(if follow-mode
(progn
+ (setq-local lazy-highlight-buffer t)
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 17:49 ` Eli Zaretskii
2015-11-02 20:35 ` John Wiegley
@ 2015-11-03 8:35 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-03 8:35 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 17453, bruce.connor.am, juri
On Mon, Nov 02, 2015 at 07:49:43PM +0200, Eli Zaretskii wrote:
> > Date: Mon, 2 Nov 2015 16:09:17 +0000
> > Cc: bruce.connor.am@gmail.com, juri@linkov.net, 17453@debbugs.gnu.org,
> > monnier@iro.umontreal.ca
> > From: Alan Mackenzie <acm@muc.de>
> > > If this means that sit*-for does something other than redisplay and
> > > wait, like switch to another window, I'd really suggest to rethink
> > > that. It is entirely counter-intuitive to have a sit-for family of
> > > functions do anything other than some kind of redisplay and some kind
> > > of waiting.
> > sit*-for's synchronising windows, switching to the appropriate window,
> > etc., is conceptually an extension of redisplay's scrolling to get point
> > on screen.
> No, redisplay doesn't necessarily scroll. It does so only if needed.
> So sit-for should not be thought as a way to scroll the window, even
> if sometimes it does.
Redisplay scrolls sometimes, and it moves point sometimes. But it does
those things only when needed for the actual display. Similarly the
sit*-for, invoking display would do other things strictly necessary for
successful redisplay. That sometimes would include resynchronising the
windows, for example.
> > But I think sit*-for could well be not needed here, anyway.
> That's okay, but my point is: please do not add to established
> functions code that does on behalf of Follow mode something that
> conceptually doesn't belong to those functions. OK?
Fine!
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 23:00 ` Artur Malabarba
@ 2015-11-03 9:18 ` Alan Mackenzie
0 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-03 9:18 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, Juri Linkov
Hello, Artur.
On Mon, Nov 02, 2015 at 11:00:21PM +0000, Artur Malabarba wrote:
> On 2 Nov 2015 10:09 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> > Of course, nothing is ever that simple. ;-). I've got a working
> > version which doesn't use "(sit-for 0)", thus allowing point to move
> > properly over Follow Mode windows.
> > However, this meant the "old" highlighting not being erased until the
> > 0.25 seconds had passed. So I tried testing the conditions for a new
> > lazy-highlight loop (apart from window-start/end changing) in the
> > command loop bit.
> > It struck me that instead of all this rigmarole, and instead of the
> > "(sit-for 0)", we could test whether or not a scroll is about to take
> > place.
> Or we can cleanup the lazy highlighting _when_ scrolling takes place.
> > What we could do with is an interface which will tell us whether or not
> > redisplay would scroll if invoked immediately, with the current value of
> > point, window-start, etc.
> I don't think that's necessary. What if isearch just added
> (lazy-highlight-cleanup t) to window-scroll-functions?
Now that's a good idea. :-)
So stale lazy highlighting could be erased by the multitude of existing
tests for most cases, and by that hook function for when we scroll. I'm
going to try it.
> I want to be very cautious about not increasing the complexity of isearch
> code here, but so far these sound like good refactorings to me.
I agree on both counts.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 23:22 ` Juri Linkov
2015-11-03 12:31 ` Alan Mackenzie
@ 2015-11-03 12:31 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-03 12:31 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Artur Malabarba, emacs-devel
Hello, Juri.
On Tue, Nov 03, 2015 at 01:22:03AM +0200, Juri Linkov wrote:
> > So how about us just moving all these checks to where they really
> > belong, in isearch-lazy-highlight-update? I've a feeling that if we do
> > this, then your function follow--search-function becomes unneeded.
> >
> > Juri?
> Right, without (sit-for 0) it's possible to switch focus to adjacent windows
> with just adding 2 lines to follow.el, i.e. I get the desired behavior with:
> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..0433854 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -420,6 +420,7 @@ (define-minor-mode follow-mode
> (if follow-mode
> (progn
> (add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
> + (add-hook 'isearch-update-post-hook 'follow-align-compilation-windows t t)
> (add-hook 'post-command-hook 'follow-post-command-hook t)
> (add-hook 'window-size-change-functions 'follow-window-size-change t))
> ;; Remove globally-installed hook functions only if there is no
> @@ -432,6 +433,7 @@ (define-minor-mode follow-mode
> (unless following
> (remove-hook 'post-command-hook 'follow-post-command-hook)
> (remove-hook 'window-size-change-functions 'follow-window-size-change)))
> + (remove-hook 'isearch-update-post-hook 'follow-align-compilation-windows t)
> (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
> (defun follow-find-file-hook ()
This is complicated. Ideally, the Follow Mode windows should be
synchronised in FM's post-command-hook, not isearch's. It is not
isearch's job to realign windows. follow-post-command-hook both realigns
windows and choses an appropriate window to put point in. We should let
it.
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..8edf8b0 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3018,7 +3018,7 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
> search string to change or the window to scroll). It is also used
> by other Emacs features."
> (when (and (null executing-kbd-macro)
> - (sit-for 0) ;make sure (window-start) is credible
> + ; (sit-for 0) ;make sure (window-start) is credible
> (or (not (equal isearch-string
> isearch-lazy-highlight-last-string))
> (not (eq (selected-window)
> So what remains to do is to fix this bug, but I don't understand the logic
> you proposed: how checks could be moved to isearch-lazy-highlight-update
> if isearch-lazy-highlight-update is scheduled by a timer conditionally
> depending on these checks?
What I'm proposing is to schedule the timer always, and do the checks
(for whether we need to start a new lazy highlight loop) in the function
that the timer triggers. The advantage is that when the timer triggers,
redisplay will already have taken place[*], and thus we can use
`window-start' and `window-end' without forcing an artificial redisplay
with "(sit-for 0)".
[*] and an appropriate FM window will have been selected by FM's
post-command-hook
The disadvantage is that stale lazy highlights are erased after
isearch-lazy-highlight-initial-delay, not immediately, as at present.
I'm not sure if this is really a disadvantage or not. In my scrappy
patch last night, I attempted to ameliorate this "problem" by repeating
(most of) the tests in the command-loop bit.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-02 23:22 ` Juri Linkov
@ 2015-11-03 12:31 ` Alan Mackenzie
2015-11-03 15:49 ` Eli Zaretskii
2015-11-03 12:31 ` Alan Mackenzie
1 sibling, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-03 12:31 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Artur Malabarba, emacs-devel
Hello, Juri.
On Tue, Nov 03, 2015 at 01:22:03AM +0200, Juri Linkov wrote:
> > So how about us just moving all these checks to where they really
> > belong, in isearch-lazy-highlight-update? I've a feeling that if we do
> > this, then your function follow--search-function becomes unneeded.
> >
> > Juri?
> Right, without (sit-for 0) it's possible to switch focus to adjacent windows
> with just adding 2 lines to follow.el, i.e. I get the desired behavior with:
> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..0433854 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -420,6 +420,7 @@ (define-minor-mode follow-mode
> (if follow-mode
> (progn
> (add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
> + (add-hook 'isearch-update-post-hook 'follow-align-compilation-windows t t)
> (add-hook 'post-command-hook 'follow-post-command-hook t)
> (add-hook 'window-size-change-functions 'follow-window-size-change t))
> ;; Remove globally-installed hook functions only if there is no
> @@ -432,6 +433,7 @@ (define-minor-mode follow-mode
> (unless following
> (remove-hook 'post-command-hook 'follow-post-command-hook)
> (remove-hook 'window-size-change-functions 'follow-window-size-change)))
> + (remove-hook 'isearch-update-post-hook 'follow-align-compilation-windows t)
> (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
> (defun follow-find-file-hook ()
This is complicated. Ideally, the Follow Mode windows should be
synchronised in FM's post-command-hook, not isearch's. It is not
isearch's job to realign windows. follow-post-command-hook both realigns
windows and choses an appropriate window to put point in. We should let
it.
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..8edf8b0 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3018,7 +3018,7 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
> search string to change or the window to scroll). It is also used
> by other Emacs features."
> (when (and (null executing-kbd-macro)
> - (sit-for 0) ;make sure (window-start) is credible
> + ; (sit-for 0) ;make sure (window-start) is credible
> (or (not (equal isearch-string
> isearch-lazy-highlight-last-string))
> (not (eq (selected-window)
> So what remains to do is to fix this bug, but I don't understand the logic
> you proposed: how checks could be moved to isearch-lazy-highlight-update
> if isearch-lazy-highlight-update is scheduled by a timer conditionally
> depending on these checks?
What I'm proposing is to schedule the timer always, and do the checks
(for whether we need to start a new lazy highlight loop) in the function
that the timer triggers. The advantage is that when the timer triggers,
redisplay will already have taken place[*], and thus we can use
`window-start' and `window-end' without forcing an artificial redisplay
with "(sit-for 0)".
[*] and an appropriate FM window will have been selected by FM's
post-command-hook
The disadvantage is that stale lazy highlights are erased after
isearch-lazy-highlight-initial-delay, not immediately, as at present.
I'm not sure if this is really a disadvantage or not. In my scrappy
patch last night, I attempted to ameliorate this "problem" by repeating
(most of) the tests in the command-loop bit.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-03 12:31 ` Alan Mackenzie
@ 2015-11-03 15:49 ` Eli Zaretskii
2015-11-03 16:18 ` Artur Malabarba
2015-11-03 16:39 ` Alan Mackenzie
0 siblings, 2 replies; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-03 15:49 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, bruce.connor.am, juri
PLEASE let's keep emacs-devel out of this??
> Date: Tue, 3 Nov 2015 12:31:16 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 17453@debbugs.gnu.org, Artur Malabarba <bruce.connor.am@gmail.com>,
> emacs-devel <emacs-devel@gnu.org>
>
> This is complicated. Ideally, the Follow Mode windows should be
> synchronised in FM's post-command-hook, not isearch's. It is not
> isearch's job to realign windows. follow-post-command-hook both realigns
> windows and choses an appropriate window to put point in. We should let
> it.
Once again, if some code in Isearch calls the same function that is
used in follow-post-command-hook, the above is not an issue.
Moreover, saving some calls to the hook will make Emacs more
responsive. (Right now, using Follow mode is a pain due to the hook:
even a simple C-f is annoyingly slow.)
> What I'm proposing is to schedule the timer always, and do the checks
> (for whether we need to start a new lazy highlight loop) in the function
> that the timer triggers. The advantage is that when the timer triggers,
> redisplay will already have taken place[*]
I don't think you can count on that: if there's a ripe timer, it will
be processed before redisplay. I think.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-03 15:49 ` Eli Zaretskii
@ 2015-11-03 16:18 ` Artur Malabarba
2015-11-03 22:11 ` Alan Mackenzie
2015-11-03 16:39 ` Alan Mackenzie
1 sibling, 1 reply; 96+ messages in thread
From: Artur Malabarba @ 2015-11-03 16:18 UTC (permalink / raw
To: Eli Zaretskii; +Cc: Alan Mackenzie, 17453, Juri Linkov
2015-11-03 15:49 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
>> This is complicated. Ideally, the Follow Mode windows should be
>> synchronised in FM's post-command-hook, not isearch's. It is not
>> isearch's job to realign windows. follow-post-command-hook both realigns
>> windows and choses an appropriate window to put point in. We should let
>> it.
>
> Once again, if some code in Isearch calls the same function that is
> used in follow-post-command-hook, the above is not an issue.
> Moreover, saving some calls to the hook will make Emacs more
> responsive.
I agree with Eli and Juri on this. If there's a solution as simple as
calling a follow-mode function in isearch-post-update-hook, then this
sounds like a no-downside solution.
Of course it's not isearch's job to realign windows, but that's why
the hook exists: so that other packages can run their own functions
and do their own jobs at that point in time.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-03 15:49 ` Eli Zaretskii
2015-11-03 16:18 ` Artur Malabarba
@ 2015-11-03 16:39 ` Alan Mackenzie
1 sibling, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-03 16:39 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 17453, bruce.connor.am, juri
Hello, Eli.
On Tue, Nov 03, 2015 at 05:49:17PM +0200, Eli Zaretskii wrote:
> PLEASE let's keep emacs-devel out of this??
OK.
> > Date: Tue, 3 Nov 2015 12:31:16 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: 17453@debbugs.gnu.org, Artur Malabarba <bruce.connor.am@gmail.com>,
> > emacs-devel <emacs-devel@gnu.org>
> > This is complicated. Ideally, the Follow Mode windows should be
> > synchronised in FM's post-command-hook, not isearch's. It is not
> > isearch's job to realign windows. follow-post-command-hook both realigns
> > windows and choses an appropriate window to put point in. We should let
> > it.
> Once again, if some code in Isearch calls the same function that is
> used in follow-post-command-hook, the above is not an issue.
> Moreover, saving some calls to the hook will make Emacs more
> responsive. (Right now, using Follow mode is a pain due to the hook:
> even a simple C-f is annoyingly slow.)
The follow-post-command-hook will run anyway. If Isearch directly
invokes that Follow Mode function, f-p-c-h will be run a second time.
That's one of the reasons I want to avoid it.
> > What I'm proposing is to schedule the timer always, and do the checks
> > (for whether we need to start a new lazy highlight loop) in the function
> > that the timer triggers. The advantage is that when the timer triggers,
> > redisplay will already have taken place[*]
> I don't think you can count on that: if there's a ripe timer, it will
> be processed before redisplay. I think.
Ah. So if somebody sets isearch-lazy-highlighting-intial-delay to
0.00001 seconds, it will trigger before redisplay has happened. Yes.
Here's another idea. In place of that "(sit-for 0)", I add another
condition
(redisplay-would-scroll-window-p)
, with the following in (probably) window.el:
(defun redisplay-would-scroll-window-p (&optional pos window)
"Would redisplay scroll WINDOW vertically on an immediate redisplay?
WINDOW defaults to the current window. POS, the position of point which
is to be tested defaults to the value of point in WINDOW. The buffer
displayed by WINDOW is assumed to be the current buffer.
Return t if a vertical scroll would be triggered by redisplay, otherwise nil."
(or window (setq window (selected-window)))
(or pos (setq pos (window-point window)))
(with-selected-window window
(with-current-buffer (window-buffer)
(not
(and (pos-visible-in-window-p pos)
(or (<= scroll-margin 0)
(let* ((this-scroll-margin (min scroll-margin
(/ (window-body-height) 4)))
(top-limit
(save-excursion
(move-to-window-line this-scroll-margin)
(point)))
(bottom-limit
(save-excursion
(move-to-window-line (- this-scroll-margin))
(point))))
(and (>= pos top-limit)
(< pos bottom-limit)))))))))
.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-03 16:18 ` Artur Malabarba
@ 2015-11-03 22:11 ` Alan Mackenzie
2015-11-04 0:28 ` Juri Linkov
0 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-03 22:11 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, Juri Linkov
Hello, Arturn.
On Tue, Nov 03, 2015 at 04:18:45PM +0000, Artur Malabarba wrote:
> 2015-11-03 15:49 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
> >> This is complicated. Ideally, the Follow Mode windows should be
> >> synchronised in FM's post-command-hook, not isearch's. It is not
> >> isearch's job to realign windows. follow-post-command-hook both realigns
> >> windows and choses an appropriate window to put point in. We should let
> >> it.
> > Once again, if some code in Isearch calls the same function that is
> > used in follow-post-command-hook, the above is not an issue.
> > Moreover, saving some calls to the hook will make Emacs more
> > responsive.
> I agree with Eli and Juri on this. If there's a solution as simple as
> calling a follow-mode function in isearch-post-update-hook, then this
> sounds like a no-downside solution.
I'm wondering if we're still talking about the same problem. ;-) A
simpler solution is _not_ to call a FM function from that Isearch hook.
Unless we're talking at cross purposes, there is simply no need. As
long as the Isearch command is allowed to go to completion without
forcibly redisplaying, FM will re-synchronise the windows (if needed)
and select an appropriate window for point, all on its own (in
follow-post-command-hook).
> Of course it's not isearch's job to realign windows, but that's why
> the hook exists: so that other packages can run their own functions
> and do their own jobs at that point in time.
Good night, and sleep well!
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-03 22:11 ` Alan Mackenzie
@ 2015-11-04 0:28 ` Juri Linkov
2015-11-04 9:01 ` Alan Mackenzie
2015-11-07 12:59 ` Alan Mackenzie
0 siblings, 2 replies; 96+ messages in thread
From: Juri Linkov @ 2015-11-04 0:28 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Artur Malabarba
>> >> This is complicated. Ideally, the Follow Mode windows should be
>> >> synchronised in FM's post-command-hook, not isearch's. It is not
>> >> isearch's job to realign windows. follow-post-command-hook both realigns
>> >> windows and choses an appropriate window to put point in. We should let
>> >> it.
>
>> > Once again, if some code in Isearch calls the same function that is
>> > used in follow-post-command-hook, the above is not an issue.
>> > Moreover, saving some calls to the hook will make Emacs more
>> > responsive.
>
>> I agree with Eli and Juri on this. If there's a solution as simple as
>> calling a follow-mode function in isearch-post-update-hook, then this
>> sounds like a no-downside solution.
>
> I'm wondering if we're still talking about the same problem. ;-) A
> simpler solution is _not_ to call a FM function from that Isearch hook.
> Unless we're talking at cross purposes, there is simply no need. As
> long as the Isearch command is allowed to go to completion without
> forcibly redisplaying, FM will re-synchronise the windows (if needed)
> and select an appropriate window for point, all on its own (in
> follow-post-command-hook).
It still might help to synchronise the windows from isearch-update-post-hook
if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
I see no problem in changing the order of calls to isearch-update-post-hook and
isearch-lazy-highlight-new-loop in isearch-update. Sure, follow-post-command-hook
will be called twice but at least this simple solution for follow-mode
doesn't require re-designing the whole lazy-highlighting machinery.
diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..75c2788 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -420,6 +420,7 @@ (define-minor-mode follow-mode
(if follow-mode
(progn
(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
+ (add-hook 'isearch-update-post-hook 'follow-post-command-hook t t)
(add-hook 'post-command-hook 'follow-post-command-hook t)
(add-hook 'window-size-change-functions 'follow-window-size-change t))
;; Remove globally-installed hook functions only if there is no
@@ -432,6 +433,7 @@ (define-minor-mode follow-mode
(unless following
(remove-hook 'post-command-hook 'follow-post-command-hook)
(remove-hook 'window-size-change-functions 'follow-window-size-change)))
+ (remove-hook 'isearch-update-post-hook 'follow-post-command-hook t)
(remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
(defun follow-find-file-hook ()
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..1e4a1a5 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1011,12 +1011,12 @@ (defun 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.
^ permalink raw reply related [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-04 0:28 ` Juri Linkov
@ 2015-11-04 9:01 ` Alan Mackenzie
2015-11-04 10:17 ` Artur Malabarba
2015-11-07 12:59 ` Alan Mackenzie
1 sibling, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-04 9:01 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Artur Malabarba
Hello, Juri.
On Wed, Nov 04, 2015 at 02:28:08AM +0200, Juri Linkov wrote:
> >> >> This is complicated. Ideally, the Follow Mode windows should be
> >> >> synchronised in FM's post-command-hook, not isearch's. It is not
> >> >> isearch's job to realign windows. follow-post-command-hook both realigns
> >> >> windows and choses an appropriate window to put point in. We should let
> >> >> it.
> >> > Once again, if some code in Isearch calls the same function that is
> >> > used in follow-post-command-hook, the above is not an issue.
> >> > Moreover, saving some calls to the hook will make Emacs more
> >> > responsive.
> >> I agree with Eli and Juri on this. If there's a solution as simple as
> >> calling a follow-mode function in isearch-post-update-hook, then this
> >> sounds like a no-downside solution.
> > I'm wondering if we're still talking about the same problem. ;-) A
> > simpler solution is _not_ to call a FM function from that Isearch hook.
> > Unless we're talking at cross purposes, there is simply no need. As
> > long as the Isearch command is allowed to go to completion without
> > forcibly redisplaying, FM will re-synchronise the windows (if needed)
> > and select an appropriate window for point, all on its own (in
> > follow-post-command-hook).
> It still might help to synchronise the windows from isearch-update-post-hook
> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
I still say, wait until we really need it before we do anything so
drastic. As Eli noted, follow-post-command-hook is SLOW, SLOW, SLOW.
If we call it twice per command, it will be twice as slow.
Also, why is the "(sit-for 0)" there at all? As its comment says, It
is there for one purpose, and one purpose only: it is so that
(window-start) is valid, and the check
(not (= (window-start)
isearch-lazy-highlight-window-start))
will work. This check means exactly "has the window scrolled?".
> I see no problem in changing the order of calls to isearch-update-post-hook and
> isearch-lazy-highlight-new-loop in isearch-update. Sure, follow-post-command-hook
> will be called twice but at least this simple solution for follow-mode
> doesn't require re-designing the whole lazy-highlighting machinery.
In one of my mails yesterday, I proposed removing the (sit-for 0) and
replacing this check on (window-start) with
(redisplay-would-scroll-window-p)
. This would fix the bug without any further changes. It would avoid
any far reaching change in design of the lazy highlighting, avoid
calling follow-post-command-hook twice, yet would work.
> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..75c2788 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -420,6 +420,7 @@ (define-minor-mode follow-mode
> (if follow-mode
> (progn
> (add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
> + (add-hook 'isearch-update-post-hook 'follow-post-command-hook t t)
> (add-hook 'post-command-hook 'follow-post-command-hook t)
> (add-hook 'window-size-change-functions 'follow-window-size-change t))
> ;; Remove globally-installed hook functions only if there is no
> @@ -432,6 +433,7 @@ (define-minor-mode follow-mode
> (unless following
> (remove-hook 'post-command-hook 'follow-post-command-hook)
> (remove-hook 'window-size-change-functions 'follow-window-size-change)))
> + (remove-hook 'isearch-update-post-hook 'follow-post-command-hook t)
> (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
> (defun follow-find-file-hook ()
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..1e4a1a5 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -1011,12 +1011,12 @@ (defun 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.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-04 9:01 ` Alan Mackenzie
@ 2015-11-04 10:17 ` Artur Malabarba
2015-11-05 12:38 ` Alan Mackenzie
0 siblings, 1 reply; 96+ messages in thread
From: Artur Malabarba @ 2015-11-04 10:17 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Juri Linkov
2015-11-04 9:01 GMT+00:00 Alan Mackenzie <acm@muc.de>:
>> It still might help to synchronise the windows from isearch-update-post-hook
>> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
>
> I still say, wait until we really need it before we do anything so
> drastic. As Eli noted, follow-post-command-hook is SLOW, SLOW, SLOW.
> If we call it twice per command, it will be twice as slow.
>
> Also, why is the "(sit-for 0)" there at all? As its comment says, It
> is there for one purpose, and one purpose only: it is so that
> (window-start) is valid, and the check
>
> (not (= (window-start)
> isearch-lazy-highlight-window-start))
>
> will work. This check means exactly "has the window scrolled?"
Have you tested redisplay-would-scroll-window-p in a fully folded
org-mode buffer with 100 headlines where each headline has 100 lines
of content?
If you report that it works in this context and isn't slow then I'm ok
with going with this solution. Like I said, as long as it doesn't
cause regressions, this would be a fine refactoring to do in
isearch.el. So we might as well apply it now and give any possible
issues some time to surface (isearch is used by tons of people, so I'm
sure they'll surface if any exist).
While you're at it, if you could also refactor all those `(not (equal
...))' tests into a single `isearch--lazy-highlight-needs-update-p'
function that would be very welcome (and if you do, do it as a
separate commit before everything else so the other stuff is easy to
revert separately).
And if it looks like I'm saying "I'm fine with this" to everything
here, that's because I am. It sounds like we're debating two fine
options and bordering on bikeshedding. So I say: merge one and see how
it goes.
Cheers
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-04 10:17 ` Artur Malabarba
@ 2015-11-05 12:38 ` Alan Mackenzie
2015-11-05 17:13 ` Artur Malabarba
0 siblings, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-05 12:38 UTC (permalink / raw
To: Artur Malabarba; +Cc: 17453, Juri Linkov
Hello, Artur.
On Wed, Nov 04, 2015 at 10:17:09AM +0000, Artur Malabarba wrote:
> 2015-11-04 9:01 GMT+00:00 Alan Mackenzie <acm@muc.de>:
> >> It still might help to synchronise the windows from isearch-update-post-hook
> >> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
> > I still say, wait until we really need it before we do anything so
> > drastic. As Eli noted, follow-post-command-hook is SLOW, SLOW, SLOW.
> > If we call it twice per command, it will be twice as slow.
> > Also, why is the "(sit-for 0)" there at all? As its comment says, It
> > is there for one purpose, and one purpose only: it is so that
> > (window-start) is valid, and the check
> > (not (= (window-start)
> > isearch-lazy-highlight-window-start))
> > will work. This check means exactly "has the window scrolled?"
> Have you tested redisplay-would-scroll-window-p in a fully folded
> org-mode buffer with 100 headlines where each headline has 100 lines
> of content?
Of course not. :-)
By the way, what does Isearch do with such org-mode folded buffer? Does
it unfold the content, ever?
> If you report that it works in this context and isn't slow then I'm ok
> with going with this solution. Like I said, as long as it doesn't
> cause regressions, this would be a fine refactoring to do in
> isearch.el. So we might as well apply it now and give any possible
> issues some time to surface (isearch is used by tons of people, so I'm
> sure they'll surface if any exist).
> While you're at it, if you could also refactor all those `(not (equal
> ...))' tests into a single `isearch--lazy-highlight-needs-update-p'
> function that would be very welcome (and if you do, do it as a
> separate commit before everything else so the other stuff is easy to
> revert separately).
I don't agree this is a good idea. At the moment, all these tests are
in the same not too big defun that sets them. This kind of keeps things
together - for instance if a new test was to be introduced, only this
one function would need amending.
> And if it looks like I'm saying "I'm fine with this" to everything
> here, that's because I am. It sounds like we're debating two fine
> options and bordering on bikeshedding. So I say: merge one and see how
> it goes.
I'm pretty much ready to do this. But maybe you could answer my
question about unfolding org-mode buffers above, first. Thanks!
> Cheers
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-05 12:38 ` Alan Mackenzie
@ 2015-11-05 17:13 ` Artur Malabarba
0 siblings, 0 replies; 96+ messages in thread
From: Artur Malabarba @ 2015-11-05 17:13 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, Juri Linkov
>> Have you tested redisplay-would-scroll-window-p in a fully folded
>> org-mode buffer with 100 headlines where each headline has 100 lines
>> of content?
>
> Of course not. :-)
>
> By the way, what does Isearch do with such org-mode folded buffer? Does
> it unfold the content, ever?
Isearch has an option for that. By default it searches inside hidden
text, and reveals the hidden text if the match is in there.
But that's not why I asked.
The 'count-screen-lines' function can be horribly slow on a large
number of lines. I just want to check whether 'move-to-window-line'
doesn't suffer from the same issue. A huge org buffer with lots of
folded headlines is just a common cause of slowness for
`count-screen-lines' because small visual movements could mean very
large actual movements.
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-04 0:28 ` Juri Linkov
2015-11-04 9:01 ` Alan Mackenzie
@ 2015-11-07 12:59 ` Alan Mackenzie
2015-11-07 13:38 ` Eli Zaretskii
1 sibling, 1 reply; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-07 12:59 UTC (permalink / raw
To: Juri Linkov; +Cc: 17453, Artur Malabarba
Hello, Juri, Artur, Eli.
On Wed, Nov 04, 2015 at 02:28:08AM +0200, Juri Linkov wrote:
> >> >> This is complicated. Ideally, the Follow Mode windows should be
> >> >> synchronised in FM's post-command-hook, not isearch's. It is not
> >> >> isearch's job to realign windows. follow-post-command-hook both realigns
> >> >> windows and choses an appropriate window to put point in. We should let
> >> >> it.
> >> > Once again, if some code in Isearch calls the same function that is
> >> > used in follow-post-command-hook, the above is not an issue.
> >> > Moreover, saving some calls to the hook will make Emacs more
> >> > responsive.
> >> I agree with Eli and Juri on this. If there's a solution as simple as
> >> calling a follow-mode function in isearch-post-update-hook, then this
> >> sounds like a no-downside solution.
> > I'm wondering if we're still talking about the same problem. ;-) A
> > simpler solution is _not_ to call a FM function from that Isearch hook.
> > Unless we're talking at cross purposes, there is simply no need. As
> > long as the Isearch command is allowed to go to completion without
> > forcibly redisplaying, FM will re-synchronise the windows (if needed)
> > and select an appropriate window for point, all on its own (in
> > follow-post-command-hook).
> It still might help to synchronise the windows from isearch-update-post-hook
> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
> I see no problem in changing the order of calls to isearch-update-post-hook and
> isearch-lazy-highlight-new-loop in isearch-update. Sure, follow-post-command-hook
> will be called twice but at least this simple solution for follow-mode
> doesn't require re-designing the whole lazy-highlighting machinery.
The discussion of this bug seems to have stalled. I'd very much like to
decide all the issues now, and to fix (this part of) this bug.
Bug summary: With follow mode active, and lazy highliting active, forward
Isearch scrolls the left hand window when it should instead move to to
the right hand window.
Immediate cause: the form "(sit-for 0)" in
isearch-lazy-highlight-new-loop scrolls the window before Follow Mode has
had a chance to resynchronise everything.
Proposed solutions:
1. Call follow-post-command-hook from isearch-update before calling
isearch-lazy-highlight-new-loop (as described above).
2. Call the proposed function `redisplay-would-scroll-window' instead of
the `sit-for'.
3. Make isearch-lazy-highlight-new-loop always set the idle timer, and
test for the need for a new loop instead in the function it triggers.
Remove the `sit-for'.
I now think solution 2. is not sensible or realistic. Redisplay is just
too complicated to second-guess.
Solution 1. has the disadvantage that follow-post-command-hook would be
called twice for every command in Isearch. It is not fast.
Solution 3. similarly might have the problem that if
lazy-highlight-initial-delay is set to zero, redisplay might not have
done its work when isearch-lazy-highlight-update runs. (I haven't tried
it out, yet).
Personally, I am in favour of solution 3, but I'm willing to be persuaded
into solution 1. But I'd like us to come to a decision quickly.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-07 12:59 ` Alan Mackenzie
@ 2015-11-07 13:38 ` Eli Zaretskii
2015-11-08 10:32 ` Alan Mackenzie
0 siblings, 1 reply; 96+ messages in thread
From: Eli Zaretskii @ 2015-11-07 13:38 UTC (permalink / raw
To: Alan Mackenzie; +Cc: 17453, bruce.connor.am, juri
> Date: Sat, 7 Nov 2015 12:59:31 +0000
> Cc: Artur Malabarba <bruce.connor.am@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> 17453@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
>
> Proposed solutions:
> 1. Call follow-post-command-hook from isearch-update before calling
> isearch-lazy-highlight-new-loop (as described above).
> 2. Call the proposed function `redisplay-would-scroll-window' instead of
> the `sit-for'.
> 3. Make isearch-lazy-highlight-new-loop always set the idle timer, and
> test for the need for a new loop instead in the function it triggers.
> Remove the `sit-for'.
>
> I now think solution 2. is not sensible or realistic. Redisplay is just
> too complicated to second-guess.
>
> Solution 1. has the disadvantage that follow-post-command-hook would be
> called twice for every command in Isearch. It is not fast.
>
> Solution 3. similarly might have the problem that if
> lazy-highlight-initial-delay is set to zero, redisplay might not have
> done its work when isearch-lazy-highlight-update runs. (I haven't tried
> it out, yet).
>
> Personally, I am in favour of solution 3, but I'm willing to be persuaded
> into solution 1. But I'd like us to come to a decision quickly.
How about using 1), but also adding some indication that could prevent
the post-command-hook from being called twice?
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Isearch doesn't work properly with Follow Mode.
2015-11-07 13:38 ` Eli Zaretskii
@ 2015-11-08 10:32 ` Alan Mackenzie
0 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-11-08 10:32 UTC (permalink / raw
To: Eli Zaretskii; +Cc: 17453, bruce.connor.am, juri
Hello, Eli.
On Sat, Nov 07, 2015 at 03:38:31PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 7 Nov 2015 12:59:31 +0000
> > Cc: Artur Malabarba <bruce.connor.am@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> > 17453@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>
> > Proposed solutions:
> > 1. Call follow-post-command-hook from isearch-update before calling
> > isearch-lazy-highlight-new-loop (as described above).
> > 2. Call the proposed function `redisplay-would-scroll-window' instead of
> > the `sit-for'.
> > 3. Make isearch-lazy-highlight-new-loop always set the idle timer, and
> > test for the need for a new loop instead in the function it triggers.
> > Remove the `sit-for'.
> > I now think solution 2. is not sensible or realistic. Redisplay is just
> > too complicated to second-guess.
> > Solution 1. has the disadvantage that follow-post-command-hook would be
> > called twice for every command in Isearch. It is not fast.
> > Solution 3. similarly might have the problem that if
> > lazy-highlight-initial-delay is set to zero, redisplay might not have
> > done its work when isearch-lazy-highlight-update runs. (I haven't tried
> > it out, yet).
> > Personally, I am in favour of solution 3, but I'm willing to be persuaded
> > into solution 1. But I'd like us to come to a decision quickly.
> How about using 1), but also adding some indication that could prevent
> the post-command-hook from being called twice?
Hmm. Such artifices are not pretty. Such a flag would have to be set in
isearch.el just after isearch-update-post-hook (which calls
follow-post-command-hook) has been invoked. Something would have to
clear it, perhaps something at the end of the lazy highlight loop. Also,
this would couple Isearch and Follow Mode undesirably: Isearch really
shouldn't have to know anything about the FM window setup.
Or, maybe just letting follow-post-command-hook run twice wouldn't be too
bad. The second time, all the windows would already be synchronised and
point would be in the right window. I've had a look at the code, and it
seems in this situation it actually does call the function to rearrange
the windows. Maybe that is a bug which might be fixed.
Do you have any views on solution 3?
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
* bug#17453: Acknowledgement (Isearch doesn't work properly with Follow Mode.)
[not found] ` <handler.17453.B.139967578531952.ack@debbugs.gnu.org>
@ 2015-12-20 12:59 ` Alan Mackenzie
0 siblings, 0 replies; 96+ messages in thread
From: Alan Mackenzie @ 2015-12-20 12:59 UTC (permalink / raw
To: 17453-done
Bug fixed in emacs-25 branch.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 96+ messages in thread
end of thread, other threads:[~2015-12-20 12:59 UTC | newest]
Thread overview: 96+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-09 22:44 bug#17453: Isearch doesn't work properly with Follow Mode Alan Mackenzie
2014-05-10 2:40 ` Stefan Monnier
2014-05-11 12:58 ` Alan Mackenzie
2014-05-11 16:09 ` Stefan Monnier
2014-05-11 18:18 ` Alan Mackenzie
2014-05-11 19:05 ` Stefan Monnier
2014-05-11 20:40 ` Alan Mackenzie
2014-05-11 21:46 ` Stefan Monnier
2015-10-29 23:23 ` Alan Mackenzie
2015-10-29 23:23 ` Alan Mackenzie
2015-10-31 22:35 ` John Wiegley
2015-10-31 22:35 ` John Wiegley
2015-10-31 23:25 ` Alan Mackenzie
2015-10-31 23:25 ` Alan Mackenzie
2015-10-31 23:41 ` John Wiegley
2015-10-31 23:41 ` John Wiegley
2015-11-01 11:59 ` Alan Mackenzie
2015-11-01 11:59 ` Alan Mackenzie
2015-11-01 0:17 ` Drew Adams
2015-11-01 0:17 ` Drew Adams
2015-10-31 23:13 ` Artur Malabarba
2015-10-31 23:13 ` Artur Malabarba
2015-10-31 23:32 ` Alan Mackenzie
2015-10-31 23:32 ` Alan Mackenzie
2015-11-01 12:20 ` Artur Malabarba
2015-11-01 12:20 ` Artur Malabarba
2015-11-01 12:23 ` Artur Malabarba
2015-11-01 13:52 ` Alan Mackenzie
2015-11-01 16:50 ` Eli Zaretskii
2015-11-01 18:27 ` Alan Mackenzie
2015-11-01 19:46 ` Artur Malabarba
2015-11-01 20:15 ` Alan Mackenzie
2015-11-01 21:37 ` Artur Malabarba
2015-11-01 20:42 ` Artur Malabarba
2015-11-01 20:54 ` Eli Zaretskii
2015-11-01 22:19 ` Alan Mackenzie
2015-11-01 12:23 ` Artur Malabarba
2015-10-31 23:35 ` Juri Linkov
2015-10-31 23:35 ` Juri Linkov
2015-10-31 23:56 ` Alan Mackenzie
2015-10-31 23:56 ` Alan Mackenzie
2015-11-02 0:14 ` Juri Linkov
2015-11-02 3:35 ` Eli Zaretskii
2015-11-02 3:35 ` Eli Zaretskii
2015-11-02 9:28 ` Alan Mackenzie
2015-11-02 11:53 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
2015-11-02 12:14 ` Artur Malabarba
2015-11-02 12:39 ` Alan Mackenzie
2015-11-02 12:39 ` Alan Mackenzie
2015-11-02 12:35 ` Alan Mackenzie
2015-11-02 13:10 ` Artur Malabarba
2015-11-02 14:18 ` Artur Malabarba
2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 15:44 ` Alan Mackenzie
2015-11-02 16:26 ` Artur Malabarba
2015-11-02 16:35 ` Drew Adams
2015-11-02 19:18 ` Artur Malabarba
2015-11-02 19:28 ` Drew Adams
2015-11-02 23:45 ` Juri Linkov
2015-11-02 22:09 ` Alan Mackenzie
2015-11-02 23:00 ` Artur Malabarba
2015-11-03 9:18 ` Alan Mackenzie
2015-11-02 17:45 ` Eli Zaretskii
2015-11-02 23:22 ` Juri Linkov
2015-11-02 23:22 ` Juri Linkov
2015-11-03 12:31 ` Alan Mackenzie
2015-11-03 15:49 ` Eli Zaretskii
2015-11-03 16:18 ` Artur Malabarba
2015-11-03 22:11 ` Alan Mackenzie
2015-11-04 0:28 ` Juri Linkov
2015-11-04 9:01 ` Alan Mackenzie
2015-11-04 10:17 ` Artur Malabarba
2015-11-05 12:38 ` Alan Mackenzie
2015-11-05 17:13 ` Artur Malabarba
2015-11-07 12:59 ` Alan Mackenzie
2015-11-07 13:38 ` Eli Zaretskii
2015-11-08 10:32 ` Alan Mackenzie
2015-11-03 16:39 ` Alan Mackenzie
2015-11-03 12:31 ` Alan Mackenzie
2015-11-02 23:28 ` Juri Linkov
2015-11-02 23:28 ` Juri Linkov
2015-11-02 14:18 ` Artur Malabarba
2015-11-02 13:10 ` Artur Malabarba
2015-11-02 15:46 ` Eli Zaretskii
2015-11-02 16:09 ` Alan Mackenzie
2015-11-02 17:49 ` Eli Zaretskii
2015-11-02 20:35 ` John Wiegley
2015-11-03 8:35 ` Alan Mackenzie
[not found] ` <<831tc8xv39.fsf@gnu.org>
2015-11-02 16:05 ` Drew Adams
2015-11-02 12:35 ` Alan Mackenzie
2015-11-02 11:53 ` Artur Malabarba
2015-11-02 23:33 ` Juri Linkov
2015-11-02 23:33 ` Juri Linkov
2015-11-02 0:14 ` Juri Linkov
[not found] ` <handler.17453.B.139967578531952.ack@debbugs.gnu.org>
2015-12-20 12:59 ` bug#17453: Acknowledgement (Isearch doesn't work properly with Follow Mode.) Alan Mackenzie
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.