bug#11746: feature request: `isearch-query-replace' should open invisible text

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Michael Heerdegen]

Hi,

if you have `search-invisible' non-nil, isearch "opens" invisible
text.  But when you hit M-% or C-M-% while searching, you loose this
ability: point will just be put inside invisible areas, and you don't
see what you're doing.  This is somewhat inconsistent.  Having an open
invisible feature for replacing text would be very convenient - think
of org files, for example.

Dunno if this would be easy to implement.  `isearch-query-replace'
uses just `perform-replace' from replace.el, which doesn't care about
invisible text.


Thanks,

Michael.




In GNU Emacs 24.1.50.1 (i486-pc-linux-gnu, GTK+ Version 3.4.2)
 of 2012-06-15 on zelenka, modified by Debian
 (emacs-snapshot package, version 2:20120615-1)
Windowing system distributor `The X.Org Foundation', version 11.0.11201902
Configured using:
 `configure '--build' 'i486-linux-gnu' '--host' 'i486-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs-snapshot:/etc/emacs:/usr/local/share/emacs/24.1.50/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.1.50/site-lisp:/usr/share/emacs/site-lisp'
 '--without-compress-info' '--with-crt-dir=/usr/lib/i386-linux-gnu/'
 '--with-x=yes' '--with-x-toolkit=gtk3' '--with-imagemagick=yes'
 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu'
 'CFLAGS=-DDEBIAN -DSITELOAD_PURESIZE_EXTRA=5000 -g -O2' 'LDFLAGS=-g
 -Wl,--as-needed -znocombreloc' 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Juri Linkov]

> Dunno if this would be easy to implement.  `isearch-query-replace'
> uses just `perform-replace' from replace.el, which doesn't care about
> invisible text.

A quick try does exactly what is needed:

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2012-05-01 02:48:41 +0000
+++ lisp/replace.el	2012-06-19 21:14:59 +0000
@@ -1840,7 +1840,9 @@ (defun perform-replace (from-string repl
 					   limit t)
 				  ;; For speed, use only integers and
 				  ;; reuse the list used last time.
-				  (replace-match-data t real-match-data)))
+				  (prog1 (replace-match-data t real-match-data)
+				    (isearch-range-invisible
+				     (match-beginning 0) (match-end 0)))))
 				((and (< (1+ (point)) (point-max))
 				      (or (null limit)
 					  (< (1+ (point)) limit)))

But it would be better to implement this the same way as for isearch
filters.  Adding the same filters in replacements also will make the
variable `query-replace-skip-read-only' obsolete.

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Juri Linkov]

> if you have `search-invisible' non-nil, isearch "opens" invisible
> text.  But when you hit M-% or C-M-% while searching, you loose this
> ability: point will just be put inside invisible areas, and you don't
> see what you're doing.

The recent adaptation of isearch functions to `perform-replace'
makes it easy to implement this now.

Depending on the value of `search-invisible', if `open' then M-% or C-M-%
will open invisible overlays, if `nil' then they will skip invisible text,
and if `t' they will match replacements inside invisible areas like now.

The key point is using a search loop like in `isearch-search' that
either opens invisible overlays or skips them.

BTW, a similar loop could be later added to occur and hi-lock too,
but since in occur and hi-lock it makes no sense to open overlays,
they should just skip invisible areas like in `isearch-lazy-highlight-search'.

The patch below moves the search related part of code from `perform-replace'
to a new function `replace-search', adds a loop like in `isearch-search',
and moves handling of `query-replace-skip-read-only' to this loop.

It's worth to note that `wdired-isearch-filter-read-only' already has
exactly the same condition that checks for read-only-ness, so the
same condition is duplicated for modes that set both a read-only-skipping
isearch filter and `query-replace-skip-read-only', but this is not a problem.

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2013-02-01 23:38:41 +0000
+++ lisp/replace.el	2013-02-14 18:55:14 +0000
@@ -1794,6 +1796,54 @@ (defvar replace-re-search-function nil
 It is called with three arguments, as if it were
 `re-search-forward'.")
 
+(defun replace-search (search-string limit regexp-flag delimited-flag
+				     case-fold-search)
+  "Search the next occurence of SEARCH-STRING to replace."
+  ;; Let-bind global isearch-* variables to values used
+  ;; to search the next replacement.  These let-bindings
+  ;; should be effective both at the time of calling
+  ;; `isearch-search-fun-default' and also at the
+  ;; time of funcalling `search-function'.
+  ;; These isearch-* bindings can't be placed higher
+  ;; outside of this function because then another I-search
+  ;; used after `recursive-edit' might override them.
+  (let* ((isearch-regexp regexp-flag)
+	 (isearch-word delimited-flag)
+	 (isearch-lax-whitespace
+	  replace-lax-whitespace)
+	 (isearch-regexp-lax-whitespace
+	  replace-regexp-lax-whitespace)
+	 (isearch-case-fold-search case-fold-search)
+	 (isearch-adjusted nil)
+	 (isearch-nonincremental t)	; don't use lax word mode
+	 (isearch-forward t)
+	 (search-function
+	  (or (if regexp-flag
+		  replace-re-search-function
+		replace-search-function)
+	      (isearch-search-fun-default)))
+	 (retry t)
+	 (success nil))
+    ;; Use a loop like in `isearch-search'.
+    (while retry
+      (setq success (funcall search-function search-string limit t))
+      ;; Clear RETRY unless the search predicate says
+      ;; to skip this search hit.
+      (if (or (not success)
+	      (and (run-hook-with-args-until-failure
+		    'isearch-filter-predicates
+		    (match-beginning 0) (match-end 0))
+		   (or (eq search-invisible t)
+		       (not (isearch-range-invisible
+			     (match-beginning 0) (match-end 0))))
+		   ;; Optionally ignore matches that have a read-only property.
+		   (or (not query-replace-skip-read-only)
+		       (not (text-property-not-all
+			     (match-beginning 0) (match-end 0)
+			     'read-only nil)))))
+	  (setq retry nil)))
+    success))
+
 (defun perform-replace (from-string replacements
 		        query-flag regexp-flag delimited-flag
 			&optional repeat-count map start end)
@@ -1881,29 +1931,6 @@ (defun perform-replace (from-string repl
 	;; Loop finding occurrences that perhaps should be replaced.
 	(while (and keep-going
 		    (not (or (eobp) (and limit (>= (point) limit))))
-		    ;; Let-bind global isearch-* variables to values used
-		    ;; to search the next replacement.  These let-bindings
-		    ;; should be effective both at the time of calling
-		    ;; `isearch-search-fun-default' and also at the
-		    ;; time of funcalling `search-function'.
-		    ;; These isearch-* bindings can't be placed higher
-		    ;; outside of this loop because then another I-search
-		    ;; used after `recursive-edit' might override them.
-		    (let* ((isearch-regexp regexp-flag)
-			   (isearch-word delimited-flag)
-			   (isearch-lax-whitespace
-			    replace-lax-whitespace)
-			   (isearch-regexp-lax-whitespace
-			    replace-regexp-lax-whitespace)
-			   (isearch-case-fold-search case-fold-search)
-			   (isearch-adjusted nil)
-			   (isearch-nonincremental t) ; don't use lax word mode
-			   (isearch-forward t)
-			   (search-function
-			    (or (if regexp-flag
-				    replace-re-search-function
-				  replace-search-function)
-				(isearch-search-fun-default))))
 		      ;; Use the next match if it is already known;
 		      ;; otherwise, search for a match after moving forward
 		      ;; one char if progress is required.
@@ -1916,8 +1943,9 @@ (defun perform-replace (from-string repl
 				  ;; adjacent match.
 				  (match-again
 				   (and
-				    (funcall search-function search-string
-					     limit t)
+				  (replace-search search-string limit
+						  regexp-flag delimited-flag
+						  case-fold-search)
 				    ;; For speed, use only integers and
 				    ;; reuse the list used last time.
 				    (replace-match-data t real-match-data)))
@@ -1930,13 +1958,12 @@ (defun perform-replace (from-string repl
 				   ;; if the search fails.
 				   (let ((opoint (point)))
 				     (forward-char 1)
-				     (if (funcall
-					  search-function search-string
-					  limit t)
-					 (replace-match-data
-					  t real-match-data)
+				   (if (replace-search search-string limit
+						       regexp-flag delimited-flag
+						       case-fold-search)
+				       (replace-match-data t real-match-data)
 				       (goto-char opoint)
-				       nil)))))))
+				     nil))))))
 
 	  ;; Record whether the match is nonempty, to avoid an infinite loop
 	  ;; repeatedly matching the same empty string.
@@ -1957,13 +1984,6 @@ (defun perform-replace (from-string repl
 			      (let ((match (match-data)))
 				(and (/= (nth 0 match) (nth 1 match))
 				     match))))))
-
-	  ;; Optionally ignore matches that have a read-only property.
-	  (unless (and query-replace-skip-read-only
-		       (text-property-not-all
-			(nth 0 real-match-data) (nth 1 real-match-data)
-			'read-only nil))
-
 	    ;; Calculate the replacement string, if necessary.
 	    (when replacements
 	      (set-match-data real-match-data)
@@ -2168,7 +2188,7 @@ (defun perform-replace (from-string repl
 				 (match-end 0)
 				 (current-buffer))
 			      (match-data t)))
-		      stack)))))
+		    stack))))
 
       (replace-dehighlight))
     (or unread-command-events

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> > if you have `search-invisible' non-nil, isearch "opens" invisible
> > text.  But when you hit M-% or C-M-% while searching, you loose this
> > ability: point will just be put inside invisible areas, and you don't
> > see what you're doing.
>
> The recent adaptation of isearch functions to `perform-replace'
> makes it easy to implement this now.

This sounds great.

I have a question about your patch: I can't find the definition of
`isearch-filter-predicates' you use - I find `isearch-filter-predicate'
however.  Is this a typo, or did I miss something?


Thanks,

Michael

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Juri Linkov]

> I have a question about your patch: I can't find the definition of
> `isearch-filter-predicates' you use - I find `isearch-filter-predicate'
> however.  Is this a typo, or did I miss something?

Sorry, that was from an uncommitted patch from bug#11378
where I couldn't decide what is the best way to handle both
the variable `search-invisible' and the invisibility isearch filter
simultaneously.  I'll summarize the current state in bug#11378.

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Juri Linkov]

>> > if you have `search-invisible' non-nil, isearch "opens" invisible
>> > text.  But when you hit M-% or C-M-% while searching, you loose this
>> > ability: point will just be put inside invisible areas, and you don't
>> > see what you're doing.
>>
>> The recent adaptation of isearch functions to `perform-replace'
>> makes it easy to implement this now.
>
> This sounds great.

Sorry for the delay, it required more testing with more fixes
like adding `isearch-clean-overlays' to `replace-dehighlight', etc.
This is installed now.

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Juri Linkov]

It would be nice to inform the users who customized
`search-invisible' to nil that now `query-replace'
ignores hidden matches, i.e. currently at the end
`query-replace' reports in the echo area:

  Replaced 2 occurrences

Instead of keeping silence about skipped occurrences
a better message would be:

  Replaced 2 occurrences (skipped 3 read-only, 4 invisible, 5 filtered out)

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2013-05-27 23:38:56 +0000
+++ lisp/replace.el	2013-05-28 22:28:01 +0000
@@ -1934,6 +1965,9 @@ (defun perform-replace (from-string repl
          (keep-going t)
          (stack nil)
          (replace-count 0)
+         (skip-read-only-count 0)
+         (skip-filtered-count 0)
+         (skip-invisible-count 0)
          (nonempty-match nil)
 	 (multi-buffer nil)
 	 (recenter-last-op nil)	; Start cycling order with initial position.
@@ -2042,20 +2076,24 @@ (defun perform-replace (from-string repl
 				(and (/= (nth 0 match) (nth 1 match))
 				     match))))))
 
-	  ;; Optionally ignore matches that have a read-only property.
-	  (when (and (or (not query-replace-skip-read-only)
-			 (not (text-property-not-all
-			       (nth 0 real-match-data) (nth 1 real-match-data)
-			       'read-only nil)))
-		     ;; Optionally filter out matches.
-		     (run-hook-with-args-until-failure
-		      'isearch-filter-predicates
-		      (nth 0 real-match-data) (nth 1 real-match-data))
-		     ;; Optionally ignore invisible matches.
-		     (or (eq search-invisible t)
-			 (not (isearch-range-invisible
-			       (nth 0 real-match-data) (nth 1 real-match-data)))))
-
+	  (cond
+	   ;; Optionally ignore matches that have a read-only property.
+	   ((not (or (not query-replace-skip-read-only)
+		     (not (text-property-not-all
+			   (nth 0 real-match-data) (nth 1 real-match-data)
+			   'read-only nil))))
+	    (setq skip-read-only-count (1+ skip-read-only-count)))
+	   ;; Optionally filter out matches.
+	   ((not (run-hook-with-args-until-failure
+		  'isearch-filter-predicates
+		  (nth 0 real-match-data) (nth 1 real-match-data)))
+	    (setq skip-filtered-count (1+ skip-filtered-count)))
+	   ;; Optionally ignore invisible matches.
+	   ((not (or (eq search-invisible t)
+		     (not (isearch-range-invisible
+			   (nth 0 real-match-data) (nth 1 real-match-data)))))
+	    (setq skip-invisible-count (1+ skip-invisible-count)))
+	   (t
 	    ;; Calculate the replacement string, if necessary.
 	    (when replacements
 	      (set-match-data real-match-data)
@@ -2260,13 +2298,31 @@ (defun perform-replace (from-string repl
 				 (match-end 0)
 				 (current-buffer))
 			      (match-data t)))
-		      stack)))))
+		      stack))))))
 
       (replace-dehighlight))
     (or unread-command-events
-	(message "Replaced %d occurrence%s"
+	(message "Replaced %d occurrence%s%s"
 		 replace-count
-		 (if (= replace-count 1) "" "s")))
+		 (if (= replace-count 1) "" "s")
+		 (if (> (+ skip-read-only-count
+			   skip-filtered-count
+			   skip-invisible-count) 0)
+		     (format " (skipped %s)"
+			     (mapconcat
+			      'identity
+			      (delq nil (list
+					 (if (> skip-read-only-count 0)
+					     (format "%s read-only"
+						     skip-read-only-count))
+					 (if (> skip-invisible-count 0)
+					     (format "%s invisible"
+						     skip-invisible-count))
+					 (if (> skip-filtered-count 0)
+					     (format "%s filtered out"
+						     skip-filtered-count))))
+			      ", "))
+		   "")))
     (or (and keep-going stack) multi-buffer)))
 
 ;;; replace.el ends here

bug#11746: feature request: `isearch-query-replace' should open invisible text

[Juri Linkov]

Actually opening hidden overlays is not optimal when the user types `!'
for automatic replacement.  There is no need to open overlays and
immediately close afterwards without seeing opened text by the user.
This can be improved with this patch:

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2013-05-29 23:16:44 +0000
+++ lisp/replace.el	2013-05-30 00:00:39 +0000
@@ -2111,6 +2111,9 @@ (defun perform-replace (from-string repl
 	    (setq skip-filtered-count (1+ skip-filtered-count)))
 	   ;; Optionally ignore invisible matches.
 	   ((not (or (eq search-invisible t)
+		     ;; Don't open overlays for automatic replacements.
+		     (and (not query-flag) search-invisible)
+		     ;; Open hidden overlays for interactive replacements.
 		     (not (isearch-range-invisible
 			   (nth 0 real-match-data) (nth 1 real-match-data)))))
 	    (setq skip-invisible-count (1+ skip-invisible-count)))