bug#6227: Color isearch regexp submatches differently

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

Just a suggestion, of course.

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

Did you mean something like this (attached)?
This is how I highlight submatches in Icicles search.

(The top part of the image, with light blue background, shows the highlighting.
The bottom part of the image, with white background, shows the regexp used and
is just an explanation of the subgroup highlighting.)


[-- Attachment #2: drew-emacs-icicle-search-context-colors.png --]
[-- Type: image/png, Size: 11424 bytes --]

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Thu, May 20, 2010 at 3:21 PM, Drew Adams <drew.adams@oracle.com> wrote:
> Did you mean something like this (attached)?
> This is how I highlight submatches in Icicles search.
>
> (The top part of the image, with light blue background, shows the highlighting.
> The bottom part of the image, with white background, shows the regexp used and
> is just an explanation of the subgroup highlighting.)

Yes, exactly.

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

> > Did you mean something like this (attached)?
> > This is how I highlight submatches in Icicles search.
> >
> > (The top part of the image, with light blue background, 
> > shows the highlighting. The bottom part of the image,
> > with white background, shows the regexp used and
> > is just an explanation of the subgroup highlighting.)
> 
> Yes, exactly.

IMO, this can be very helpful when searching with regexps.
And it can help users learn about using regexps more generally.

Of course, such highlighting should be optional, and preferably via a toggle
during Isearch.

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

> > Yes, exactly.
> 
> IMO, this can be very helpful when searching with regexps.
> And it can help users learn about using regexps more generally.
> 
> Of course, such highlighting should be optional, and 
> preferably via a toggle
> during Isearch.

Need I add that this need not be shown for all search hits simultaneously
(costly and distracting to the user, in general). Just the current hit is
sufficient (what is shown now using face `isearch', not `lazy-highlight').

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

> Just a suggestion, of course.

We already have highlighting like that: lisp/emacs-lisp/re-builder.el
uses faces `reb-match-1', `reb-match-2', `reb-match-3' to highlight
regexp subexpressions.  I think this should be used by isearch.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Fri, May 21, 2010 at 2:07 AM, Juri Linkov <juri@jurta.org> wrote:
>> Just a suggestion, of course.
>
> We already have highlighting like that: lisp/emacs-lisp/re-builder.el
> uses faces `reb-match-1', `reb-match-2', `reb-match-3' to highlight
> regexp subexpressions.  I think this should be used by isearch.

That sounds right to me.

Also Drew suggestion to not color submatches in lazy marking seems right.

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

>> We already have highlighting like that: lisp/emacs-lisp/re-builder.el
>> uses faces `reb-match-1', `reb-match-2', `reb-match-3' to highlight
>> regexp subexpressions.  I think this should be used by isearch.
>
> That sounds right to me.
>
> Also Drew suggestion to not color submatches in lazy marking seems right.

(add-hook 'isearch-update-post-hook
          (lambda ()
	    (require 're-builder)
	    (when isearch-regexp
	      (let ((reb-regexp isearch-string)
		    (reb-target-buffer (current-buffer))
		    (reb-target-window (selected-window)))
		(reb-update-overlays)))))

(add-hook 'isearch-mode-end-hook
          (lambda ()
	    (let ((reb-target-buffer (current-buffer)))
	      (reb-delete-overlays))))

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Sun, May 23, 2010 at 1:44 AM, Juri Linkov <juri@jurta.org> wrote:
>>> We already have highlighting like that: lisp/emacs-lisp/re-builder.el
>>> uses faces `reb-match-1', `reb-match-2', `reb-match-3' to highlight
>>> regexp subexpressions.  I think this should be used by isearch.
>>
>> That sounds right to me.
>>
>> Also Drew suggestion to not color submatches in lazy marking seems right.
>
> (add-hook 'isearch-update-post-hook
>          (lambda ()
>            (require 're-builder)
>            (when isearch-regexp
>              (let ((reb-regexp isearch-string)
>                    (reb-target-buffer (current-buffer))
>                    (reb-target-window (selected-window)))
>                (reb-update-overlays)))))
>
> (add-hook 'isearch-mode-end-hook
>          (lambda ()
>            (let ((reb-target-buffer (current-buffer)))
>              (reb-delete-overlays))))


Nice. So I suggest moving (and renaming) `reb-count-subexps' to
isearch.el and splitting off the marking of one overlay from
`reb-update-overlays' and moving that too to isearch.el (since
isearch.el) is probably always loaded for a normal Emacs user).

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

> Nice. So I suggest moving (and renaming) `reb-count-subexps' to
> isearch.el and splitting off the marking of one overlay from
> `reb-update-overlays' and moving that too to isearch.el (since
> isearch.el) is probably always loaded for a normal Emacs user).

I think `reb-update-overlays' should be completely rewritten
for isearch.el.

The only thing we need from re-builder.el are faces
reb-match-1, reb-match-2, reb-match-3.  We should try
using the existing faces for the same functionality.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Sun, May 23, 2010 at 2:54 AM, Juri Linkov <juri@jurta.org> wrote:
>> Nice. So I suggest moving (and renaming) `reb-count-subexps' to
>> isearch.el and splitting off the marking of one overlay from
>> `reb-update-overlays' and moving that too to isearch.el (since
>> isearch.el) is probably always loaded for a normal Emacs user).
>
> I think `reb-update-overlays' should be completely rewritten
> for isearch.el.


You surely know this things much better than me, but is there any
reason to double the code? If it is rewritten why not let re-builder
share the same code?


> The only thing we need from re-builder.el are faces
> reb-match-1, reb-match-2, reb-match-3.  We should try
> using the existing faces for the same functionality.

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

>> I think `reb-update-overlays' should be completely rewritten
>> for isearch.el.
>
> You surely know this things much better than me, but is there any
> reason to double the code?

`reb-update-overlays' highlights all matches in the buffer.
This is like what lazy-highlighting does.  But we agreed
that it should affect only the current isearch match,
not all lazy-highlighted matches.

> If it is rewritten why not let re-builder share the same code?

Yes, and query-replace highlighting could share it too.

>> The only thing we need from re-builder.el are faces
>> reb-match-1, reb-match-2, reb-match-3.  We should try
>> using the existing faces for the same functionality.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Sun, May 23, 2010 at 6:12 PM, Juri Linkov <juri@jurta.org> wrote:
>>> I think `reb-update-overlays' should be completely rewritten
>>> for isearch.el.
>>
>> You surely know this things much better than me, but is there any
>> reason to double the code?
>
> `reb-update-overlays' highlights all matches in the buffer.
> This is like what lazy-highlighting does.  But we agreed
> that it should affect only the current isearch match,
> not all lazy-highlighted matches.
>
>> If it is rewritten why not let re-builder share the same code?
>
> Yes, and query-replace highlighting could share it too.


I see. A misunderstanding, we mean the same. I wrote the
reb-update-overlays should be split and I meant then into one function
that hilights only one match (which is given as a parameter) and one
that loops for reb-update-overlays.


>>> The only thing we need from re-builder.el are faces
>>> reb-match-1, reb-match-2, reb-match-3.  We should try
>>> using the existing faces for the same functionality.
>
> --
> Juri Linkov
> http://www.jurta.org/emacs/
>

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

On Sun, May 23, 2010 at 6:40 PM, Lennart Borgman
<lennart.borgman@gmail.com> wrote:
> On Sun, May 23, 2010 at 6:12 PM, Juri Linkov <juri@jurta.org> wrote:
>>>> I think `reb-update-overlays' should be completely rewritten
>>>> for isearch.el.
>>>
>>> You surely know this things much better than me, but is there any
>>> reason to double the code?
>>
>> `reb-update-overlays' highlights all matches in the buffer.
>> This is like what lazy-highlighting does.  But we agreed
>> that it should affect only the current isearch match,
>> not all lazy-highlighted matches.
>>
>>> If it is rewritten why not let re-builder share the same code?
>>
>> Yes, and query-replace highlighting could share it too.
>
>>>> The only thing we need from re-builder.el are faces
>>>> reb-match-1, reb-match-2, reb-match-3.  We should try
>>>> using the existing faces for the same functionality.

Here is a patch for the submatches highlighting. (It includes a bug
fix for the prompt face too and a help window scrolling I think is
useful.)

The current faces does not look very well together so that must be fixed.

[-- Attachment #2: isearch-hisub-1.diff --]
[-- Type: text/x-patch, Size: 6077 bytes --]

=== modified file 'lisp/isearch.el'
--- trunk/lisp/isearch.el	2010-05-20 22:33:09 +0000
+++ patched/lisp/isearch.el	2010-06-08 13:28:37 +0000
@@ -223,6 +223,12 @@
   :type 'boolean
   :group 'isearch)
 
+(defcustom search-highlight-submatches t
+  "Non-nil means incremental search highlights submatches.
+This is only done for the current hit."
+  :type 'boolean
+  :group 'isearch)
+
 (defface isearch
   '((((class color) (min-colors 88) (background light))
      ;; The background must not be too dark, for that means
@@ -1911,6 +1917,18 @@
 	  ((eq search-exit-option 'edit)
 	   (apply 'isearch-unread keylist)
 	   (isearch-edit-string))
+          ;; Always scroll other window if help buffer
+          ((let ((binding (key-binding key))
+                 other-buffer-is-help)
+             (when (or (eq binding 'scroll-other-window-down)
+                       (eq binding 'scroll-other-window))
+               (save-selected-window
+                 (other-window 1)
+                 (setq other-buffer-is-help (equal (buffer-name) "*Help*")))
+               (when other-buffer-is-help
+                 (command-execute binding)
+                 (isearch-update)
+                 t))))
           ;; Handle a scrolling function.
           ((and isearch-allow-scroll
                 (progn (setq key (isearch-reread-key-sequence-naturally keylist))
@@ -2182,9 +2200,12 @@
 		   (if current-input-method
 		       (concat " [" current-input-method-title "]: ")
 		     ": ")
-		   )))
-    (propertize (concat (upcase (substring m 0 1)) (substring m 1))
-		'face 'minibuffer-prompt)))
+                   ))
+        m2)
+    (setq m2 (apply 'propertize
+                    (concat (upcase (substring m 0 1)) (substring m 1))
+                    minibuffer-prompt-properties))
+    (propertize m2 'read-only nil)))
 
 (defun isearch-message-suffix (&optional c-q-hack ellipsis)
   (concat (if c-q-hack "^Q" "")
@@ -2526,9 +2547,80 @@
 ;; Highlighting
 
 (defvar isearch-overlay nil)
+(defvar isearch-submatches-overlays nil)
+
+(defun isearch-count-subexps (re)
+  "Return max possible subexp number for the regexp RE."
+  (save-match-data
+    (let ((i 0) (beg 0) (max-n 0))
+      ;;(while (string-match "\\\\(" re beg)
+      ;; (string-match "\\\\(" "")
+      ;; (string-match "\\\\(\\(\?[0-9]+:\\)?" "")
+      ;; (string-match "\\\\(\\(\\?[0-9]+:\\)?" "")
+      ;; (string-match "\\\\(\\(\\?[0-9]+:\\)?" "\\(?9:\\)")
+      (while (string-match "\\\\(\\(\\?[0-9]+:\\)?" re beg)
+        (setq i (1+ (max max-n i)))
+        (setq beg (match-end 0))
+        (let ((sub (match-string-no-properties 1 re)))
+          (when sub
+            (setq sub (substring sub 1))
+            (setq max-n (max max-n (string-to-number sub))))))
+      (max max-n i))))
+
+(defun isearch-unhighlight-submatches ()
+  (dolist (subovl isearch-submatches-overlays)
+    (delete-overlay subovl)))
+
+(defvar isearch-submatch-count nil) ;; For rebuilder
+(defvar isearch-subexp-to-mark nil
+  "If non-nil mark only the corresponding submatch.
+This variable must be nil or a positive integer.")
+
+(defun isearch-highlight-submatches ()
+  (isearch-unhighlight-submatches)
+  (setq isearch-submatches-overlays nil)
+  (when search-highlight-submatches
+    (require 're-builder) ;; fix-me
+    (let ((subexps (isearch-count-subexps isearch-string))
+          (subexp isearch-subexp-to-mark)
+          (submatches 0)
+          (ii 1)
+          suffix max-suffix)
+      (while (<= ii subexps)
+        (when (and (or (not subexp) (= subexp ii))
+                   (match-beginning ii))
+          (let ((overlay (make-overlay (match-beginning ii)
+                                       (match-end ii)))
+                ;; When we have exceeded the number of provided faces,
+                ;; cycle thru them where `max-suffix' denotes the maximum
+                ;; suffix for `reb-match-*' that has been defined and
+                ;; `suffix' the suffix calculated for the current match.
+                (face
+                 (cond
+                  (max-suffix
+                   (if (= suffix max-suffix)
+                       (setq suffix 1)
+                     (setq suffix (1+ suffix)))
+                   (intern-soft (format "reb-match-%d" suffix)))
+                  ((intern-soft (format "reb-match-%d" ii)))
+                  ((setq max-suffix (1- ii))
+                   (setq suffix 1)
+                   ;; `reb-match-1' must exist.
+                   'reb-match-1))))
+            ;; (unless firstmatch (setq firstmatch (match-data)))
+            ;; (unless firstmatch-after-here
+            ;;   (when (> (point) here)
+            ;;     (setq firstmatch-after-here (match-data))))
+            (setq isearch-submatches-overlays
+                  (cons overlay isearch-submatches-overlays))
+            (setq submatches (1+ submatches))
+            (overlay-put overlay 'face face)
+            ;; Priority must be higher than isearch base overlay.
+            (overlay-put overlay 'priority (+ ii 1001))))
+        (setq ii (1+ ii))))))
 
 (defun isearch-highlight (beg end)
-  (if search-highlight
+  (when search-highlight
       (if isearch-overlay
 	  ;; Overlay already exists, just move it.
 	  (move-overlay isearch-overlay beg end (current-buffer))
@@ -2536,11 +2628,14 @@
 	(setq isearch-overlay (make-overlay beg end))
 	;; 1001 is higher than lazy's 1000 and ediff's 100+
 	(overlay-put isearch-overlay 'priority 1001)
-	(overlay-put isearch-overlay 'face isearch))))
+      (overlay-put isearch-overlay 'face isearch))
+    (when isearch-regexp
+      (isearch-highlight-submatches))))
 
 (defun isearch-dehighlight ()
   (when isearch-overlay
-    (delete-overlay isearch-overlay)))
+    (delete-overlay isearch-overlay))
+  (isearch-unhighlight-submatches))
 \f
 ;; isearch-lazy-highlight feature
 ;; by Bob Glickstein <http://www.zanshin.com/~bobg/>

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

> Here is a patch for the submatches highlighting.
> (It includes a bug fix for the prompt face too

What's a bug in the prompt face?

> and a help window scrolling I think is useful.)

Please provide an example of the scrolling bug too.

> The current faces does not look very well together so that must be fixed.

If current faces does not look well, then maybe we should completely
get rid of using re-builder.el in isearch, its faces and messy functions
like count-subexps, and to write this functionality for isearch from scratch.

Do you think something more complicated is necessary for this
functionality than the following simple code:

(defvar isearch-sub-overlays nil)
(add-hook 'isearch-update-post-hook
          (lambda ()
            ;; This code could be added to `isearch-highlight'.
            (mapc 'delete-overlay isearch-sub-overlays)
            (setq isearch-sub-overlays nil)
            (when isearch-regexp
              (dolist (i '(1 2 3 4))
                (when (match-beginning i)
                  (let ((ov (make-overlay (match-beginning i) (match-end i))))
                    (overlay-put ov 'face (intern-soft (format "isearch-%d" i)))
                    (overlay-put ov 'priority 1002)
                    (push ov isearch-sub-overlays)))))))

It relies on new faces `isearch-1', `isearch-2', `isearch-3', `isearch-4'.
As for face colors, I tried "magenta1", "magenta2", "magenta3", "magenta4"
for background colors, and they look good.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Wed, Jun 9, 2010 at 10:36 AM, Juri Linkov <juri@jurta.org> wrote:
>> Here is a patch for the submatches highlighting.
>> (It includes a bug fix for the prompt face too
>
> What's a bug in the prompt face?


There is a variable, minibuffer-prompt-properties, that holds the name
of the face to use. I think this should be used for consistency. It
makes it much easier for users.


>> and a help window scrolling I think is useful.)
>
> Please provide an example of the scrolling bug too.


I am not sure on that one. Maybe I just forgot to remove it when you
implemented your way of doing it?


>> The current faces does not look very well together so that must be fixed.
>
> If current faces does not look well, then maybe we should completely
> get rid of using re-builder.el in isearch, its faces and messy functions
> like count-subexps, and to write this functionality for isearch from scratch.


I thought maybe something like count-subexps is needed now with the
numbered submatches.


> Do you think something more complicated is necessary for this
> functionality than the following simple code:
>
> (defvar isearch-sub-overlays nil)
> (add-hook 'isearch-update-post-hook
>          (lambda ()
>            ;; This code could be added to `isearch-highlight'.
>            (mapc 'delete-overlay isearch-sub-overlays)
>            (setq isearch-sub-overlays nil)
>            (when isearch-regexp
>              (dolist (i '(1 2 3 4))
>                (when (match-beginning i)
>                  (let ((ov (make-overlay (match-beginning i) (match-end i))))
>                    (overlay-put ov 'face (intern-soft (format "isearch-%d" i)))
>                    (overlay-put ov 'priority 1002)
>                    (push ov isearch-sub-overlays)))))))


It does not take care of numbered matches. But, yes, why not guess? I
agree, your approach is probably better. But check for more
submatches. Maybe upto the value of some variable, say
isearch-max-submatch-num.


> It relies on new faces `isearch-1', `isearch-2', `isearch-3', `isearch-4'.
> As for face colors, I tried "magenta1", "magenta2", "magenta3", "magenta4"
> for background colors, and they look good.


The problem with mixing isearch faces with re-builder dito was the
resulting colors from merging. If it works then just use your
suggestions.

I  have rewritten re-builder.el and got rid of its internal. Just need
to cleanup a bit. Now it is just a front end to isearch with more
editing capabilities, like rx. I think that can be useful.

I plan to keep three "regexp source styles" there and maybe rename
them a bit: regexp, string (or maybe read) and rx. I think those names
are self explanatory. Unfortunately re-builder now uses string/read
instead of regexp/string which is more user-level names.

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

>>> (It includes a bug fix for the prompt face too
>>
>> What's a bug in the prompt face?
>
> There is a variable, minibuffer-prompt-properties, that holds the name
> of the face to use. I think this should be used for consistency. It
> makes it much easier for users.

Why do you remove `read-only' with `(propertize m2 'read-only nil)'?
What was a problem with `read-only' in the isearch prompt?

>>> and a help window scrolling I think is useful.)
>>
>> Please provide an example of the scrolling bug too.
>
> I am not sure on that one. Maybe I just forgot to remove it when you
> implemented your way of doing it?

I see no scrolling problems after setting `isearch-allow-scroll' to t.

> I agree, your approach is probably better. But check for more
> submatches. Maybe upto the value of some variable, say
> isearch-max-submatch-num.

Good idea.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Thu, Jun 10, 2010 at 5:28 PM, Juri Linkov <juri@jurta.org> wrote:
>>>> (It includes a bug fix for the prompt face too
>>>
>>> What's a bug in the prompt face?
>>
>> There is a variable, minibuffer-prompt-properties, that holds the name
>> of the face to use. I think this should be used for consistency. It
>> makes it much easier for users.
>
> Why do you remove `read-only' with `(propertize m2 'read-only nil)'?
> What was a problem with `read-only' in the isearch prompt?

Hm, can't remember. I did this quite a while ago. I just tested and it
seems to work without that. Probably it is something I forgot to
remove after testing.

>>>> and a help window scrolling I think is useful.)
>>>
>>> Please provide an example of the scrolling bug too.
>>
>> I am not sure on that one. Maybe I just forgot to remove it when you
>> implemented your way of doing it?
>
> I see no scrolling problems after setting `isearch-allow-scroll' to t.

Seems that you are right. Fine, just skip that part of the patch.

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

>> I agree, your approach is probably better. But check for more
>> submatches. Maybe upto the value of some variable, say
>> isearch-max-submatch-num.
>
> Good idea.

Maybe something like this:

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2010-06-10 14:32:41 +0000
+++ lisp/isearch.el	2010-06-10 20:51:43 +0000
@@ -223,6 +223,15 @@ (defcustom search-highlight t
   :type 'boolean
   :group 'isearch)
 
+(defcustom search-highlight-submatches 0
+  "Highlight regexp subexpressions of the current regexp match.
+An integer means highlight regexp subexpressions up to the
+specified maximal number.
+When 0, do not highlight regexp subexpressions."
+  :type 'integer
+  :version "24.1"
+  :group 'isearch)
+
 (defface isearch
   '((((class color) (min-colors 88) (background light))
      ;; The background must not be too dark, for that means
@@ -2526,6 +2535,23 @@ (defun isearch-unread (&rest char-or-eve
 ;; Highlighting
 
 (defvar isearch-overlay nil)
+(defvar isearch-submatches-overlays nil)
+
+(defface isearch-1
+  '((((class color) (min-colors 88) (background light))
+     :background "magenta2" :foreground "lightskyblue1")
+    (((class color) (min-colors 88) (background dark))
+     :background "palevioletred3" :foreground "brown4"))
+  "Used for displaying the first matching subexpression."
+  :group 'isearch)
+
+(defface isearch-2
+  '((((class color) (min-colors 88) (background light))
+     :background "magenta1" :foreground "lightskyblue1")
+    (((class color) (min-colors 88) (background dark))
+     :background "palevioletred4" :foreground "brown4"))
+  "Used for displaying the second matching subexpression."
+  :group 'isearch)
 
 (defun isearch-highlight (beg end)
   (if search-highlight
@@ -2536,11 +2562,28 @@ (defun isearch-highlight (beg end)
 	(setq isearch-overlay (make-overlay beg end))
 	;; 1001 is higher than lazy's 1000 and ediff's 100+
 	(overlay-put isearch-overlay 'priority 1001)
-	(overlay-put isearch-overlay 'face isearch))))
+	(overlay-put isearch-overlay 'face isearch)))
+  (when (and (integerp search-highlight-submatches)
+	     (> search-highlight-submatches 0)
+	     isearch-regexp)
+    (mapc 'delete-overlay isearch-submatches-overlays)
+    (setq isearch-submatches-overlays nil)
+    (let ((i 0) ov)
+      (while (<= i search-highlight-submatches)
+	(when (match-beginning i)
+	  (setq ov (make-overlay (match-beginning i) (match-end i)))
+	  (overlay-put ov 'face (intern-soft (format "isearch-%d" i)))
+	  (overlay-put ov 'priority 1002)
+	  (push ov isearch-submatches-overlays))
+	(setq i (1+ i))))))
 
 (defun isearch-dehighlight ()
   (when isearch-overlay
-    (delete-overlay isearch-overlay)))
+    (delete-overlay isearch-overlay))
+  (when search-highlight-submatches
+    (mapc 'delete-overlay isearch-submatches-overlays)
+    (setq isearch-submatches-overlays nil)))
+
 \f
 ;; isearch-lazy-highlight feature
 ;; by Bob Glickstein <http://www.zanshin.com/~bobg/>

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lennart Borgman]

On Thu, Jun 10, 2010 at 10:52 PM, Juri Linkov <juri@jurta.org> wrote:
>>> I agree, your approach is probably better. But check for more
>>> submatches. Maybe upto the value of some variable, say
>>> isearch-max-submatch-num.
>>
>> Good idea.
>
> Maybe something like this:

Yes,

> +(defcustom search-highlight-submatches 0

but set I suggest a default of 100. The loop costs essentially nothing
for non-submatches and this is on command level.

> +    (let ((i 0) ov)
> +      (while (<= i search-highlight-submatches)
> +       (when (match-beginning i)
> +         (setq ov (make-overlay (match-beginning i) (match-end i)))
> +         (overlay-put ov 'face (intern-soft (format "isearch-%d" i)))
> +         (overlay-put ov 'priority 1002)
> +         (push ov isearch-submatches-overlays))
> +       (setq i (1+ i))))))

bug#6227: Color isearch regexp submatches differently

[Stefan Monnier]

> but set I suggest a default of 100. The loop costs essentially nothing
> for non-submatches and this is on command level.

(/ (length (match-data)) 2)


        Stefan

bug#6227: Color isearch regexp submatches differently

[Juri Linkov]

>> but set I suggest a default of 100. The loop costs essentially nothing
>> for non-submatches and this is on command level.
>
> (/ (length (match-data)) 2)

Then the loop should iterate from 1 up to the minimum of
`(/ (length (match-data)) 2)' and `search-highlight-submatches'.

As for the faces, perhaps we should dynamically create submatch faces
like in `vc-annotate-lines' that uses `vc-annotate-color-map'.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#6227: Color isearch regexp submatches differently

[Lars Ingebrigtsen]

Juri Linkov <juri@jurta.org> writes:

>>> I agree, your approach is probably better. But check for more
>>> submatches. Maybe upto the value of some variable, say
>>> isearch-max-submatch-num.
>>
>> Good idea.
>
> Maybe something like this:

The ten year old patch no longer applied to Emacs 28, so I've respun it.

I think the results are really nice.  I guess we should add a few more
faces before applying?

Anybody else got any comments on this?

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 7fb1d8a3ca..56eb443d31 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -269,6 +269,14 @@ search-highlight
   "Non-nil means incremental search highlights the current match."
   :type 'boolean)
 
+(defcustom search-highlight-submatches 2
+  "Highlight regexp subexpressions of the current regexp match.
+An integer means highlight regexp subexpressions up to the
+specified maximal number.
+When 0, do not highlight regexp subexpressions."
+  :type 'integer
+  :version "28.1")
+
 (defface isearch
   '((((class color) (min-colors 88) (background light))
      ;; The background must not be too dark, for that means
@@ -3654,6 +3662,21 @@ isearch-unread
 ;; Highlighting
 
 (defvar isearch-overlay nil)
+(defvar isearch-submatches-overlays nil)
+
+(defface isearch-1
+  '((((class color) (min-colors 88) (background light))
+     :background "magenta2" :foreground "lightskyblue1")
+    (((class color) (min-colors 88) (background dark))
+     :background "palevioletred3" :foreground "brown4"))
+  "Used for displaying the first matching subexpression.")
+
+(defface isearch-2
+  '((((class color) (min-colors 88) (background light))
+     :background "magenta1" :foreground "lightskyblue1")
+    (((class color) (min-colors 88) (background dark))
+     :background "palevioletred4" :foreground "brown4"))
+  "Used for displaying the second matching subexpression.")
 
 (defun isearch-highlight (beg end)
   (if search-highlight
@@ -3664,11 +3687,28 @@ isearch-highlight
 	(setq isearch-overlay (make-overlay beg end))
 	;; 1001 is higher than lazy's 1000 and ediff's 100+
 	(overlay-put isearch-overlay 'priority 1001)
-	(overlay-put isearch-overlay 'face isearch-face))))
+	(overlay-put isearch-overlay 'face isearch-face)))
+  (when (and (integerp search-highlight-submatches)
+	     (> search-highlight-submatches 0)
+	     isearch-regexp)
+    (mapc 'delete-overlay isearch-submatches-overlays)
+    (setq isearch-submatches-overlays nil)
+    (let ((i 0) ov)
+      (while (<= i search-highlight-submatches)
+	(when (match-beginning i)
+	  (setq ov (make-overlay (match-beginning i) (match-end i)))
+	  (overlay-put ov 'face (intern-soft (format "isearch-%d" i)))
+	  (overlay-put ov 'priority 1002)
+	  (push ov isearch-submatches-overlays))
+	(setq i (1+ i))))))
 
 (defun isearch-dehighlight ()
   (when isearch-overlay
-    (delete-overlay isearch-overlay)))
+    (delete-overlay isearch-overlay))
+  (when search-highlight-submatches
+    (mapc 'delete-overlay isearch-submatches-overlays)
+    (setq isearch-submatches-overlays nil)))
+
 \f
 ;; isearch-lazy-highlight feature
 ;; by Bob Glickstein <http://www.zanshin.com/~bobg/>

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

> I guess we should add a few more faces before applying?
> 
> Anybody else got any comments on this?

Haven't tried the patch.  But Isearch+ does this.
It uses 8 faces, for the first 8 regexp levels
(groups).

(The code could be go further, repeating those 8
faces if needed for more groups, but I haven't found
that that's needed.)

For lazy-highlighting, the odd groups are highlighted
differently from the even groups.  The odd groups use
face `isearchp-lazy-odd-regexp-groups'.  That simple
lazy highlighting actually helps quite a bit, I find.  

There is also a user option for whether to highlight
groups: `isearchp-highlight-regexp-group-levels-flag'.
And a toggle command for that option: `M-s h R'
(prefix `M-s' is for all Isearch stuff, `h' is for
highlighting, and `R' is for regexp groups).

bug#6227: Color isearch regexp submatches differently

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 19 Sep 2020 23:29:48 +0200
> Cc: Lennart Borgman <lennart.borgman@gmail.com>, 6227@debbugs.gnu.org
> 
> I think the results are really nice.  I guess we should add a few more
> faces before applying?
> 
> Anybody else got any comments on this?

Comment #1: Do we really want to turn this feature on by default?

> +(defface isearch-1
> +  '((((class color) (min-colors 88) (background light))
> +     :background "magenta2" :foreground "lightskyblue1")
> +    (((class color) (min-colors 88) (background dark))
> +     :background "palevioletred3" :foreground "brown4"))
> +  "Used for displaying the first matching subexpression.")
> +
> +(defface isearch-2
> +  '((((class color) (min-colors 88) (background light))
> +     :background "magenta1" :foreground "lightskyblue1")
> +    (((class color) (min-colors 88) (background dark))
> +     :background "palevioletred4" :foreground "brown4"))
> +  "Used for displaying the second matching subexpression.")

Comment #2: This seems to effectively disable the feature on displays
that have fewer than 88 colors.  Is that intentional?  If so, why
doesn't the documentation say so?

Comment #3: What about NEWS and the manual?

Thanks.

bug#6227: Color isearch regexp submatches differently

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Anybody else got any comments on this?
>
> Comment #1: Do we really want to turn this feature on by default?

I don't see why not.  It's just a more detailed visualisation.

>> +(defface isearch-2
>> +  '((((class color) (min-colors 88) (background light))
>> +     :background "magenta1" :foreground "lightskyblue1")
>> +    (((class color) (min-colors 88) (background dark))
>> +     :background "palevioletred4" :foreground "brown4"))
>> +  "Used for displaying the second matching subexpression.")
>
> Comment #2: This seems to effectively disable the feature on displays
> that have fewer than 88 colors.  Is that intentional?  If so, why
> doesn't the documentation say so?

I'm guessing it's just cargo-culting off of the isearch face:

(defface isearch
  '((((class color) (min-colors 88) (background light))

> Comment #3: What about NEWS and the manual?

Those would have to be added, of course.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6227: Color isearch regexp submatches differently

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: juri@jurta.org,  lennart.borgman@gmail.com,  6227@debbugs.gnu.org
> Date: Sun, 20 Sep 2020 11:41:10 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Anybody else got any comments on this?
> >
> > Comment #1: Do we really want to turn this feature on by default?
> 
> I don't see why not.  It's just a more detailed visualisation.

But with slightly different colors.  How do we know long-time users of
Isearch will understand what they mean, unless they deliberately turn
on this option?

> > Comment #2: This seems to effectively disable the feature on displays
> > that have fewer than 88 colors.  Is that intentional?  If so, why
> > doesn't the documentation say so?
> 
> I'm guessing it's just cargo-culting off of the isearch face:
> 
> (defface isearch
>   '((((class color) (min-colors 88) (background light))

I don't understand: the 'isearch' face has definitions for all kinds
of displays, even for monochrome ones:

  (defface isearch
    '((((class color) (min-colors 88) (background light))
       ;; The background must not be too dark, for that means
       ;; the character is hard to see when the cursor is there.
       (:background "magenta3" :foreground "lightskyblue1"))
      (((class color) (min-colors 88) (background dark))
       (:background "palevioletred2" :foreground "brown4"))
      (((class color) (min-colors 16))
       (:background "magenta4" :foreground "cyan1"))
      (((class color) (min-colors 8))
       (:background "magenta4" :foreground "cyan1"))
      (t (:inverse-video t)))

bug#6227: Color isearch regexp submatches differently

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I don't see why not.  It's just a more detailed visualisation.
>
> But with slightly different colors.  How do we know long-time users of
> Isearch will understand what they mean, unless they deliberately turn
> on this option?

When they're typing "foo\(bar\)" in regexp isearch and see the different
colours, I think they'll catch on pretty quickly.

>> > Comment #2: This seems to effectively disable the feature on displays
>> > that have fewer than 88 colors.  Is that intentional?  If so, why
>> > doesn't the documentation say so?
>> 
>> I'm guessing it's just cargo-culting off of the isearch face:
>> 
>> (defface isearch
>>   '((((class color) (min-colors 88) (background light))
>
> I don't understand: the 'isearch' face has definitions for all kinds
> of displays, even for monochrome ones:
>
>   (defface isearch
>     '((((class color) (min-colors 88) (background light))
>        ;; The background must not be too dark, for that means
>        ;; the character is hard to see when the cursor is there.
>        (:background "magenta3" :foreground "lightskyblue1"))

I meant that these deffaces aren't complete -- they look like examples,
and the examples are taken from the isearch face.

There should be more of them, and they should be complete, and they
should have better names (isearch-group-1 etc perhaps).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6227: Color isearch regexp submatches differently

[Lars Ingebrigtsen]

Now pushed.  The faces should be legible both on dark and light
backgrounds, but they aren't very...  pretty.  Feel free to tweak them.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6227: Color isearch regexp submatches differently

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: juri@jurta.org,  lennart.borgman@gmail.com,  6227@debbugs.gnu.org
> Date: Sun, 20 Sep 2020 15:47:13 +0200
> 
> Now pushed.  The faces should be legible both on dark and light
> backgrounds, but they aren't very...  pretty.  Feel free to tweak them.  :-)

What bothers me is that faces isearch-group-6 to isearch-group-9 are
not defined.  Should we define them?

bug#6227: Color isearch regexp submatches differently

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: juri@jurta.org,  lennart.borgman@gmail.com,  6227@debbugs.gnu.org
>> Date: Sun, 20 Sep 2020 15:47:13 +0200
>> 
>> Now pushed.  The faces should be legible both on dark and light
>> backgrounds, but they aren't very...  pretty.  Feel free to tweak them.  :-)
>
> What bothers me is that faces isearch-group-6 to isearch-group-9 are
> not defined.  Should we define them?

I'd rather go the other way, to be honest -- just leave group-1 to -3,
perhaps.  I think it's rather unusual to have a that many sub-groups
interactively...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

> > What bothers me is that faces isearch-group-6 to isearch-group-9 are
> > not defined.  Should we define them?
> 
> I'd rather go the other way, to be honest -- just leave group-1 to -3,
> perhaps.  I think it's rather unusual to have a that many sub-groups
> interactively...

It is unusual, as in not so common.  But when you do
have a complex regexp with many groups, that's exactly
when such highlighting can be most useful.

I settled on 8 levels, for Isearch+, but that was
without any special research into the question.

It doesn't hurt, in any way that I'm aware of, to
have more rather than less.  In the case where many
would actually be used (which, as you say, is not
usual), a little more time would be needed, but it's
likely that in such a complex case there would not
be many matches to highlight.  I think any time cost
of such highlighting is negligible.

bug#6227: Color isearch regexp submatches differently

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: juri@jurta.org,  lennart.borgman@gmail.com,  6227@debbugs.gnu.org
> Date: Sun, 20 Sep 2020 21:45:38 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Lars Ingebrigtsen <larsi@gnus.org>
> >> Cc: juri@jurta.org,  lennart.borgman@gmail.com,  6227@debbugs.gnu.org
> >> Date: Sun, 20 Sep 2020 15:47:13 +0200
> >> 
> >> Now pushed.  The faces should be legible both on dark and light
> >> backgrounds, but they aren't very...  pretty.  Feel free to tweak them.  :-)
> >
> > What bothers me is that faces isearch-group-6 to isearch-group-9 are
> > not defined.  Should we define them?
> 
> I'd rather go the other way, to be honest -- just leave group-1 to -3,
> perhaps.  I think it's rather unusual to have a that many sub-groups
> interactively...

That's true, but the problem which bothers me is that customizing
search-highlight-submatches alone to a larger value is not enough.  I
don't think any other customization we have requires users to create
faces or similar objects.

bug#6227: Color isearch regexp submatches differently

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> That's true, but the problem which bothers me is that customizing
> search-highlight-submatches alone to a larger value is not enough.  I
> don't think any other customization we have requires users to create
> faces or similar objects.

That's true...  OK, I'll just add the remaining four faces, and then
make the variable a boolean toggle, because (as Drew notes) nobody is
going to want to customise the number of sub-expressions, really.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

> >   (defface isearch
> >     '((((class color) (min-colors 88) (background light))
> >        ;; The background must not be too dark, for that means
> >        ;; the character is hard to see when the cursor is there.
> >        (:background "magenta3" :foreground "lightskyblue1"))
> 
> I meant that these deffaces aren't complete -- they look like examples,
> and the examples are taken from the isearch face.
> 
> There should be more of them, and they should be complete, and they
> should have better names (isearch-group-1 etc perhaps).

FWIW, I use simple definitions like this one for the
eight regexp-level faces, not bothering with multiple
kinds of displays.  (Not saying that's what isearch.el
should do.)

(defface isearchp-regexp-level-8 
  '((((background dark)) (:background "#12EC00003F0E")) ; a very dark blue
    (t (:background "#F6F5FFFFE1E1")))  ; a light yellow
  "Face used to highlight subgroup level 8 of your search context.
This highlighting is done during regexp searching whenever
`isearchp-highlight-regexp-group-levels-flag' is non-nil."
  :group 'isearch-plus :group 'faces)

bug#6227: Color isearch regexp submatches differently

[Eli Zaretskii]

> Date: Sun, 20 Sep 2020 09:41:59 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: lennart.borgman@gmail.com, 6227@debbugs.gnu.org
> 
> > >   (defface isearch
> > >     '((((class color) (min-colors 88) (background light))
> > >        ;; The background must not be too dark, for that means
> > >        ;; the character is hard to see when the cursor is there.
> > >        (:background "magenta3" :foreground "lightskyblue1"))
> > 
> > I meant that these deffaces aren't complete -- they look like examples,
> > and the examples are taken from the isearch face.
> > 
> > There should be more of them, and they should be complete, and they
> > should have better names (isearch-group-1 etc perhaps).
> 
> FWIW, I use simple definitions like this one for the
> eight regexp-level faces, not bothering with multiple
> kinds of displays.  (Not saying that's what isearch.el
> should do.)
> 
> (defface isearchp-regexp-level-8 
>   '((((background dark)) (:background "#12EC00003F0E")) ; a very dark blue
>     (t (:background "#F6F5FFFFE1E1")))  ; a light yellow
>   "Face used to highlight subgroup level 8 of your search context.
> This highlighting is done during regexp searching whenever
> `isearchp-highlight-regexp-group-levels-flag' is non-nil."
>   :group 'isearch-plus :group 'faces)

That's not the same thing at all: your face definition will work with
any terminal (through the automatic color translation).

bug#6227: Color isearch regexp submatches differently

[Drew Adams]

> > I'd rather go the other way, to be honest -- just leave group-1 to -3,
> > perhaps.  I think it's rather unusual to have a that many sub-groups
> > interactively...
> 
> That's true, but the problem which bothers me is that customizing
> search-highlight-submatches alone to a larger value is not enough.  I
> don't think any other customization we have requires users to create
> faces or similar objects.

FWIW, the option in Isearch+ is just a boolean, not
a max number of levels/groups.  I use it for both
Isearch and `replace-highlight'.  The use cases are
only interactive, AFAICimagine, and I don't imagine
(and haven't seen) any need for lots of levels.  I
use 8 levels, and that's already a lot.

As for the relation between your max # groups and
faces: if you really want to let users change the
max # (not needed, I think), you can just have a
fixed number of faces and recycle them.  You don't
have to imagine that users will need to both (1)
increase `search-highlight-submatches' and (2)
create corresponding new faces for the additional
groups to be matched.  It's pretty clear which
level is involved if the same color is used for,
say, level 2 and level 10.

I mentioned this recycling possibility earlier,
saying that I considered going through the 1-8
faces I have and then going through them again, for
groups 9-18, and again,...  But I've never seen any
need for that.  Your design sounds like overkill, if
the intention is just interactive use.

Just a suggestion.

Also, your option name should have "max" in it, I
think.  And if you use it only for Isearch, then
maybe use "isearch", not "search" in the name.