From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Tue, 19 Feb 2013 14:50:57 +0000 Message-ID: <20130219145057.GA4377@acm.acm> References: <20130110132530.GA2805@acm.acm> <20130214200937.GA3336@acm.acm> <87d2w2ggsh.fsf@mail.jurta.org> <87liapan32.fsf@mail.jurta.org> <20130215132004.GB2907@acm.acm> <87ip5s3tq8.fsf@mail.jurta.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1361285535 19771 80.91.229.3 (19 Feb 2013 14:52:15 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 19 Feb 2013 14:52:15 +0000 (UTC) Cc: 13402@debbugs.gnu.org To: Juri Linkov Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Feb 19 15:52:37 2013 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1U7oYf-0006uF-3s for geb-bug-gnu-emacs@m.gmane.org; Tue, 19 Feb 2013 15:52:37 +0100 Original-Received: from localhost ([::1]:45901 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U7oYL-0007Ej-4N for geb-bug-gnu-emacs@m.gmane.org; Tue, 19 Feb 2013 09:52:17 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:34590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U7oYB-0007D6-C2 for bug-gnu-emacs@gnu.org; Tue, 19 Feb 2013 09:52:14 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U7oY1-0002yS-E4 for bug-gnu-emacs@gnu.org; Tue, 19 Feb 2013 09:52:07 -0500 Original-Received: from debbugs.gnu.org ([140.186.70.43]:59029) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U7oY1-0002yO-Aq for bug-gnu-emacs@gnu.org; Tue, 19 Feb 2013 09:51:57 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.72) (envelope-from ) id 1U7oZ4-0004Mw-Bn for bug-gnu-emacs@gnu.org; Tue, 19 Feb 2013 09:53:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 19 Feb 2013 14:53:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 13402 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 13402-submit@debbugs.gnu.org id=B13402.136128556716753 (code B ref 13402); Tue, 19 Feb 2013 14:53:02 +0000 Original-Received: (at 13402) by debbugs.gnu.org; 19 Feb 2013 14:52:47 +0000 Original-Received: from localhost ([127.0.0.1]:36260 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1U7oYo-0004M6-HD for submit@debbugs.gnu.org; Tue, 19 Feb 2013 09:52:47 -0500 Original-Received: from colin.muc.de ([193.149.48.1]:42037 helo=mail.muc.de) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1U7oYk-0004Lp-B3 for 13402@debbugs.gnu.org; Tue, 19 Feb 2013 09:52:44 -0500 Original-Received: (qmail 50865 invoked by uid 3782); 19 Feb 2013 14:51:35 -0000 Original-Received: from acm.muc.de (pD955791A.dip.t-dialin.net [217.85.121.26]) by colin.muc.de (tmda-ofmipd) with ESMTP; Tue, 19 Feb 2013 15:51:29 +0100 Original-Received: (qmail 4615 invoked by uid 1000); 19 Feb 2013 14:50:57 -0000 Content-Disposition: inline In-Reply-To: <87ip5s3tq8.fsf@mail.jurta.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Delivery-Agent: TMDA/1.1.12 (Macallan) X-Primary-Address: acm@muc.de X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.13 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:71515 Archived-At: Hi, Juri. I'm answering both your last two emails together, here. On Sat, Feb 16, 2013 at 11:50:39PM +0200, Juri Linkov wrote: > > OK, here's a better patch. As already suggested, every match now has its > > lazy-highlight overlay, just that the one overlapping the current match > > no longer has the 'face property set. I don't think there can be more > > than one overlapping match. This approach preserves the optimisation > > with `C-s'. > Yes, this is a more reliable approach, and it works correctly now in isearch. > It fails only in `replace-highlight', i.e. after running `query-replace' > and answering `n' to skip to the next match, it doesn't re-highlight > the previous skipped match. Fixed, see patch below. > But in principle I don't oppose your approach provided it's working > correctly in all cases. .../textmodes/ispell.el also uses the lazy highlighting, so it will need amending too. > While testing I noticed an interesting effect. Test case: > emacs -Q > Eval (info "(elisp) Syntax Table Internals") > Place point at the start of the second paragraph > (" Each entry in a ...."). > Type `C-s C-w C-w' that puts " each" to the search string. > Type another C-s (isearch-repeat-forward) that will go to the second > match of " each" (try to use a smaller font to be able to see both > matches of the string " each" on the same screen). > As soon as the cursor leaves the first match, its highlighting > (with a different color for a lazy match) grows to a wider region > previously hidden by your patch. Yes. This is logical - the (extended) lazy highlighting indicates what would get fully highlighted on a future (wrapped) repeated search. I'm not saying it's a feature, but it's certainly logical. ;-) > This indicates that surprises will remain even after using your approach. > Another case surprising to users is reversing the search direction - > e.g. when the cursor is on the second match, type `C-r' and see how > the lazy-highlighted first match shrinks from multi-line to single-line. > It seems nothing can be done to fix this case. This is present even without my patch. This shrunk lazy highlight indicates precisely what would be matched on a repeated backward search. This is sort of logical too. It would be too much work to make the backward search match all the whitespace greedily. > Also I noticed that typing `C-s M-s SPC C-s' quickly causes the error: > Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p > nil) > overlays-in(nil 244) [ .... ] This is now fixed, too. It seems that `run-with-idle-timer' doesn't retain the current `let'-bindings when it runs, even though the documentation doesn't mention this. This is the reason for all these isearch-lazy-highlight-.... static variables shadowing dynamic ones. I've learnt something today. :-) > Oh, and another problem: after customizing `lazy-highlight-cleanup' to > `nil', exiting isearch doesn't leave the current match > lazy-highlighted. Yuck, what a variable! I've fixed this, too. > The problem is that other features are expecting that _all_ matches > are lazy-highlighted, so I'm starting to doubt whether it's worth the > trouble trying to improve such cosmetic issue? It made me seriously unhappy, to the point where I would have disabled whatever was causing it. It looks very scrappy, unlike the rest of Emacs. I predict, if we don't fix it, there will be quite a few angry bug reports, a bit like when the "fringe" was introduced in 21.1 with no way of disabling it, but probably not as bad. And this _is_ a regression, maybe not in the code, but certainly from the point of view of a user. It is true that this effect can be disabled by `isearch-toggle-lax-whitespace' but there is no way a normal user can find this out, apart from stumbling on it by luck. We can't really document this either. I'm surprised how difficult the fix is. Maybe lazy-highlighting should be refactored out of isearch.el and given a better interface. Here's the latest patch, which I think now works, apart from ispell.el. === modified file 'lisp/isearch.el' *** lisp/isearch.el 2013-02-01 23:38:41 +0000 --- lisp/isearch.el 2013-02-19 13:05:44 +0000 *************** *** 2862,2867 **** --- 2862,2870 ---- (defvar isearch-lazy-highlight-end-limit nil) (defvar isearch-lazy-highlight-start nil) (defvar isearch-lazy-highlight-end nil) + (defvar isearch-lazy-highlight-point nil) + (defvar isearch-lazy-highlight-shadowed nil) + (defvar isearch-lazy-highlight-other-end nil) (defvar isearch-lazy-highlight-timer nil) (defvar isearch-lazy-highlight-last-string nil) (defvar isearch-lazy-highlight-window nil) *************** *** 2882,2891 **** `lazy-highlight-cleanup' is non-nil." (interactive '(t)) (if (or force lazy-highlight-cleanup) ! (while isearch-lazy-highlight-overlays ! (delete-overlay (car isearch-lazy-highlight-overlays)) ! (setq isearch-lazy-highlight-overlays ! (cdr isearch-lazy-highlight-overlays)))) (when isearch-lazy-highlight-timer (cancel-timer isearch-lazy-highlight-timer) (setq isearch-lazy-highlight-timer nil))) --- 2885,2898 ---- `lazy-highlight-cleanup' is non-nil." (interactive '(t)) (if (or force lazy-highlight-cleanup) ! (progn ! (while isearch-lazy-highlight-overlays ! (delete-overlay (car isearch-lazy-highlight-overlays)) ! (setq isearch-lazy-highlight-overlays ! (cdr isearch-lazy-highlight-overlays))) ! (setq isearch-lazy-highlight-shadowed nil)) ! (when isearch-lazy-highlight-shadowed ! (overlay-put isearch-lazy-highlight-shadowed 'face lazy-highlight-face))) (when isearch-lazy-highlight-timer (cancel-timer isearch-lazy-highlight-timer) (setq isearch-lazy-highlight-timer nil))) *************** *** 2894,2955 **** '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." ! (when (and (null executing-kbd-macro) ! (sit-for 0) ;make sure (window-start) is credible ! (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-word ! isearch-word)) ! (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)))) ! ;; something important did indeed change ! (lazy-highlight-cleanup t) ;kill old loop & remove overlays ! (setq isearch-lazy-highlight-error isearch-error) ! ;; It used to check for `(not isearch-error)' here, but actually ! ;; lazy-highlighting might find matches to highlight even when ! ;; `isearch-error' is non-nil. (Bug#9918) ! (setq isearch-lazy-highlight-start-limit beg ! isearch-lazy-highlight-end-limit end) ! (setq isearch-lazy-highlight-window (selected-window) ! isearch-lazy-highlight-window-start (window-start) ! isearch-lazy-highlight-window-end (window-end) ! isearch-lazy-highlight-start (point) ! isearch-lazy-highlight-end (point) ! isearch-lazy-highlight-wrapped nil ! isearch-lazy-highlight-last-string isearch-string ! isearch-lazy-highlight-case-fold-search isearch-case-fold-search ! isearch-lazy-highlight-regexp isearch-regexp ! isearch-lazy-highlight-lax-whitespace isearch-lax-whitespace ! isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace ! isearch-lazy-highlight-word isearch-word ! 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))))) (defun isearch-lazy-highlight-search () "Search ahead for the next or previous match, for lazy highlighting. --- 2901,2984 ---- 'lazy-highlight-cleanup "22.1") + (defun isearch-lazy-highlight-move-shadow () + "Move the lazy highlight \"shadow\" to the current match position." + (overlay-put isearch-lazy-highlight-shadowed 'face lazy-highlight-face) + (let ((ovs (if isearch-forward + (overlays-in isearch-other-end (point)) + (overlays-in (point) isearch-other-end))) + ov) + (while ovs + (setq ov (car ovs) + ovs (cdr ovs)) + (when (memq ov isearch-lazy-highlight-overlays) + (overlay-put ov 'face nil) + (setq isearch-lazy-highlight-shadowed ov))))) + (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." ! (if (and (null executing-kbd-macro) ! (sit-for 0) ;make sure (window-start) is credible ! (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-word ! isearch-word)) ! (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)))) ! ;; something important did indeed change ! (progn ! (lazy-highlight-cleanup t) ;kill old loop & remove overlays ! (setq isearch-lazy-highlight-error isearch-error) ! ;; It used to check for `(not isearch-error)' here, but actually ! ;; lazy-highlighting might find matches to highlight even when ! ;; `isearch-error' is non-nil. (Bug#9918) ! (setq isearch-lazy-highlight-start-limit beg ! isearch-lazy-highlight-end-limit end) ! (setq isearch-lazy-highlight-window (selected-window) ! isearch-lazy-highlight-window-start (window-start) ! isearch-lazy-highlight-window-end (window-end) ! isearch-lazy-highlight-start (point) ! isearch-lazy-highlight-end (point) ! isearch-lazy-highlight-point (point) ! isearch-lazy-highlight-shadowed nil ! isearch-lazy-highlight-other-end isearch-other-end ! isearch-lazy-highlight-wrapped nil ! isearch-lazy-highlight-last-string isearch-string ! isearch-lazy-highlight-case-fold-search isearch-case-fold-search ! isearch-lazy-highlight-regexp isearch-regexp ! isearch-lazy-highlight-lax-whitespace isearch-lax-whitespace ! isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace ! isearch-lazy-highlight-word isearch-word ! 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)))) ! ! ;; Swap the previously "shadowed" lazy highlight overlay for the new one. ! (when isearch-lazy-highlight-shadowed ! (isearch-lazy-highlight-move-shadow)))) (defun isearch-lazy-highlight-search () "Search ahead for the next or previous match, for lazy highlighting. *************** *** 3033,3039 **** ;; 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)))) (if isearch-lazy-highlight-forward (setq isearch-lazy-highlight-end (point)) --- 3062,3074 ---- ;; 1000 is higher than ediff's 100+, ;; but lower than isearch main overlay's 1001 (overlay-put ov 'priority 1000) ! (if (if isearch-lazy-highlight-forward ! (or (<= me isearch-lazy-highlight-other-end) ! (>= mb isearch-lazy-highlight-point)) ! (or (<= me isearch-lazy-highlight-point) ! (>= mb isearch-lazy-highlight-other-end))) ! (overlay-put ov 'face lazy-highlight-face) ! (setq isearch-lazy-highlight-shadowed ov)) (overlay-put ov 'window (selected-window)))) (if isearch-lazy-highlight-forward (setq isearch-lazy-highlight-end (point)) === modified file 'lisp/replace.el' *** lisp/replace.el 2013-02-01 23:38:41 +0000 --- lisp/replace.el 2013-02-17 22:16:38 +0000 *************** *** 2198,2203 **** --- 2198,2204 ---- replace-regexp-lax-whitespace) (isearch-case-fold-search case-fold-search) (isearch-forward t) + (isearch-other-end match-beg) (isearch-error nil)) (isearch-lazy-highlight-new-loop range-beg range-end)))) -- Alan Mackenzie (Nuremberg, Germany).