bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
current customization I have in my .emacs.

I believe that my proposal is useful in general.  So I request that it
be folded in to Emacs-24.1.

Part-I: `hi-lock-face-buffer' & Co.
----------------------------------

1) Review the face names used in `hi-lock-face-defaults' and make the
   faces customizable.  The defaults may not look good on a user's his
   own font-lock configuration.

2) Make `hi-lock-face-buffer' use a different face on each invocation.  

   Here is a real-world usecase for the above request.

   As a programmer, I use highlighting to trace variable dependencies
   within a function.  For example, in the example below, after
   highlighting the variables in __different__ faces, I will come to the
   conclusion that "a" depends on "d" and "tmp".

     c = d;
     b = c + tmp;
     a = b;

   And I use this very often to track variables and how they get their
   values from.

   If I were to use the default Emacs provided behaviour then I would
   have to press M-n multiple times as I highlight more and more
   symbols. (Typically I have 3-5 symbols highlighted before I turn off
   highlighting.)

See elisp snippet at the end of the mail.


Part-II: `unhighlight-regexp'
------------------------------

See usecase in Part-I/Item-2 

1) I want to selectively turn-off highlighting for certain regexps
   (arguably) that _specific_ highlighted regexp cursor is stationed on.
   This would happen when I decide that I don't want to factor a
   particular variable for my current refactoring exercise.

   I find the current behaviour of `unhighlight-regexp' slightly
   tedious.  

   1. There is no completion for which regexp I want to unhighlight and
      I have to circle through `hi-lock-interactive-patterns'.

   2. Browsing the `hi-lock-interactive-patterns' is tedious for the
      following reasons:

      - The order in which unhighlighting happens could totally be
        unrelated to the order in which highlighting happens.  When I am
        analyzing the variable flow, I don't want to do a "context
        switch" trying to find out what item to choose from the
        `unhighlight-regexp' menu.

2) I want to unhighlight all regexps.  This happens when I am done with
   variable analysis.


ps: I am not questioning the current defaults.  I am only saying that it
the current behaviour is too generic to be immediately useful (atleast
for my usecase) and so needs some sort of extra augmentation.

Part-III: Elisp snippet
-----------------------

;; Why should the color of the faces be encoded in the variable name?
;; Seems counter-intutitive to me.  I cannot relate to a hi-yellow
;; face customized to render in red.

;; (defvar hi-lock-face-defaults
;;   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
;;     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
;;   "Default faces for hi-lock interactive functions.")

;; So roll out my own face for highlighting.  Make them
;; __customizable__.  Note that the name of the face doesn't say what
;; the color of the highlight will be.  Works for the color theme that
;; I have.
(custom-set-faces
 '(highlight1 ((t (:background "yellow" :foreground "black"))))
 '(highlight2 ((t (:background "OrangeRed" :foreground "black"))))
 '(highlight3 ((((class color) (background dark)) (:background "AntiqueWhite" :foreground "black"))))
 '(highlight4 ((t (:background "SystemHotTrackingColor"))))
 '(highlight5 ((t (:background "VioletRed" :foreground "black")))))

;; override the Emacs default
(setq hi-lock-face-defaults
      '("highlight1" "highlight2" "highlight3" "highlight4" "highlight5"))

(defvar hi-lock-faces
  (let ((l (copy-list hi-lock-face-defaults)))
    (setcdr (last l) l))
  "Circular list of faces in `hi-lock-face-defaults'.")

;; make `hi-lock-faces' buffer local
(make-variable-buffer-local 'hi-lock-faces)

(defun highlight-symbol ()
  "Highlight symbol at point.  
For illustartion only.  Note the use of `hi-lock-face-buffer'.
Choose a new face from `hi-lock-faces' on each invocation.
Overrides the default behaviour in vanilla Emacs which is to use
the face at the head of the list."
  (interactive)
  (hi-lock-face-buffer
   (concat "\\_<" (regexp-quote (thing-at-point 'symbol)) "\\_>")
   ;; rotate the face list
   (prog1 (car hi-lock-faces)
     (setq hi-lock-faces (cdr hi-lock-faces)))))

(defun my-unhighlight-regexp (arg)
  "Wrapper around `unhighlight-regexp'.
With a prefix argument, turn off all highlighting.

TODO: Check if the symbol is right now on a highlighted regexp.
If yes, unhighlight only that regexp."
  (interactive "P")
  (if arg
      (mapc (lambda (p)
	      (unhighlight-regexp (car p)))
	    hi-lock-interactive-patterns)
    (call-interactively 'unhighlight-regexp)))

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


I am attaching a patch that addresses Part-I/Item-1 below.  

The patch adds no functionality.  It merely removes "color" from
appearing verbatim in the face variables.  

I am using `hi-lock-' as prefix.  Let me know if `highlight-' will be
more appropriate (considering that there is a standard highlight face).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-part1.diff --]
[-- Type: text/x-diff, Size: 5015 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-10 14:45:26 +0000
+++ lisp/ChangeLog	2012-10-10 20:05:04 +0000
@@ -1,3 +1,14 @@
+2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-yellow, hi-pink, hi-green, hi-blue, hi-black-b)
+        (hi-blue-b, hi-green-b, hi-red-b, hi-black-hb): Mark these faces
+	as obsolete.
+        (hi-lock-1, hi-lock-2, hi-lock-3, hi-lock-4, hi-lock-5, hi-lock-6)
+	(hi-lock-7, hi-lock-8, hi-lock-9): New faces.
+	(hi-lock-face-defaults, hi-lock-line-face-buffer)
+	(hi-lock-face-buffer, hi-lock-face-phrase-buffer): Propagate above
+	changes.
+
 2012-10-10  Kenichi Handa  <handa@gnu.org>
 
 	* select.el (xselect--encode-string): If a coding is specified for

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-07 00:27:31 +0000
+++ lisp/hi-lock.el	2012-10-10 20:01:23 +0000
@@ -140,7 +140,7 @@ patterns."
   :group 'hi-lock
   :group 'faces)
 
-(defface hi-yellow
+(defface hi-lock-1
   '((((min-colors 88) (background dark))
      (:background "yellow1" :foreground "black"))
     (((background dark)) (:background "yellow" :foreground "black"))
@@ -149,13 +149,13 @@ patterns."
   "Default face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-pink
+(defface hi-lock-2
   '((((background dark)) (:background "pink" :foreground "black"))
     (t (:background "pink")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-green
+(defface hi-lock-3
   '((((min-colors 88) (background dark))
      (:background "green1" :foreground "black"))
     (((background dark)) (:background "green" :foreground "black"))
@@ -164,40 +164,50 @@ patterns."
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-blue
+(defface hi-lock-4
   '((((background dark)) (:background "light blue" :foreground "black"))
     (t (:background "light blue")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-black-b
+(defface hi-lock-5
   '((t (:weight bold)))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-blue-b
+(defface hi-lock-6
   '((((min-colors 88)) (:weight bold :foreground "blue1"))
     (t (:weight bold :foreground "blue")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-green-b
+(defface hi-lock-7
   '((((min-colors 88)) (:weight bold :foreground "green1"))
     (t (:weight bold :foreground "green")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-red-b
+(defface hi-lock-8
   '((((min-colors 88)) (:weight bold :foreground "red1"))
     (t (:weight bold :foreground "red")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-black-hb
+(defface hi-lock-9
   '((t (:weight bold :height 1.67 :inherit variable-pitch)))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
+(define-obsolete-face-alias 'hi-yellow 'hi-lock-1 "24.3")
+(define-obsolete-face-alias 'hi-pink 'hi-lock-2 "24.3")
+(define-obsolete-face-alias 'hi-green 'hi-lock-3 "24.3")
+(define-obsolete-face-alias 'hi-blue 'hi-lock-4 "24.3")
+(define-obsolete-face-alias 'hi-black-b 'hi-lock-5 "24.3")
+(define-obsolete-face-alias 'hi-blue-b 'hi-lock-6 "24.3")
+(define-obsolete-face-alias 'hi-green-b 'hi-lock-7 "24.3")
+(define-obsolete-face-alias 'hi-red-b 'hi-lock-8 "24.3")
+(define-obsolete-face-alias 'hi-black-hb 'hi-lock-9 "24.3")
+
 (defvar hi-lock-file-patterns nil
   "Patterns found in file for hi-lock.  Should not be changed.")
 
@@ -207,8 +217,8 @@ patterns."
 (define-obsolete-variable-alias 'hi-lock-face-history
                                 'hi-lock-face-defaults "23.1")
 (defvar hi-lock-face-defaults
-  '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
-    "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
+  '("hi-lock-1" "hi-lock-2" "hi-lock-3" "hi-lock-4" "hi-lock-5"
+    "hi-lock-6" "hi-lock-7" "hi-lock-8" "hi-lock-9")
   "Default faces for hi-lock interactive functions.")
 
 ;;(dolist (f hi-lock-face-defaults)
@@ -410,7 +420,7 @@ updated as you type."
     (hi-lock-regexp-okay
      (read-regexp "Regexp to highlight line" (car regexp-history)))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern
    ;; The \\(?:...\\) grouping construct ensures that a leading ^, +, * or ?
@@ -435,7 +445,7 @@ updated as you type."
     (hi-lock-regexp-okay
      (read-regexp "Regexp to highlight" (car regexp-history)))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 
@@ -457,7 +467,7 @@ updated as you type."
      (hi-lock-process-phrase
       (read-regexp "Phrase to highlight" (car regexp-history))))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 


[-- Attachment #3: Type: text/plain, Size: 5114 bytes --]


> Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
> Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
> current customization I have in my .emacs.
>
> I believe that my proposal is useful in general.  So I request that it
> be folded in to Emacs-24.1.
>
> Part-I: `hi-lock-face-buffer' & Co.
> ----------------------------------
>
> 1) Review the face names used in `hi-lock-face-defaults' and make the
>    faces customizable.  The defaults may not look good on a user's his
>    own font-lock configuration.
>
> 2) Make `hi-lock-face-buffer' use a different face on each invocation.  
>
>    Here is a real-world usecase for the above request.
>
>    As a programmer, I use highlighting to trace variable dependencies
>    within a function.  For example, in the example below, after
>    highlighting the variables in __different__ faces, I will come to the
>    conclusion that "a" depends on "d" and "tmp".
>
>      c = d;
>      b = c + tmp;
>      a = b;
>
>    And I use this very often to track variables and how they get their
>    values from.
>
>    If I were to use the default Emacs provided behaviour then I would
>    have to press M-n multiple times as I highlight more and more
>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>    highlighting.)
>
> See elisp snippet at the end of the mail.
>
>
> Part-II: `unhighlight-regexp'
> ------------------------------
>
> See usecase in Part-I/Item-2 
>
> 1) I want to selectively turn-off highlighting for certain regexps
>    (arguably) that _specific_ highlighted regexp cursor is stationed on.
>    This would happen when I decide that I don't want to factor a
>    particular variable for my current refactoring exercise.
>
>    I find the current behaviour of `unhighlight-regexp' slightly
>    tedious.  
>
>    1. There is no completion for which regexp I want to unhighlight and
>       I have to circle through `hi-lock-interactive-patterns'.
>
>    2. Browsing the `hi-lock-interactive-patterns' is tedious for the
>       following reasons:
>
>       - The order in which unhighlighting happens could totally be
>         unrelated to the order in which highlighting happens.  When I am
>         analyzing the variable flow, I don't want to do a "context
>         switch" trying to find out what item to choose from the
>         `unhighlight-regexp' menu.
>
> 2) I want to unhighlight all regexps.  This happens when I am done with
>    variable analysis.
>
>
> ps: I am not questioning the current defaults.  I am only saying that it
> the current behaviour is too generic to be immediately useful (atleast
> for my usecase) and so needs some sort of extra augmentation.
>
> Part-III: Elisp snippet
> -----------------------
>
> ;; Why should the color of the faces be encoded in the variable name?
> ;; Seems counter-intutitive to me.  I cannot relate to a hi-yellow
> ;; face customized to render in red.
>
> ;; (defvar hi-lock-face-defaults
> ;;   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
> ;;     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
> ;;   "Default faces for hi-lock interactive functions.")
>
> ;; So roll out my own face for highlighting.  Make them
> ;; __customizable__.  Note that the name of the face doesn't say what
> ;; the color of the highlight will be.  Works for the color theme that
> ;; I have.
> (custom-set-faces
>  '(highlight1 ((t (:background "yellow" :foreground "black"))))
>  '(highlight2 ((t (:background "OrangeRed" :foreground "black"))))
>  '(highlight3 ((((class color) (background dark)) (:background "AntiqueWhite" :foreground "black"))))
>  '(highlight4 ((t (:background "SystemHotTrackingColor"))))
>  '(highlight5 ((t (:background "VioletRed" :foreground "black")))))
>
> ;; override the Emacs default
> (setq hi-lock-face-defaults
>       '("highlight1" "highlight2" "highlight3" "highlight4" "highlight5"))
>
> (defvar hi-lock-faces
>   (let ((l (copy-list hi-lock-face-defaults)))
>     (setcdr (last l) l))
>   "Circular list of faces in `hi-lock-face-defaults'.")
>
> ;; make `hi-lock-faces' buffer local
> (make-variable-buffer-local 'hi-lock-faces)
>
> (defun highlight-symbol ()
>   "Highlight symbol at point.  
> For illustartion only.  Note the use of `hi-lock-face-buffer'.
> Choose a new face from `hi-lock-faces' on each invocation.
> Overrides the default behaviour in vanilla Emacs which is to use
> the face at the head of the list."
>   (interactive)
>   (hi-lock-face-buffer
>    (concat "\\_<" (regexp-quote (thing-at-point 'symbol)) "\\_>")
>    ;; rotate the face list
>    (prog1 (car hi-lock-faces)
>      (setq hi-lock-faces (cdr hi-lock-faces)))))
>
> (defun my-unhighlight-regexp (arg)
>   "Wrapper around `unhighlight-regexp'.
> With a prefix argument, turn off all highlighting.
>
> TODO: Check if the symbol is right now on a highlighted regexp.
> If yes, unhighlight only that regexp."
>   (interactive "P")
>   (if arg
>       (mapc (lambda (p)
> 	      (unhighlight-regexp (car p)))
> 	    hi-lock-interactive-patterns)
>     (call-interactively 'unhighlight-regexp)))

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

> -(defface hi-yellow
> +(defface hi-lock-1

I'm not sure it's an improvement.  When choosing a face in
hi-lock-face-buffer, "hi-lock-1" doesn't speak much to me contrary to
"hi-yellow".

So, could you expand on your motivations for this change, so we can find
another solution that satisfies your use case and mine?

I've installed your second patch (the hi-lock-auto-select-face, tho
using `pop' to simplify the code), but when hi-lock-auto-select-face is
t the user can't specify a face any more, which I think is too drastic,
which should provide a C-u override or something.

I also installed the 4th patch (the defaults for unhighlight), tho
I removed the docstring change (we usually don't document which default
is used in minibuffer arguments).  Also I moved your code to a separate
function, to clarify the code.  Furthermore I did not install your
change to the completion table so only the regexps that match at point
get completed.  Instead the list of regexps is passed as a list
of defaults.

BTW the hi-lock-auto-select-face should be fixed to just hash-cons (aka
uniquify) regexps, so you don't need your maphash loop to recover the
regexp from the unique "serialized" number.

I also installed the 5th patch (the "unhighlight all") tho I'm not
yet sure this is the right interface.  OT1H I think it would be nicer to
provide this "all" as one of the choices in the minibuffer, but OTOH
I can't think of any way to do that which is not hideously hackish.

As for the 3rd patch, I haven't installed it yet because I'm worried
that (format "\\_<%s\\_>" (regexp-quote tag)) may sometimes fail to
match `tag', so I think it needs some additional sanity check.


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Drew Adams]

> > -(defface hi-yellow
> > +(defface hi-lock-1
> 
> I'm not sure it's an improvement.  When choosing a face in
> hi-lock-face-buffer, "hi-lock-1" doesn't speak much to me contrary to
> "hi-yellow".

Not specifically related to this face, but it is a bad idea, in general (no
doubt there are exceptions), for a face name to advertize particular face
attributes, such as the color.

The color is presumably something that the user can customize, and is typically
not something that speaks to the use or meaning of the face.

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

>> > -(defface hi-yellow
>> > +(defface hi-lock-1
>> I'm not sure it's an improvement.  When choosing a face in
>> hi-lock-face-buffer, "hi-lock-1" doesn't speak much to me contrary to
>> "hi-yellow".
> Not specifically related to this face, but it is a bad idea, in general (no
> doubt there are exceptions), for a face name to advertize particular face
> attributes, such as the color.

Depends.  In the present case, the face has no particular purpose, so
saying that "its purpose to highlight in yellow" doesn't sound like such
a bad idea.  Not really worse than "its purpose is to be number 2".

> The color is presumably something that the user can customize, and is
> typically not something that speaks to the use or meaning of the face.

"2" doesn't "speak to the use or meaning of the face" very much either.

I think the real issue here is that hi-lock should have a customizable
set of faces rather than a set of customizable faces.
So if the user doesn't like hi-yellow (which should be called
hi-lock-yellow, BTW) because she never highlights in yellow, she can
replace it with her own face with the name she likes.


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Drew Adams]

> >> > -(defface hi-yellow
> >> > +(defface hi-lock-1
> >> I'm not sure it's an improvement.  When choosing a face in
> >> hi-lock-face-buffer, "hi-lock-1" doesn't speak much to me 
> >> contrary to "hi-yellow".
> >
> > Not specifically related to this face, but it is a bad 
> > idea, in general (no doubt there are exceptions), for a
> > face name to advertize particular face attributes, such
> > as the color.
> 
> Depends.

I too suggested that it depends.  But it depends on what "depends" means ;-).
I.e., depends on what?  The dependence I see is that there could be exceptions
where the purpose/use/meaning of the face is in fact related to a particular
color.

> In the present case, the face has no particular purpose,

So in particular, it is NOT related to any particular color, including yellow.

> so saying that "its purpose to highlight in yellow" doesn't 
> sound like such a bad idea.

That makes as much sense (to me) as to say that its name should be
`hi-lock-rumpelstiltskin".  Unless it is true that it really always should
"highlight in yellow", regardless of the face's current color.

If all we can say is that this face is about highlighting, then the only
operative word is "highlight".  If we can be more specific - e.g., highlighting
like a marker pen, then that's what is called for.

In `highlight.el', for instance, I have a command for highlighting text by
dragging the mouse over it, like using a marker pen, aka a highlighter.

That command is called `hlt-highlighter' (it can use any face, like all `hlt-*'
commands).  If `hl-lock-yellow' were intended for this kind of highlighting then
you might call it `hl-lock-highlighter' or some such.

I have no idea what `hl-lock-yellow' is really for.  You yourself have just said
that its use has nothing to do with "yellow", however.

> Not really worse than "its purpose is to be number 2".

Is its purpose really "to highlight in yellow"?  You seem to say no, above.  But
if so then you probably don't need to define a customizable face for that.  Just
highlight in yellow.

Or if the color must always be (or is always intended to be) yellow, but users
can customize other face attributes, then using "yellow" in the name would make
sense.  In that case, the doc string should say something about this
restriction/intention wrt the color being constant.

> > The color is presumably something that the user can 
> > customize, and is typically not something that speaks
> > to the use or meaning of the face.
> 
> "2" doesn't "speak to the use or meaning of the face" very 
> much either.

You didn't hear me argue in favor of using `2' here.  Those are presumably not
the only two choices: 2 vs yellow.

On the other hand, if absolutely nothing can be said about this face other than
it is one of N hi-lock faces (none of which we can say anything else about),
then `2' is better than nothing.

It is certainly better than being overly specific and misleading, and doubly
certainly better than suggesting a specific face attribute such as a particular
color.

`hi-lock-2' does not suggest that any particular face attribute has the value
`2', unless it is something like a box line width of 2 pixels.  `hi-lock-yellow'
misleads immediately, suggesting that the face always has the color yellow.

> I think the real issue here is that hi-lock should have a customizable
> set of faces rather than a set of customizable faces.
> So if the user doesn't like hi-yellow (which should be called
> hi-lock-yellow, BTW) because she never highlights in yellow, she can
> replace it with her own face with the name she likes.

Ah, in that case you are really talking, I think, about having one or more user
options, which each has a face (or a set of faces) as value.

I don't see how that is related to the above, however.  If you call such an
option `*-yellow' then its name would still be misleading, no?  And if on the
other hand you feel that `hi-lock-2' is OK as an option name here, but not as a
face name, then I don't follow the logic.

Just why would you prefer a "customizable set of faces" over a "set of
customizable faces"?  And how does that relate to the names?

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

>> I think the real issue here is that hi-lock should have a customizable
>> set of faces rather than a set of customizable faces.
>> So if the user doesn't like hi-yellow (which should be called
>> hi-lock-yellow, BTW) because she never highlights in yellow, she can
>> replace it with her own face with the name she likes.
> Ah, in that case you are really talking, I think, about having one or
> more user options, which each has a face (or a set of faces) as value.

Just one option `hi-lock-faces'.

> Just why would you prefer a "customizable set of faces" over a "set of
> customizable faces"?  And how does that relate to the names?

Because the user can then choose the names that make sense to her.


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I think the real issue here is that hi-lock should have a customizable
>>> set of faces rather than a set of customizable faces.
>>> So if the user doesn't like hi-yellow (which should be called
>>> hi-lock-yellow, BTW) because she never highlights in yellow, she can
>>> replace it with her own face with the name she likes.
>> Ah, in that case you are really talking, I think, about having one or
>> more user options, which each has a face (or a set of faces) as value.
>
> Just one option `hi-lock-faces'.

1. I want the name to be opaque and semantic.

2. I also want a pre-defined set of faces for highlighting apart from
   the one "core" highlight face.  I think there are 9 hi-* faces and
   these numbers are good enough.

   Think of them as extra colors in my palette.

   Having a set of highlighting faces will help in theming.  For
   example, consider finding file in ido-mode.  When I do C-x C-f, I see
   various faces - the minibuffer prompt, ido-first-match, ido-subdir,
   ido-indicator all occurring /next/ to each other.  If there are
   hi-lock-N faces, chosen by a theme designed, one can simply have ido
   faces inherit from these themed faces.  It is much cleaner.

   Remember choosing faces that can co-exist in buffer without much
   trouble to eyes is challenging task - one needs to balance harmony
   and contrast.  A theme designer is likely to work with a palette and
   can go with color-picking techniques like triad, tetrad schemes.  See

        http://colorschemedesigner.com/
        http://www.w3.org/wiki/Colour_theory
        http://packages.debian.org/squeeze/agave

   Triad and tetrads apparently are colors that are 120 and 90 degrees
   apart in the color wheel.  So if there are N highlighting faces, they
   can be spaced 360/N degree apart in a color wheeel.

   Drew's reasoning that it is the N-th highlighting face in a sequence.

3. Configuring an yellow face in red is a bit ugly.  It is declaring a
   variable name FALSE that is assigned a variable value true.

>> Just why would you prefer a "customizable set of faces" over a "set of
>> customizable faces"?  And how does that relate to the names?
>
> Because the user can then choose the names that make sense to her.

While reading a face name from minibuffer, if the face name itself is
highlighted in that face - think rainbow mode - then the name of the
face shouldn't matter.

What you are asking for is a constant face whose properties don't change
at all.  One can have an elpa packages which provides constant faces,
that are immediately useful.

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

>> Just one option `hi-lock-faces'.
> 1. I want the name to be opaque and semantic.

That's why I suggest hi-lock-faces, where you choose the face names you like.

> 2. I also want a pre-defined set of faces for highlighting apart from
>    the one "core" highlight face.

Sure, hi-lock-faces's default value would include a predefined set of
faces, to match the current behavior.


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


There are three issues that I see with your commmit:

Issue-1:  face-at-point broken?
===============================

M-x toggle-debug-on-error RET
M-x find-function RET face-at-point RET
C-x w h
C-x w r

    Debugger entered--Lisp error: (error "Not a face: nil")
      signal(error ("Not a face: nil"))
      error("Not a face: %s" nil)
      check-face(nil)
      face-name(nil)
      hi-lock--regexps-at-point()
      byte-code("\b\203\a\305C\207\306 \203.	<\203.\n\203.\307\310\215\207\v\204!\311\312!\210\313 \314\f\204-\315\2022\316\317\f@\"\v\320\305\320\211\f&\a)C\207" [current-prefix-arg last-nonmenu-event use-dialog-box hi-lock-interactive-patterns defaults t display-popup-menus-p snafu (byte-code "\301\302\303\304\305\306\b\"BB\"\206.\307\310\311\"\207" [hi-lock-interactive-patterns x-popup-menu t keymap "Select Pattern to Unhighlight" mapcar #[(pattern) "\b@\301\302\b@\303\bA@A@A@!#\304\211B\b@F\207" [pattern format "%s (%s)" symbol-name nil] 6] throw snafu ("")] 7) error "No highlighting to remove" hi-lock--regexps-at-point completing-read "Regexp to unhighlight: " format "Regexp to unhighlight (default %s): " nil] 8)
      call-interactively(unhighlight-regexp nil nil)

The reason is faceprop happens to be a string

(get-char-property (point) 'face) 
 : "hi-yellow"

Issue-2:  Various issues with unhighlighting 
============================================

Once you fix Issue-1 you will run in to other issues with
un-highlighting.  Try highlighting and UN-highlighting in following 3
ways

1. Buffer with font-lock-mode ON
2. Buffer with font-lock-mode OFF
3. Unhighlight from the menu

Caveat:  Extra testing needed if /type/ of face names are changed 
=================================================================

hi-lock-face-defautls is currently a list of face names (stringp).  If
it is made a defcustom, it will be cast to a list of symbols (symbolp).
In some places, face names are expected and in some other places face as
a symbol is used.  So you need to re-run the tests if move from
string->symbols.

Suggestion: In default faces, don't mix bold and foreground/background
=======================================================================

I am OK with defcustom of faces.  Something like

    (defcustom hi-lock-face-defaults
      '(hi-yellow hi-pink hi-green hi-blue hi-black-b
                  hi-blue-b hi-red-b hi-green-b hi-black-hb)
      "Default faces for hi-lock interactive functions."
      :type '(repeat face)
      :group 'hi-lock-faces)

Bonus points if the default settings of the faces that go in there is
revised as part of this bug.  I want to highlight variables in a buffer.
So consistent policy of highlighting - a changed background of normal
face - will require no additional work.

Here is how my own faces look like.  Note that the first 4 come from
"blue" space and the later 4 or so come from "pink" space, all chosen
using agave.

ps:  I will let you install a change for the above issues.


[-- Attachment #2: hi-defaults.pn --]
[-- Type: image/png, Size: 8701 bytes --]

[-- Attachment #3: my-hi-faces.png --]
[-- Type: image/png, Size: 10898 bytes --]

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


Please review the attached patch.

The patch exposes exposes a bug in defcustom and defvar-local which I
will outline separately in a followup post (after another 2-3 hours).

ps: I only wish you had tested unhighlighting part.  It would have saved
some re-working for me.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095.patch --]
[-- Type: text/x-diff, Size: 12186 bytes --]

=== modified file 'etc/NEWS'
--- etc/NEWS	2012-12-04 17:07:09 +0000
+++ etc/NEWS	2012-12-06 14:44:01 +0000
@@ -74,6 +74,15 @@ when its arg ADJACENT is non-nil (when c
 it works like the utility `uniq'.  Otherwise by default it deletes
 duplicate lines everywhere in the region without regard to adjacency.
 
+** Various improvements to hi-lock.el
+*** New user variables `hi-lock-faces' and `hi-lock-auto-select-face'
+*** Highlighting commands (`hi-lock-face-buffer', `hi-lock-face-phrase-buffer'
+and `hi-lock-line-face-buffer') now take a prefix argument which
+temporarily inverts the meaning of `hi-lock-auto-select-face'.
+*** Unhighlighting command (`hi-lock-unface-buffer') now un-highlights text at
+point.  When called interactively with C-u, removes all highlighting
+in current buffer.
+
 ** Tramp
 +++
 *** New connection method "adb", which allows to access Android

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-06 09:15:27 +0000
+++ lisp/ChangeLog	2012-12-06 14:24:34 +0000
@@ -1,3 +1,18 @@
+2012-12-06  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-lock-faces): New user variable.
+	(hi-lock--auto-select-face-defaults): Use `hi-lock-faces'.
+	(hi-lock-read-face-name): New optional param `toggle-auto-select'.
+	(hi-lock-line-face-buffer, hi-lock-face-buffer)
+	(hi-lock-face-phrase-buffer): Allow prefix argument to temporarily
+	toggle the value of `hi-lock-auto-select-face'.
+	(hi-lock--regexps-at-point, hi-lock-unface-buffer): Fix earlier
+	commit.
+	(hi-lock-set-pattern): Refuse to highlight a regexp that is
+	already highlighted.
+
+	* faces.el (face-at-point): Fix bug (Bug#11095).
+
 2012-12-06  Michael Albinus  <michael.albinus@gmx.de>
 
 	* net/tramp.el (tramp-replace-environment-variables): Hide

=== modified file 'lisp/faces.el'
--- lisp/faces.el	2012-11-25 04:50:20 +0000
+++ lisp/faces.el	2012-12-05 19:35:05 +0000
@@ -1884,6 +1884,7 @@ Return nil if it has no specified face."
                        (get-char-property (point) 'face)
                        'default))
          (face (cond ((symbolp faceprop) faceprop)
+		     ((stringp faceprop) (intern-soft faceprop))
                      ;; List of faces (don't treat an attribute spec).
                      ;; Just use the first face.
                      ((and (consp faceprop) (not (keywordp (car faceprop)))

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-12-04 21:13:47 +0000
+++ lisp/hi-lock.el	2012-12-06 14:02:42 +0000
@@ -213,13 +213,27 @@ When non-nil, each hi-lock command will
 
 (define-obsolete-variable-alias 'hi-lock-face-history
                                 'hi-lock-face-defaults "23.1")
+
 (defvar hi-lock-face-defaults
   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
   "Default faces for hi-lock interactive functions.")
 
+(defcustom hi-lock-faces
+  (or
+   (when (boundp 'hi-lock-face-defaults)
+     (mapcar
+      (lambda (face-name) (intern-soft face-name))
+      hi-lock-face-defaults))
+   '(hi-yellow hi-pink hi-green hi-blue hi-black-b
+	       hi-blue-b hi-red-b hi-green-b hi-black-hb))
+  "Default faces for hi-lock interactive functions."
+  :type '(repeat face)
+  :group 'hi-lock
+  :version "24.4")
+
 (defvar-local hi-lock--auto-select-face-defaults
-  (let ((l (copy-sequence hi-lock-face-defaults)))
+  (let ((l (copy-sequence hi-lock-faces)))
     (setcdr (last l) l))
   "Circular list of faces used for interactive highlighting.
 When `hi-lock-auto-select-face' is non-nil, use the face at the
@@ -410,8 +424,12 @@ versions before 22 use the following in
 ;;;###autoload
 (defun hi-lock-line-face-buffer (regexp &optional face)
   "Set face of all lines containing a match of REGEXP to FACE.
-Interactively, prompt for REGEXP then FACE, using a buffer-local
-history list for REGEXP and a global history list for FACE.
+Interactively, prompt for REGEXP, using a buffer-local history
+list for REGEXP .  When `hi-lock-auto-select-face' is non-nil,
+prompt for FACE using a global history list.  Otherwise, use the
+next of `hi-lock-faces'.  When invoked with
+\\[universal-argument] prefix, invert the meaning of
+`hi-lock-auto-select-face'.
 
 If Font Lock mode is enabled in the buffer, it is used to
 highlight REGEXP.  If Font Lock mode is disabled, overlays are
@@ -421,8 +439,9 @@ updated as you type."
    (list
     (hi-lock-regexp-okay
      (read-regexp "Regexp to highlight line" (car regexp-history)))
-    (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+    (let ((toggle-auto-select current-prefix-arg))
+      (hi-lock-read-face-name toggle-auto-select))))
+  (unless (facep face) (setq face (hi-lock-read-face-name)))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern
    ;; The \\(?:...\\) grouping construct ensures that a leading ^, +, * or ?
@@ -435,8 +454,12 @@ updated as you type."
 ;;;###autoload
 (defun hi-lock-face-buffer (regexp &optional face)
   "Set face of each match of REGEXP to FACE.
-Interactively, prompt for REGEXP then FACE, using a buffer-local
-history list for REGEXP and a global history list for FACE.
+Interactively, prompt for REGEXP, using a buffer-local history
+list for REGEXP .  When `hi-lock-auto-select-face' is non-nil,
+prompt for FACE using a global history list.  Otherwise, use the
+next of `hi-lock-faces'.  When invoked with
+\\[universal-argument] prefix, invert the meaning of
+`hi-lock-auto-select-face'.
 
 If Font Lock mode is enabled in the buffer, it is used to
 highlight REGEXP.  If Font Lock mode is disabled, overlays are
@@ -446,8 +469,9 @@ updated as you type."
    (list
     (hi-lock-regexp-okay
      (read-regexp "Regexp to highlight" (car regexp-history)))
-    (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+    (let ((toggle-auto-select current-prefix-arg))
+      (hi-lock-read-face-name toggle-auto-select))))
+  (unless (facep face) (setq face (hi-lock-read-face-name)))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 
@@ -457,7 +481,12 @@ updated as you type."
 (defun hi-lock-face-phrase-buffer (regexp &optional face)
   "Set face of each match of phrase REGEXP to FACE.
 If called interactively, replaces whitespace in REGEXP with
-arbitrary whitespace and makes initial lower-case letters case-insensitive.
+arbitrary whitespace and makes initial lower-case letters
+case-insensitive.  When `hi-lock-auto-select-face' is non-nil,
+prompt for FACE using a global history list.  Otherwise, use the
+next of `hi-lock-faces'.  When invoked with
+\\[universal-argument] prefix, invert the meaning of
+`hi-lock-auto-select-face'.
 
 If Font Lock mode is enabled in the buffer, it is used to
 highlight REGEXP.  If Font Lock mode is disabled, overlays are
@@ -467,9 +496,10 @@ updated as you type."
    (list
     (hi-lock-regexp-okay
      (hi-lock-process-phrase
-      (read-regexp "Phrase to highlight" (car regexp-history))))
-    (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+      (read-regexp "Phrase to highlight" (car regexp-history))))))
+  (let ((toggle-auto-select current-prefix-arg))
+    (hi-lock-read-face-name toggle-auto-select))
+  (unless (facep face) (setq face (hi-lock-read-face-name)))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 
@@ -482,26 +512,29 @@ updated as you type."
     (let ((desired-serial (get-char-property
                            (point) 'hi-lock-overlay-regexp)))
       (when desired-serial
-        (catch 'regexp
           (maphash
            (lambda (regexp serial)
              (when (= serial desired-serial)
                (push regexp regexps)))
-           hi-lock-string-serialize-hash))))
-    ;; With font-locking on, check if the cursor is on an highlighted text.
-    ;; Checking for hi-lock face is a good heuristic.
-    (and (string-match "\\`hi-lock-" (face-name (face-at-point)))
+	 hi-lock-string-serialize-hash)))
+    ;; With font-locking on, check if cursor is on an highlighted
+    ;; text.
+    (when (member (list 'quote (face-at-point))
+		  (mapcar (lambda (pattern)
+			    (cadr (cadr pattern)))
+			  hi-lock-interactive-patterns))
          (let* ((hi-text
                  (buffer-substring-no-properties
-                  (previous-single-property-change (point) 'face)
-                  (next-single-property-change (point) 'face))))
+	       (previous-single-char-property-change (point) 'face)
+	       (next-single-char-property-change (point) 'face))))
            ;; Compute hi-lock patterns that match the
            ;; highlighted text at point.  Use this later in
            ;; during completing-read.
            (dolist (hi-lock-pattern hi-lock-interactive-patterns)
              (let ((regexp (car hi-lock-pattern)))
-               (if (string-match regexp hi-text)
-                   (push regexp regexps))))))))
+	    (when (string-match regexp hi-text)
+	      (push regexp regexps))))))
+    regexps))
 
 ;;;###autoload
 (defalias 'unhighlight-regexp 'hi-lock-unface-buffer)
@@ -529,9 +562,7 @@ then remove all hi-lock highlighting."
                           (list (car pattern)
                                 (format
                                  "%s (%s)" (car pattern)
-                                 (symbol-name
-                                  (car
-                                   (cdr (car (cdr (car (cdr pattern))))))))
+				 (cadr (cadr (cadr pattern))))
                                 (cons nil nil)
                                 (car pattern)))
                         hi-lock-interactive-patterns))))
@@ -557,6 +588,7 @@ then remove all hi-lock highlighting."
   (dolist (keyword (if (eq regexp t) hi-lock-interactive-patterns
                      (list (assoc regexp hi-lock-interactive-patterns))))
     (when keyword
+      (setq regexp (car keyword))
       (font-lock-remove-keywords nil (list keyword))
       (setq hi-lock-interactive-patterns
             (delq keyword hi-lock-interactive-patterns))
@@ -615,31 +647,36 @@ not suitable."
       (error "Regexp cannot match an empty string")
     regexp))
 
-(defun hi-lock-read-face-name ()
+(defun hi-lock-read-face-name (&optional toggle-auto-select)
   "Return face name for interactive highlighting.
 When `hi-lock-auto-select-face' is non-nil, just return the next face.
-Otherwise, read face name from minibuffer with completion and history."
-  (if hi-lock-auto-select-face
+Otherwise, read face name from minibuffer with completion and history.
+
+When TOGGLE-AUTO-SELECT is non-nil, temporarily invert the value
+of `hi-lock-auto-select-face'."
+  (let ((auto-select
+	 (if toggle-auto-select (not hi-lock-auto-select-face)
+	   hi-lock-auto-select-face)))
+    (if auto-select
       ;; Return current head and rotate the face list.
       (pop hi-lock--auto-select-face-defaults)
-    (intern (completing-read
+      (intern
+       (let* ((face-names (mapcar #'face-name hi-lock-faces))
+	      (prefix (try-completion "" face-names)))
+	 (completing-read
              "Highlight using face: "
              obarray 'facep t
-             (cons (car hi-lock-face-defaults)
-                   (let ((prefix
-                          (try-completion
-                           (substring (car hi-lock-face-defaults) 0 1)
-                           hi-lock-face-defaults)))
+	  (cons (car face-names)
                      (if (and (stringp prefix)
-                              (not (equal prefix (car hi-lock-face-defaults))))
-                         (length prefix) 0)))
-             'face-name-history
-	     (cdr hi-lock-face-defaults)))))
+			 (not (equal prefix (car face-names))))
+		    (length prefix) 0))
+	  'face-name-history (cdr face-names)))))))
 
 (defun hi-lock-set-pattern (regexp face)
   "Highlight REGEXP with face FACE."
   (let ((pattern (list regexp (list 0 (list 'quote face) t))))
-    (unless (member pattern hi-lock-interactive-patterns)
+    ;; Check if REGEXP is already highlighted.
+    (unless (assoc regexp hi-lock-interactive-patterns)
       (push pattern hi-lock-interactive-patterns)
       (if font-lock-mode
 	  (progn


[-- Attachment #3: Type: text/plain, Size: 3085 bytes --]


> There are three issues that I see with your commmit:
>
> Issue-1:  face-at-point broken?
> ===============================
>
> M-x toggle-debug-on-error RET
> M-x find-function RET face-at-point RET
> C-x w h
> C-x w r
>
>     Debugger entered--Lisp error: (error "Not a face: nil")
>       signal(error ("Not a face: nil"))
>       error("Not a face: %s" nil)
>       check-face(nil)
>       face-name(nil)
>       hi-lock--regexps-at-point()
>       byte-code("\b\203\a\305C\207\306 \203.	<\203.\n\203.\307\310\215\207\v\204!\311\312!\210\313 \314\f\204-\315\2022\316\317\f@\"\v\320\305\320\211\f&\a)C\207" [current-prefix-arg last-nonmenu-event use-dialog-box hi-lock-interactive-patterns defaults t display-popup-menus-p snafu (byte-code "\301\302\303\304\305\306\b\"BB\"\206.\307\310\311\"\207" [hi-lock-interactive-patterns x-popup-menu t keymap "Select Pattern to Unhighlight" mapcar #[(pattern) "\b@\301\302\b@\303\bA@A@A@!#\304\211B\b@F\207" [pattern format "%s (%s)" symbol-name nil] 6] throw snafu ("")] 7) error "No highlighting to remove" hi-lock--regexps-at-point completing-read "Regexp to unhighlight: " format "Regexp to unhighlight (default %s): " nil] 8)
>       call-interactively(unhighlight-regexp nil nil)
>
> The reason is faceprop happens to be a string
>
> (get-char-property (point) 'face) 
>  : "hi-yellow"
>
> Issue-2:  Various issues with unhighlighting 
> ============================================
>
> Once you fix Issue-1 you will run in to other issues with
> un-highlighting.  Try highlighting and UN-highlighting in following 3
> ways
>
> 1. Buffer with font-lock-mode ON
> 2. Buffer with font-lock-mode OFF
> 3. Unhighlight from the menu
>
> Caveat:  Extra testing needed if /type/ of face names are changed 
> =================================================================
>
> hi-lock-face-defautls is currently a list of face names (stringp).  If
> it is made a defcustom, it will be cast to a list of symbols (symbolp).
> In some places, face names are expected and in some other places face as
> a symbol is used.  So you need to re-run the tests if move from
> string->symbols.
>
> Suggestion: In default faces, don't mix bold and foreground/background
> =======================================================================
>
> I am OK with defcustom of faces.  Something like
>
>     (defcustom hi-lock-face-defaults
>       '(hi-yellow hi-pink hi-green hi-blue hi-black-b
>                   hi-blue-b hi-red-b hi-green-b hi-black-hb)
>       "Default faces for hi-lock interactive functions."
>       :type '(repeat face)
>       :group 'hi-lock-faces)
>
> Bonus points if the default settings of the faces that go in there is
> revised as part of this bug.  I want to highlight variables in a buffer.
> So consistent policy of highlighting - a changed background of normal
> face - will require no additional work.
>
> Here is how my own faces look like.  Note that the first 4 come from
> "blue" space and the later 4 or so come from "pink" space, all chosen
> using agave.
>
> ps:  I will let you install a change for the above issues.

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

> (get-char-property (point) 'face) 
>  : "hi-yellow"

I just fixed that a little while ago.

> Issue-2:  Various issues with unhighlighting 
> ============================================
> Once you fix Issue-1 you will run in to other issues with
> un-highlighting.  Try highlighting and UN-highlighting in following 3
> ways
> 1. Buffer with font-lock-mode ON
> 2. Buffer with font-lock-mode OFF
> 3. Unhighlight from the menu

Seems to be working now (haven't tried before my recent changes, tho).

>     (defcustom hi-lock-face-defaults
>       '(hi-yellow hi-pink hi-green hi-blue hi-black-b
>                   hi-blue-b hi-red-b hi-green-b hi-black-hb)
>       "Default faces for hi-lock interactive functions."
>       :type '(repeat face)
>       :group 'hi-lock-faces)

I think it'd be important to make it possible/easy for the user to add
new faces of his own creation in there.  So we'd need a :setter that
creates those faces as needed.

BTW, I wouldn't spend too much time on hi-lock-auto-select-face since
setting it to t just saves you from typing RET RET instead of RET.
I'm not even sure we want to keep such configuration.


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Drew Adams]

> >     (defcustom hi-lock-face-defaults
> >       '(hi-yellow hi-pink hi-green hi-blue hi-black-b
> >                   hi-blue-b hi-red-b hi-green-b hi-black-hb)
> >       "Default faces for hi-lock interactive functions."
> >       :type '(repeat face)
> >       :group 'hi-lock-faces)
> 
> I think it'd be important to make it possible/easy for the user to add
> new faces of his own creation in there.  So we'd need a :setter that
> creates those faces as needed.

The code still defines and uses faces named for colors.  Bad idea.  There is no
need for that, and nothing gained by it.

Look at the doc for each of those faces - it should give you a clue.

It says only that this is a face for hi-lock mode.  It says nothing about the
color that is in the name, and rightfully so.

The default color used to define the face is irrelevant to the
use/meaning/purpose of the face.  These faces should be renamed.

If you need a separate bug report for that I will be glad to file it.

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

>> (get-char-property (point) 'face) 
>>  : "hi-yellow"
>
> I just fixed that a little while ago.
>
>> Issue-2:  Various issues with unhighlighting 
>> ============================================
>> Once you fix Issue-1 you will run in to other issues with
>> un-highlighting.  Try highlighting and UN-highlighting in following 3
>> ways
>> 1. Buffer with font-lock-mode ON
>> 2. Buffer with font-lock-mode OFF
>> 3. Unhighlight from the menu
>
> Seems to be working now (haven't tried before my recent changes, tho).

By now, if you mean 111134, the unhighlighting is not working as
expected.

My patch has hints on where the brokenness comes from.

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

>> Seems to be working now (haven't tried before my recent changes, tho).
> By now, if you mean 111134, the unhighlighting is not working as
> expected.

Can you give a recipe, because "it works for me",


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Seems to be working now (haven't tried before my recent changes, tho).
>> By now, if you mean 111134, the unhighlighting is not working as
>> expected.
>
> Can you give a recipe, because "it works for me",

How about a screenshot? This is one of the brokenness.

Recipe-1:

    Couple of C-x w h, followed by C-x w r.  Note the cursor position
    and the choice offered.  See screenshot.  Why is there no default
    offered and why even absurd candidates are offered?

Recipe-2:

    Turn off font-lock-mode.

    Couple of C-x w h and then try C-x w r, C-u C-x w r, try from menu.




[-- Attachment #2: unhighlight-1.png --]
[-- Type: image/png, Size: 11744 bytes --]

[-- Attachment #3: unhighlight-2.png --]
[-- Type: image/png, Size: 12881 bytes --]

[-- Attachment #4: Type: text/plain, Size: 21 bytes --]





>         Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

>>>> Seems to be working now (haven't tried before my recent changes, tho).
>>> By now, if you mean 111134, the unhighlighting is not working as
>>> expected.
>> Can you give a recipe, because "it works for me",
> How about a screenshot?

That can work as well, tho since it's not a display problem,
explanations work at least as well, usually better.
I never use hi-lock, so don't assume I know how it's expected to behave.

> Recipe-1:
>     Couple of C-x w h, followed by C-x w r.  Note the cursor position
>     and the choice offered.

I don't see anything wrong with the cursor position.  Please explain.

>     See screenshot.  Why is there no default offered

Don't know, should there be one?

>     and why even absurd candidates are offered?

Which absurd candidates are you talking about?

> Recipe-2:
>     Turn off font-lock-mode.
>     Couple of C-x w h and then try C-x w r, C-u C-x w r, try from menu.

What am I supposed to see when I try that?


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>>> Seems to be working now (haven't tried before my recent changes, tho).
>>>> By now, if you mean 111134, the unhighlighting is not working as
>>>> expected.
>>> Can you give a recipe, because "it works for me",
>> How about a screenshot?
>
> That can work as well, tho since it's not a display problem,
> explanations work at least as well, usually better.
> I never use hi-lock, so don't assume I know how it's expected to
> behave.

This is the behaviour I am expecting.  This is what I
circulated in etc/NEWS entry.

    *** Unhighlighting command (`hi-lock-unface-buffer') now un-highlights
    text at point.  When called interactively with C-u, removes all
    highlighting in current buffer.

We seem to be talking too much but not taking a moment to understand to
each other.  I will exchange no more mails with you in this thread,
sorry.  You reply, but with no effort on your part to understand what I
am saying or showing you.  I am happy with whatever you conceive to be
useful.

>
>> Recipe-1:
>>     Couple of C-x w h, followed by C-x w r.  Note the cursor position
>>     and the choice offered.
>
> I don't see anything wrong with the cursor position.  Please explain.
>
>>     See screenshot.  Why is there no default offered
>
> Don't know, should there be one?
>
>>     and why even absurd candidates are offered?
>
> Which absurd candidates are you talking about?
>
>> Recipe-2:
>>     Turn off font-lock-mode.
>>     Couple of C-x w h and then try C-x w r, C-u C-x w r, try from menu.
>
> What am I supposed to see when I try that?
>
>
>         Stefan
>
>
>
>

-- 

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

> This is the behaviour I am expecting.  This is what I
> circulated in etc/NEWS entry.

>     *** Unhighlighting command (`hi-lock-unface-buffer') now un-highlights
>     text at point.  When called interactively with C-u, removes all
>     highlighting in current buffer.

Oh, right, I had forgotten about that.  Indeed, the default regexp
choice had a bug and a misfeature.  Should be fixed now.

> We seem to be talking too much but not taking a moment to understand to
> each other.

I generally have many conversions in flight at the same time, so it
really helps if you repeat more of the context.  E.g. making it clear
whether you're talking about a regression, an old bug, a request for
a new feature, a reminder for god-knows-what, ...

> You reply, but with no effort on your part to understand what I
> am saying or showing you.

I'm sorry you feel I'm not making enough efforts.  Maintaining Emacs
takes a lot of time, so please bear with me and try to help me
understand faster by giving me as many details as possible.
For example, often sending a short patch of code is a good way forward,
or a *precise* recipe telling what happens and what should have happened
instead (I assure you I won't be offended even if some parts are too
obvious).


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


Attaching a patch.  See ChangeLog for details.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-rev111152-1.diff --]
[-- Type: text/x-diff, Size: 3131 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-07 16:48:42 +0000
+++ lisp/ChangeLog	2012-12-08 12:43:29 +0000
@@ -1,3 +1,11 @@
+2012-12-08  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-lock--regexps-at-point): Use a better heuristic
+	that depends on actual faces rather than their common prefix.
+	(hi-lock-unface-buffer): Fix unhighlight all, when using overlays.
+	(hi-lock-set-pattern): Refuse to highlight an already highlighted
+	regexp.
+
 2012-12-07  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* hi-lock.el (hi-lock-unface-buffer): If there's no matching regexp at

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-12-07 16:48:42 +0000
+++ lisp/hi-lock.el	2012-12-08 10:38:25 +0000
@@ -471,19 +471,19 @@
     (let ((regexp (get-char-property (point) 'hi-lock-overlay-regexp)))
       (when regexp (push regexp regexps)))
     ;; With font-locking on, check if the cursor is on an highlighted text.
-    ;; Checking for hi-lock face is a good heuristic.  FIXME: use "hi-lock-".
-    (and (string-match "\\`hi-" (face-name (face-at-point)))
-         (let* ((hi-text
-                 (buffer-substring-no-properties
-                  (previous-single-property-change (point) 'face)
-                  (next-single-property-change (point) 'face))))
-           ;; Compute hi-lock patterns that match the
-           ;; highlighted text at point.  Use this later in
-           ;; during completing-read.
-           (dolist (hi-lock-pattern hi-lock-interactive-patterns)
-             (let ((regexp (car hi-lock-pattern)))
-               (if (string-match regexp hi-text)
-                   (push regexp regexps))))))
+    (and (member (list 'quote (face-at-point))
+		 (mapcar #'cadadr hi-lock-interactive-patterns))
+	 (let* ((hi-text
+		 (buffer-substring-no-properties
+		  (previous-single-property-change (point) 'face)
+		  (next-single-property-change (point) 'face))))
+	   ;; Compute hi-lock patterns that match the
+	   ;; highlighted text at point.  Use this later in
+	   ;; during completing-read.
+	   (dolist (hi-lock-pattern hi-lock-interactive-patterns)
+	     (let ((regexp (car hi-lock-pattern)))
+	       (if (string-match regexp hi-text)
+		   (push regexp regexps))))))
     regexps))
 
 (defvar-local hi-lock--last-face nil)
@@ -541,6 +541,7 @@
   (dolist (keyword (if (eq regexp t) hi-lock-interactive-patterns
                      (list (assoc regexp hi-lock-interactive-patterns))))
     (when keyword
+      (setq regexp (car keyword))
       (let ((face (cadr (cadr (cadr keyword)))))
         ;; Make `face' the next one to use by default.
         (setq hi-lock--last-face
@@ -628,7 +629,8 @@
   ;; Hashcons the regexp, so it can be passed to remove-overlays later.
   (setq regexp (hi-lock--hashcons regexp))
   (let ((pattern (list regexp (list 0 (list 'quote face) t))))
-    (unless (member pattern hi-lock-interactive-patterns)
+    ;; Refuse to highlight a text that is already highlighted.
+    (unless (assoc regexp hi-lock-interactive-patterns)
       (push pattern hi-lock-interactive-patterns)
       (if font-lock-mode
 	  (progn

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


This patch which /improves/ status quo. Applies cleanly on top of
earlier patch at http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11095#94

This patch makes sure that faces used for highlighting are always
distinct and recycles them only when it is inevitable.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-rev111152-2.diff --]
[-- Type: text/x-diff, Size: 3477 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-10 04:18:24 +0000
+++ lisp/ChangeLog	2012-12-10 04:18:59 +0000
@@ -1,3 +1,11 @@
+2012-12-10  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-lock--last-face): Remove it.
+	(hi-lock--unused-faces): New variable, a free list of faces
+	available for highlighting text.
+	(hi-lock-unface-buffer, hi-lock-read-face-name): Propagate above
+	changes.
+
 2012-12-08  Jambunathan K  <kjambunathan@gmail.com>
 
 	* hi-lock.el (hi-lock--regexps-at-point): Use a better heuristic

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-12-08 13:06:46 +0000
+++ lisp/hi-lock.el	2012-12-10 04:06:21 +0000
@@ -486,7 +486,9 @@ updated as you type."
 		   (push regexp regexps))))))
     regexps))
 
-(defvar-local hi-lock--last-face nil)
+(defvar-local hi-lock--unused-faces nil
+  "List of faces that is not used and is available for highlighting new text.
+Face names from this list come from `hi-lock-face-defaults'.")
 
 ;;;###autoload
 (defalias 'unhighlight-regexp 'hi-lock-unface-buffer)
@@ -544,9 +546,7 @@ then remove all hi-lock highlighting."
       (setq regexp (car keyword))
       (let ((face (cadr (cadr (cadr keyword)))))
         ;; Make `face' the next one to use by default.
-        (setq hi-lock--last-face
-              (cadr (member (symbol-name face)
-                            (reverse hi-lock-face-defaults)))))
+	(add-to-list 'hi-lock--unused-faces (face-name face)))
       (font-lock-remove-keywords nil (list keyword))
       (setq hi-lock-interactive-patterns
             (delq keyword hi-lock-interactive-patterns))
@@ -609,20 +609,26 @@ not suitable."
   "Return face for interactive highlighting.
 When `hi-lock-auto-select-face' is non-nil, just return the next face.
 Otherwise, read face name from minibuffer with completion and history."
-  (let ((default (or (cadr (member hi-lock--last-face hi-lock-face-defaults))
-                      (car hi-lock-face-defaults))))
-    (setq hi-lock--last-face
+  (unless hi-lock-interactive-patterns
+    (setq hi-lock--unused-faces hi-lock-face-defaults))
+  (let* ((last-used-face
+	  (when hi-lock-interactive-patterns
+	    (face-name (cadar (cdar (cdar hi-lock-interactive-patterns))))))
+	 (defaults (append hi-lock--unused-faces
+			   (cdr (member last-used-face hi-lock-face-defaults))
+			   hi-lock-face-defaults))
+	 face)
           (if (and hi-lock-auto-select-face (not current-prefix-arg))
-              default
-            (completing-read
-             (format "Highlight using face (default %s): " default)
-             obarray 'facep t nil 'face-name-history
-             (append (member default hi-lock-face-defaults)
-                     hi-lock-face-defaults))))
-    (unless (member hi-lock--last-face hi-lock-face-defaults)
-      (setq hi-lock-face-defaults
-            (append hi-lock-face-defaults (list hi-lock--last-face))))
-    (intern hi-lock--last-face)))
+	(setq face (or (pop hi-lock--unused-faces) (car defaults)))
+      (setq face (completing-read
+		  (format "Highlight using face (default %s): "
+			  (car defaults))
+		  obarray 'facep t nil 'face-name-history defaults))
+      ;; Update list of un-used faces.
+      (setq hi-lock--unused-faces (delete face hi-lock--unused-faces))
+      ;; Grow the list of defaults.
+      (add-to-list 'hi-lock-face-defaults face t))
+    (intern face)))
 
 (defun hi-lock-set-pattern (regexp face)
   "Highlight REGEXP with face FACE."

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

> This patch makes sure that faces used for highlighting are always
> distinct and recycles them only when it is inevitable.

Installed with 2 changes:
- replace "delete" with "remove" because it was applied to a list which
  is reachable from other places (basically, from other buffer's
  hi-lock--unused-faces).
- introduced a helper hi-lock-keyword->face to get rid of those cadadadr
  (some of which needed `cl' even it was not `require'd).


        Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> This patch makes sure that faces used for highlighting are always
>> distinct and recycles them only when it is inevitable.
>
> Installed with 2 changes:
> - replace "delete" with "remove" because it was applied to a list which
>   is reachable from other places (basically, from other buffer's
>   hi-lock--unused-faces).

Attaching a patch with following changes.  (This is the last lap)

1. Fix following bug when `font-lock-mode' is disabled.

    ,----
    | Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
    |   buffer-substring-no-properties(nil nil)
    |   (let* ((hi-text (buffer-substring-no-properties (previous-single-property-change (point) (quote face)) (next-single-property-change (point) (quote face))))) (progn (let ((--dolist-tail-- hi-lock-interactive-patterns)) (while --dolist-tail-- (let ((hi-lock-pattern (car --dolist-tail--))) (let ((regexp ...)) (if (string-match regexp hi-text) (setq regexps ...))) (setq --dolist-tail-- (cdr --dolist-tail--)))))))
    |   (and (memq (face-at-point) (mapcar (function hi-lock-keyword->face) hi-lock-interactive-patterns)) (let* ((hi-text (buffer-substring-no-properties (previous-single-property-change (point) (quote face)) (next-single-property-change (point) (quote face))))) (progn (let ((--dolist-tail-- hi-lock-interactive-patterns)) (while --dolist-tail-- (let ((hi-lock-pattern ...)) (let (...) (if ... ...)) (setq --dolist-tail-- (cdr --dolist-tail--))))))))
    |   (let ((regexps (quote nil))) (let ((regexp (get-char-property (point) (quote hi-lock-overlay-regexp)))) (if regexp (progn (setq regexps (cons regexp regexps))))) (and (memq (face-at-point) (mapcar (function hi-lock-keyword->face) hi-lock-interactive-patterns)) (let* ((hi-text (buffer-substring-no-properties (previous-single-property-change (point) (quote face)) (next-single-property-change (point) (quote face))))) (progn (let ((--dolist-tail-- hi-lock-interactive-patterns)) (while --dolist-tail-- (let (...) (let ... ...) (setq --dolist-tail-- ...))))))) regexps)
    |   hi-lock--regexps-at-point()
    |   (or (hi-lock--regexps-at-point) (mapcar (function car) hi-lock-interactive-patterns))
    |   (let* ((defaults (or (hi-lock--regexps-at-point) (mapcar (function car) hi-lock-interactive-patterns)))) (list (completing-read (if (null defaults) "Regexp to unhighlight: " (format "Regexp to unhighlight (default %s): " (car defaults))) hi-lock-interactive-patterns nil t nil nil defaults)))
    |   (cond (current-prefix-arg (list t)) ((and (display-popup-menus-p) (listp last-nonmenu-event) use-dialog-box) (catch (quote snafu) (or (x-popup-menu t (cons (quote keymap) (cons "Select Pattern to Unhighlight" (mapcar ... hi-lock-interactive-patterns)))) (throw (quote snafu) (quote ("")))))) (t (if hi-lock-interactive-patterns nil (error "No highlighting to remove")) (let* ((defaults (or (hi-lock--regexps-at-point) (mapcar (function car) hi-lock-interactive-patterns)))) (list (completing-read (if (null defaults) "Regexp to unhighlight: " (format "Regexp to unhighlight (default %s): " (car defaults))) hi-lock-interactive-patterns nil t nil nil defaults)))))
    |   call-interactively(unhighlight-regexp nil nil)
    `----

2. Add a NEWS entry.
3. Re-works how hi-lock--unused-faces is allocated.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-rev111175.patch --]
[-- Type: text/x-diff, Size: 3087 bytes --]

=== modified file 'etc/NEWS'
--- etc/NEWS	2012-12-10 12:38:49 +0000
+++ etc/NEWS	2012-12-10 20:08:35 +0000
@@ -92,6 +92,12 @@ when its arg ADJACENT is non-nil (when c
 it works like the utility `uniq'.  Otherwise by default it deletes
 duplicate lines everywhere in the region without regard to adjacency.
 
+** Interactive highlighting
+*** New user variable `hi-lock-auto-select-face'.
+*** Unhighlighting command (`hi-lock-unface-buffer') now un-highlights
+text at point.  When called interactively with a non-nil prefix,
+removes all highlighting in current buffer.
+
 ** Tramp
 +++
 *** New connection method "adb", which allows to access Android

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-10 18:33:59 +0000
+++ lisp/ChangeLog	2012-12-10 20:19:36 +0000
@@ -1,5 +1,14 @@
 2012-12-10  Jambunathan K  <kjambunathan@gmail.com>
 
+	* hi-lock.el (hi-lock--regexps-at-point): When `font-lock-mode' is
+	disabled, highlighting will use overlays.  So use
+	`*-single-char-property-change' instead of
+	``*-single-property-change'.
+	(hi-lock-read-face-name): Initialize `hi-lock--unused-faces' only
+	once.
+
+2012-12-10  Jambunathan K  <kjambunathan@gmail.com>
+
 	* hi-lock.el: Refine the choice of default face.
 	(hi-lock-keyword->face): New function.  Use it wherever we used
 	cadadadr instead.

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-12-10 18:33:59 +0000
+++ lisp/hi-lock.el	2012-12-10 19:42:51 +0000
@@ -478,8 +478,8 @@ updated as you type."
                (mapcar #'hi-lock-keyword->face hi-lock-interactive-patterns))
 	 (let* ((hi-text
 		 (buffer-substring-no-properties
-		  (previous-single-property-change (point) 'face)
-		  (next-single-property-change (point) 'face))))
+		  (previous-single-char-property-change (point) 'face)
+		  (next-single-char-property-change (point) 'face))))
 	   ;; Compute hi-lock patterns that match the
 	   ;; highlighted text at point.  Use this later in
 	   ;; during completing-read.
@@ -611,8 +611,10 @@ not suitable."
   "Return face for interactive highlighting.
 When `hi-lock-auto-select-face' is non-nil, just return the next face.
 Otherwise, read face name from minibuffer with completion and history."
-  (unless hi-lock-interactive-patterns
-    (setq hi-lock--unused-faces hi-lock-face-defaults))
+  (unless (or hi-lock-interactive-patterns hi-lock--unused-faces)
+    ;; This is the very first request for interactive highlighting.
+    ;; Initialize unused faces list.
+    (setq hi-lock--unused-faces (copy-sequence hi-lock-face-defaults)))
   (let* ((last-used-face
 	  (when hi-lock-interactive-patterns
 	    (face-name (hi-lock-keyword->face
@@ -628,7 +630,7 @@ Otherwise, read face name from minibuffe
 			  (car defaults))
 		  obarray 'facep t nil 'face-name-history defaults))
       ;; Update list of un-used faces.
-      (setq hi-lock--unused-faces (remove face hi-lock--unused-faces))
+      (setq hi-lock--unused-faces (delete face hi-lock--unused-faces))
       ;; Grow the list of defaults.
       (add-to-list 'hi-lock-face-defaults face t))
     (intern face)))


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]




> - introduced a helper hi-lock-keyword->face to get rid of those cadadadr
>   (some of which needed `cl' even it was not `require'd).
>
>
>         Stefan

bug#11095: [PATCH] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Stefan Monnier]

>  	 (let* ((hi-text
>  		 (buffer-substring-no-properties
> -		  (previous-single-property-change (point) 'face)
> -		  (next-single-property-change (point) 'face))))
> +		  (previous-single-char-property-change (point) 'face)
> +		  (next-single-char-property-change (point) 'face))))
>  	   ;; Compute hi-lock patterns that match the
>  	   ;; highlighted text at point.  Use this later in
>  	   ;; during completing-read.

But when overlays are used, the previous code should have found the
overlay already.  IIUC the above patch works only because of an
inconsistency between previous-single-property-change and
previous-single-char-property-change where the first may return nil
whereas the other always returns an integer.

But now that I look at this code I see that it doesn't work right for
its intended use case (i.e. when font-lock-mode is on) when point is at the
very start/end of the matched.

So I've installed a completely different patch instead (see below).
There should be an easier way to do it :-(

> -  (unless hi-lock-interactive-patterns
> -    (setq hi-lock--unused-faces hi-lock-face-defaults))
> +  (unless (or hi-lock-interactive-patterns hi-lock--unused-faces)
> +    ;; This is the very first request for interactive highlighting.
> +    ;; Initialize unused faces list.
> +    (setq hi-lock--unused-faces (copy-sequence hi-lock-face-defaults)))
[...]
> -      (setq hi-lock--unused-faces (remove face hi-lock--unused-faces))
> +      (setq hi-lock--unused-faces (delete face hi-lock--unused-faces))

Why?


        Stefan


=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-12-10 18:33:59 +0000
+++ lisp/hi-lock.el	2012-12-10 21:16:31 +0000
@@ -474,19 +474,33 @@
     (let ((regexp (get-char-property (point) 'hi-lock-overlay-regexp)))
       (when regexp (push regexp regexps)))
     ;; With font-locking on, check if the cursor is on a highlighted text.
-    (and (memq (face-at-point)
-               (mapcar #'hi-lock-keyword->face hi-lock-interactive-patterns))
+    (let ((face-after (get-text-property (point) 'face))
+          (face-before
+           (unless (bobp) (get-text-property (1- (point)) 'face)))
+          (faces (mapcar #'hi-lock-keyword->face
+                         hi-lock-interactive-patterns)))
+      (unless (memq face-before faces) (setq face-before nil))
+      (unless (memq face-after faces) (setq face-after nil))
+      (when (and face-before face-after (not (eq face-before face-after)))
+        (setq face-before nil))
+      (when (or face-after face-before)
 	 (let* ((hi-text
 		 (buffer-substring-no-properties
-		  (previous-single-property-change (point) 'face)
-		  (next-single-property-change (point) 'face))))
+                 (if face-before
+                     (or (previous-single-property-change (point) 'face)
+                         (point-min))
+                   (point))
+                 (if face-after
+                     (or (next-single-property-change (point) 'face)
+                         (point-max))
+                   (point)))))
 	   ;; Compute hi-lock patterns that match the
 	   ;; highlighted text at point.  Use this later in
 	   ;; during completing-read.
 	   (dolist (hi-lock-pattern hi-lock-interactive-patterns)
 	     (let ((regexp (car hi-lock-pattern)))
 	       (if (string-match regexp hi-text)
-		   (push regexp regexps))))))
+                  (push regexp regexps)))))))
     regexps))
 
 (defvar-local hi-lock--unused-faces nil

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


Here is a patch for Part-1/Item-2.  This patch applies ON TOP OF
bug11095-part1.patch attached with
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11095#8)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-part2.patch --]
[-- Type: text/x-diff, Size: 2989 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-10 20:23:49 +0000
+++ lisp/ChangeLog	2012-10-10 21:59:39 +0000
@@ -1,5 +1,11 @@
 2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
 
+	* hi-lock.el (hi-lock-auto-select-face): New user variable.
+	(hi-lock-auto-select-face-defaults): New buffer local variable.
+	(hi-lock-read-face-name): Honor `hi-lock-auto-select-face'.
+
+2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
+
 	* hi-lock.el (hi-yellow, hi-pink, hi-green, hi-blue, hi-black-b)
         (hi-blue-b, hi-green-b, hi-red-b, hi-black-hb): Mark these faces
 	as obsolete.

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-10 20:23:49 +0000
+++ lisp/hi-lock.el	2012-10-10 21:44:28 +0000
@@ -135,6 +135,14 @@ patterns."
 ;; It can have a function value.
 (put 'hi-lock-file-patterns-policy 'risky-local-variable t)
 
+(defcustom hi-lock-auto-select-face nil
+  "Non-nil if highlighting commands should not prompt for face names.
+When non-nil, each hi-lock command will cycle through faces in
+`hi-lock-face-defaults'."
+  :type 'boolean
+  :group 'hi-lock
+  :version "24.3")
+
 (defgroup hi-lock-faces nil
   "Faces for hi-lock."
   :group 'hi-lock
@@ -221,8 +229,15 @@ patterns."
     "hi-lock-6" "hi-lock-7" "hi-lock-8" "hi-lock-9")
   "Default faces for hi-lock interactive functions.")
 
-;;(dolist (f hi-lock-face-defaults)
-;;  (unless (facep f) (error "%s not a face" f)))
+(defvar hi-lock-auto-select-face-defaults
+  (let ((l (copy-sequence hi-lock-face-defaults)))
+    (setcdr (last l) l))
+  "Circular list of faces used for interactive highlighting.
+When `hi-lock-auto-select-face' is non-nil, use the face at the
+head of this list for next interactive highlighting.  See also
+`hi-lock-read-face-name'.")
+
+(make-variable-buffer-local 'hi-lock-auto-select-face-defaults)
 
 (define-obsolete-variable-alias 'hi-lock-regexp-history
                                 'regexp-history
@@ -577,7 +592,15 @@ not suitable."
     regexp))
 
 (defun hi-lock-read-face-name ()
-  "Read face name from minibuffer with completion and history."
+  "Return face name for interactive highlighting.
+When `hi-lock-auto-select-face' is non-nil, return head of
+`hi-lock-auto-select-face-defaults'.  Otherwise, read face name
+from minibuffer with completion and history."
+  (if hi-lock-auto-select-face
+      ;; Return current head and rotate the face list.
+      (prog1 (car hi-lock-auto-select-face-defaults)
+	(setq hi-lock-auto-select-face-defaults
+	      (cdr hi-lock-auto-select-face-defaults)))
   (intern (completing-read
            "Highlight using face: "
            obarray 'facep t
@@ -590,7 +613,7 @@ not suitable."
                             (not (equal prefix (car hi-lock-face-defaults))))
                        (length prefix) 0)))
            'face-name-history
-	   (cdr hi-lock-face-defaults))))
+	     (cdr hi-lock-face-defaults)))))
 
 (defun hi-lock-set-pattern (regexp face)
   "Highlight REGEXP with face FACE."


[-- Attachment #3: Type: text/plain, Size: 5127 bytes --]


> Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
> Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
> current customization I have in my .emacs.
>
> I believe that my proposal is useful in general.  So I request that it
> be folded in to Emacs-24.1.
>
> Part-I: `hi-lock-face-buffer' & Co.
> ----------------------------------
>
> 1) Review the face names used in `hi-lock-face-defaults' and make the
>    faces customizable.  The defaults may not look good on a user's his
>    own font-lock configuration.
>
> 2) Make `hi-lock-face-buffer' use a different face on each invocation.  
>
>    Here is a real-world usecase for the above request.
>
>    As a programmer, I use highlighting to trace variable dependencies
>    within a function.  For example, in the example below, after
>    highlighting the variables in __different__ faces, I will come to the
>    conclusion that "a" depends on "d" and "tmp".
>
>      c = d;
>      b = c + tmp;
>      a = b;
>
>    And I use this very often to track variables and how they get their
>    values from.
>
>    If I were to use the default Emacs provided behaviour then I would
>    have to press M-n multiple times as I highlight more and more
>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>    highlighting.)
>
> See elisp snippet at the end of the mail.
>
>
> Part-II: `unhighlight-regexp'
> ------------------------------
>
> See usecase in Part-I/Item-2 
>
> 1) I want to selectively turn-off highlighting for certain regexps
>    (arguably) that _specific_ highlighted regexp cursor is stationed on.
>    This would happen when I decide that I don't want to factor a
>    particular variable for my current refactoring exercise.
>
>    I find the current behaviour of `unhighlight-regexp' slightly
>    tedious.  
>
>    1. There is no completion for which regexp I want to unhighlight and
>       I have to circle through `hi-lock-interactive-patterns'.
>
>    2. Browsing the `hi-lock-interactive-patterns' is tedious for the
>       following reasons:
>
>       - The order in which unhighlighting happens could totally be
>         unrelated to the order in which highlighting happens.  When I am
>         analyzing the variable flow, I don't want to do a "context
>         switch" trying to find out what item to choose from the
>         `unhighlight-regexp' menu.
>
> 2) I want to unhighlight all regexps.  This happens when I am done with
>    variable analysis.
>
>
> ps: I am not questioning the current defaults.  I am only saying that it
> the current behaviour is too generic to be immediately useful (atleast
> for my usecase) and so needs some sort of extra augmentation.
>
> Part-III: Elisp snippet
> -----------------------
>
> ;; Why should the color of the faces be encoded in the variable name?
> ;; Seems counter-intutitive to me.  I cannot relate to a hi-yellow
> ;; face customized to render in red.
>
> ;; (defvar hi-lock-face-defaults
> ;;   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
> ;;     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
> ;;   "Default faces for hi-lock interactive functions.")
>
> ;; So roll out my own face for highlighting.  Make them
> ;; __customizable__.  Note that the name of the face doesn't say what
> ;; the color of the highlight will be.  Works for the color theme that
> ;; I have.
> (custom-set-faces
>  '(highlight1 ((t (:background "yellow" :foreground "black"))))
>  '(highlight2 ((t (:background "OrangeRed" :foreground "black"))))
>  '(highlight3 ((((class color) (background dark)) (:background "AntiqueWhite" :foreground "black"))))
>  '(highlight4 ((t (:background "SystemHotTrackingColor"))))
>  '(highlight5 ((t (:background "VioletRed" :foreground "black")))))
>
> ;; override the Emacs default
> (setq hi-lock-face-defaults
>       '("highlight1" "highlight2" "highlight3" "highlight4" "highlight5"))
>
> (defvar hi-lock-faces
>   (let ((l (copy-list hi-lock-face-defaults)))
>     (setcdr (last l) l))
>   "Circular list of faces in `hi-lock-face-defaults'.")
>
> ;; make `hi-lock-faces' buffer local
> (make-variable-buffer-local 'hi-lock-faces)
>
> (defun highlight-symbol ()
>   "Highlight symbol at point.  
> For illustartion only.  Note the use of `hi-lock-face-buffer'.
> Choose a new face from `hi-lock-faces' on each invocation.
> Overrides the default behaviour in vanilla Emacs which is to use
> the face at the head of the list."
>   (interactive)
>   (hi-lock-face-buffer
>    (concat "\\_<" (regexp-quote (thing-at-point 'symbol)) "\\_>")
>    ;; rotate the face list
>    (prog1 (car hi-lock-faces)
>      (setq hi-lock-faces (cdr hi-lock-faces)))))
>
> (defun my-unhighlight-regexp (arg)
>   "Wrapper around `unhighlight-regexp'.
> With a prefix argument, turn off all highlighting.
>
> TODO: Check if the symbol is right now on a highlighted regexp.
> If yes, unhighlight only that regexp."
>   (interactive "P")
>   (if arg
>       (mapc (lambda (p)
> 	      (unhighlight-regexp (car p)))
> 	    hi-lock-interactive-patterns)
>     (call-interactively 'unhighlight-regexp)))
>
>
>
>

-- 

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


The attached patch, applies on top of earlier two patches. See
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11095#11

The patch allows highlighting of tag at point.  (Note that for all
practical purposes, tag at point is the symbol at point.) See
Part_I/Item-2 below for a usecase.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-part3.patch --]
[-- Type: text/x-diff, Size: 3440 bytes --]

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-10 22:01:18 +0000
+++ lisp/hi-lock.el	2012-10-11 20:02:17 +0000
@@ -433,7 +433,7 @@ updated as you type."
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight line" (car regexp-history)))
+     (read-regexp "Regexp to highlight line"))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -458,7 +458,7 @@ updated as you type."
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight" (car regexp-history)))
+     (read-regexp "Regexp to highlight"))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -480,7 +480,7 @@ updated as you type."
    (list
     (hi-lock-regexp-okay
      (hi-lock-process-phrase
-      (read-regexp "Phrase to highlight" (car regexp-history))))
+      (read-regexp "Phrase to highlight")))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -598,7 +598,7 @@ When `hi-lock-auto-select-face' is non-n
 from minibuffer with completion and history."
   (if hi-lock-auto-select-face
       ;; Return current head and rotate the face list.
-      (prog1 (car hi-lock-auto-select-face-defaults)
+      (prog1 (intern (car hi-lock-auto-select-face-defaults))
 	(setq hi-lock-auto-select-face-defaults
 	      (cdr hi-lock-auto-select-face-defaults)))
     (intern (completing-read

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2012-10-04 19:28:11 +0000
+++ lisp/replace.el	2012-10-11 19:46:42 +0000
@@ -585,27 +585,32 @@ of `history-length', which see.")
 When PROMPT doesn't end with a colon and space, it adds a final \": \".
 If DEFAULTS is non-nil, it displays the first default in the prompt.
 
-Non-nil optional arg DEFAULTS is a string or a list of strings that
-are prepended to a list of standard default values, which include the
-string at point, the last isearch regexp, the last isearch string, and
-the last replacement regexp.
+Optional arg DEFAULTS is a string or a list of strings that are
+prepended to a list of standard default values, which include the
+tag at point, the last isearch regexp, the last isearch string,
+and the last replacement regexp.
 
 Non-nil HISTORY is a symbol to use for the history list.
 If HISTORY is nil, `regexp-history' is used."
-  (let* ((default (if (consp defaults) (car defaults) defaults))
-	 (defaults
+  (let* ((defaults
 	   (append
 	    (if (listp defaults) defaults (list defaults))
-	    (list (regexp-quote
-		   (or (funcall (or find-tag-default-function
+	    (list
+	     ;; Regexp for tag at point.
+	     (let* ((tagf (or find-tag-default-function
 				    (get major-mode 'find-tag-default-function)
 				    'find-tag-default))
-		       ""))
+		    (tag (funcall tagf)))
+	       (cond ((not tag) "")
+		     ((eq tagf 'find-tag-default)
+		      (format "\\_<%s\\_>" (regexp-quote tag)))
+		     (t (regexp-quote tag))))
 		  (car regexp-search-ring)
 		  (regexp-quote (or (car search-ring) ""))
 		  (car (symbol-value
 			query-replace-from-history-variable)))))
 	 (defaults (delete-dups (delq nil (delete "" defaults))))
+	 (default (car defaults))
 	 ;; Do not automatically add default to the history for empty input.
 	 (history-add-new-input nil)
 	 (input (read-from-minibuffer


[-- Attachment #3: Type: text/plain, Size: 5127 bytes --]


> Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
> Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
> current customization I have in my .emacs.
>
> I believe that my proposal is useful in general.  So I request that it
> be folded in to Emacs-24.1.
>
> Part-I: `hi-lock-face-buffer' & Co.
> ----------------------------------
>
> 1) Review the face names used in `hi-lock-face-defaults' and make the
>    faces customizable.  The defaults may not look good on a user's his
>    own font-lock configuration.
>
> 2) Make `hi-lock-face-buffer' use a different face on each invocation.  
>
>    Here is a real-world usecase for the above request.
>
>    As a programmer, I use highlighting to trace variable dependencies
>    within a function.  For example, in the example below, after
>    highlighting the variables in __different__ faces, I will come to the
>    conclusion that "a" depends on "d" and "tmp".
>
>      c = d;
>      b = c + tmp;
>      a = b;
>
>    And I use this very often to track variables and how they get their
>    values from.
>
>    If I were to use the default Emacs provided behaviour then I would
>    have to press M-n multiple times as I highlight more and more
>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>    highlighting.)
>
> See elisp snippet at the end of the mail.
>
>
> Part-II: `unhighlight-regexp'
> ------------------------------
>
> See usecase in Part-I/Item-2 
>
> 1) I want to selectively turn-off highlighting for certain regexps
>    (arguably) that _specific_ highlighted regexp cursor is stationed on.
>    This would happen when I decide that I don't want to factor a
>    particular variable for my current refactoring exercise.
>
>    I find the current behaviour of `unhighlight-regexp' slightly
>    tedious.  
>
>    1. There is no completion for which regexp I want to unhighlight and
>       I have to circle through `hi-lock-interactive-patterns'.
>
>    2. Browsing the `hi-lock-interactive-patterns' is tedious for the
>       following reasons:
>
>       - The order in which unhighlighting happens could totally be
>         unrelated to the order in which highlighting happens.  When I am
>         analyzing the variable flow, I don't want to do a "context
>         switch" trying to find out what item to choose from the
>         `unhighlight-regexp' menu.
>
> 2) I want to unhighlight all regexps.  This happens when I am done with
>    variable analysis.
>
>
> ps: I am not questioning the current defaults.  I am only saying that it
> the current behaviour is too generic to be immediately useful (atleast
> for my usecase) and so needs some sort of extra augmentation.
>
> Part-III: Elisp snippet
> -----------------------
>
> ;; Why should the color of the faces be encoded in the variable name?
> ;; Seems counter-intutitive to me.  I cannot relate to a hi-yellow
> ;; face customized to render in red.
>
> ;; (defvar hi-lock-face-defaults
> ;;   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
> ;;     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
> ;;   "Default faces for hi-lock interactive functions.")
>
> ;; So roll out my own face for highlighting.  Make them
> ;; __customizable__.  Note that the name of the face doesn't say what
> ;; the color of the highlight will be.  Works for the color theme that
> ;; I have.
> (custom-set-faces
>  '(highlight1 ((t (:background "yellow" :foreground "black"))))
>  '(highlight2 ((t (:background "OrangeRed" :foreground "black"))))
>  '(highlight3 ((((class color) (background dark)) (:background "AntiqueWhite" :foreground "black"))))
>  '(highlight4 ((t (:background "SystemHotTrackingColor"))))
>  '(highlight5 ((t (:background "VioletRed" :foreground "black")))))
>
> ;; override the Emacs default
> (setq hi-lock-face-defaults
>       '("highlight1" "highlight2" "highlight3" "highlight4" "highlight5"))
>
> (defvar hi-lock-faces
>   (let ((l (copy-list hi-lock-face-defaults)))
>     (setcdr (last l) l))
>   "Circular list of faces in `hi-lock-face-defaults'.")
>
> ;; make `hi-lock-faces' buffer local
> (make-variable-buffer-local 'hi-lock-faces)
>
> (defun highlight-symbol ()
>   "Highlight symbol at point.  
> For illustartion only.  Note the use of `hi-lock-face-buffer'.
> Choose a new face from `hi-lock-faces' on each invocation.
> Overrides the default behaviour in vanilla Emacs which is to use
> the face at the head of the list."
>   (interactive)
>   (hi-lock-face-buffer
>    (concat "\\_<" (regexp-quote (thing-at-point 'symbol)) "\\_>")
>    ;; rotate the face list
>    (prog1 (car hi-lock-faces)
>      (setq hi-lock-faces (cdr hi-lock-faces)))))
>
> (defun my-unhighlight-regexp (arg)
>   "Wrapper around `unhighlight-regexp'.
> With a prefix argument, turn off all highlighting.
>
> TODO: Check if the symbol is right now on a highlighted regexp.
> If yes, unhighlight only that regexp."
>   (interactive "P")
>   (if arg
>       (mapc (lambda (p)
> 	      (unhighlight-regexp (car p)))
> 	    hi-lock-interactive-patterns)
>     (call-interactively 'unhighlight-regexp)))
>
>
>
>

-- 

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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

Jambunathan K <kjambunathan@gmail.com> writes:

> The attached patch, applies on top of earlier two patches. See
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11095#11
>
> The patch allows highlighting of tag at point.  (Note that for all
> practical purposes, tag at point is the symbol at point.) See
> Part_I/Item-2 below for a usecase.

> [2. bug11095-part3.patch --- text/x-diff; bug11095-part3.patch]...

Here is the Changelog entry for bug11095-part3.patch.  (Sorry for the
confusion, if any)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-part3-changelog.patch --]
[-- Type: text/x-diff, Size: 771 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-10 22:01:18 +0000
+++ lisp/ChangeLog	2012-10-11 20:13:40 +0000
@@ -1,3 +1,15 @@
+2012-10-11  Jambunathan K  <kjambunathan@gmail.com>
+
+	* replace.el (read-regexp): Tighten the regexp that matches tag.
+	When tag is retrieved with `find-tag-default', use regexp that
+	matches symbol at point.  Also update docstring.
+	* hi-lock.el (hi-lock-line-face-buffer, hi-lock-face-buffer)
+	(hi-lock-face-phrase-buffer): Don't provide a default to
+	`read-regexp'.  Effectively the default regexp matches tag at
+	point.
+	(hi-lock-read-face-name): Return face as a symbol.  Fix earlier
+	commit.
+
 2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
 
 	* hi-lock.el (hi-lock-auto-select-face): New user variable.


[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


-- 

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Juri Linkov]

> The patch allows highlighting of tag at point.  (Note that for all
> practical purposes, tag at point is the symbol at point.) See
> Part_I/Item-2 below for a usecase.
> [...]
> +	       (cond ((not tag) "")
> +		     ((eq tagf 'find-tag-default)
> +		      (format "\\_<%s\\_>" (regexp-quote tag)))
> [...]
>>    As a programmer, I use highlighting to trace variable dependencies
>>    within a function.  For example, in the example below, after
>>    highlighting the variables in __different__ faces, I will come to the
>>    conclusion that "a" depends on "d" and "tmp".
>>
>>      c = d;
>>      b = c + tmp;
>>      a = b;
>>
>>    And I use this very often to track variables and how they get their
>>    values from.
>>
>>    If I were to use the default Emacs provided behaviour then I would
>>    have to press M-n multiple times as I highlight more and more
>>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>>    highlighting.)

Would you agree that a better way to implement your proposal is to add a new
command to hi-lock.el with a name like `highlight-symbol'?  I mean there are
already such hi-lock commands as:

1. highlight-lines-matching-regexp (that corresponds to occur)
2. highlight-regexp (that corresponds to isearch-forward-regexp)
3. highlight-phrase (that corresponds to isearch-forward-word)

what is currently missing is this command:

4. highlight-symbol (that corresponds to isearch-forward-symbol)

Then both highlight-phrase and highlight-symbol could use internally
isearch functions that turn words and symbols into regexps
and that will do the right thing using search-upper-case and
search-whitespace-regexp.

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

Juri Linkov <juri@jurta.org> writes:

>> The patch allows highlighting of tag at point.  (Note that for all
>> practical purposes, tag at point is the symbol at point.) See
>> Part_I/Item-2 below for a usecase.
>> [...]
>> +	       (cond ((not tag) "")
>> +		     ((eq tagf 'find-tag-default)
>> +		      (format "\\_<%s\\_>" (regexp-quote tag)))
>> [...]
>>>    As a programmer, I use highlighting to trace variable dependencies
>>>    within a function.  For example, in the example below, after
>>>    highlighting the variables in __different__ faces, I will come to the
>>>    conclusion that "a" depends on "d" and "tmp".
>>>
>>>      c = d;
>>>      b = c + tmp;
>>>      a = b;
>>>
>>>    And I use this very often to track variables and how they get their
>>>    values from.
>>>
>>>    If I were to use the default Emacs provided behaviour then I would
>>>    have to press M-n multiple times as I highlight more and more
>>>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>>>    highlighting.)
>
> Would you agree that a better way to implement your proposal is to add a new
> command to hi-lock.el with a name like `highlight-symbol'?  I mean there are
> already such hi-lock commands as:
>
> 1. highlight-lines-matching-regexp (that corresponds to occur)
> 2. highlight-regexp (that corresponds to isearch-forward-regexp)
> 3. highlight-phrase (that corresponds to isearch-forward-word)
>
> what is currently missing is this command:
>
> 4. highlight-symbol (that corresponds to isearch-forward-symbol)
>
> Then both highlight-phrase and highlight-symbol could use internally
> isearch functions that turn words and symbols into regexps
> and that will do the right thing using search-upper-case and
> search-whitespace-regexp.

Have you tried the patches?

With my current patches, M-s h r will highlight symbol at point (more
precisely tag at point).  It may not be the best thing, but achieves the
task at hand.

Let me emphasize that the patches are mostly concerned with easy and
hands-off highlighting and un-highlighting (i.e., the highlighting
process itself - the "how" - rather than "what (regexp)" is highlighted
and how those regexes are composed.)  So the patches are valid
candidates for consideration, irrespective of your observations above.
As for handling of regexes themselves, I will rather leave it to more
experienced hands.

ps: I have one more patch to circulate - wrt unhighlighting - before I
leave the table open for further discussion.  Once I get some initial
comments, I can rework the changes and provide a (revised) consolidated
patch.

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Juri Linkov]

> Have you tried the patches?

I'm trying your patches now, thanks.  My initial reaction was to using
a symbol regexp "\\_<%s\\_>" in `read-regexp' to match a symbol at point.
I think such symbol specific defaults could be specified by callers of
`read-regexp' in its argument `DEFAULTS'.

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

Juri Linkov <juri@jurta.org> writes:

>> Have you tried the patches?
>
> I'm trying your patches now, thanks.  My initial reaction was to using
> a symbol regexp "\\_<%s\\_>" in `read-regexp' to match a symbol at point.
> I think such symbol specific defaults could be specified by callers of
> `read-regexp' in its argument `DEFAULTS'.

(This is not a response to your post, but only to record my observations
for further consideration by the reviewer)

1. Highlighting (M-shr) can happen via isearch or through hi-lock mode
   (C-xwh).  

   I don't use isearch much.  So the patch merely formalizes what I had
   in my .emacs and it is done with my hi-lock mode glasses.

2. In `read-regexp' there is a `find-tag-default-function' but no
   `find-tag-regexp-function'.  i.e., `find-tag-default' returns a
   symbol at point.  But using `regexp-quote' on the result is a poor
   choice when in should actually be returning a symbol regexp. (When I
   am renaming symbols, I highlight them and then re-name.  In that
   case, the looser regexp is unreliable and annoying.  As you may very
   well agree, it is not uncommon to have variables share a common
   prefix.)

3. `read-regexp' will use tag at point as the default regexp.  If a
    caller - say a `highlight-symbol' function or a symbol yank from
    isearch context - then it is the responsibility of the caller to
    provide the right regexp.

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


I am attaching a patch that addresses Part-II/Item-1 below.

This patch is 4-th in the series and applies on top of the third patch
seen here at http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11095#14.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-part4.patch --]
[-- Type: text/x-diff, Size: 3894 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-11 20:39:05 +0000
+++ lisp/ChangeLog	2012-10-12 16:08:17 +0000
@@ -1,3 +1,10 @@
+2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-lock-unface-buffer): Prompt user with useful
+	defaults.  Specifically, if cursor is on an highlighted text,
+	offer a regexp that matches that text as the default.  Update
+	docstring.
+
 2012-10-11  Jambunathan K  <kjambunathan@gmail.com>
 
 	* replace.el (read-regexp): Tighten the regexp that matches tag.

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-11 20:39:05 +0000
+++ lisp/hi-lock.el	2012-10-12 15:55:49 +0000
@@ -493,8 +493,15 @@ updated as you type."
 ;;;###autoload
 (defun hi-lock-unface-buffer (regexp)
   "Remove highlighting of each match to REGEXP set by hi-lock.
-Interactively, prompt for REGEXP, accepting only regexps
-previously inserted by hi-lock interactive functions."
+Interactively, prompt for REGEXP.  If the cursor is on a
+previously highlighted text and if the associated regexp can be
+inferred via simple heuristics, offer that regexp as default.
+Otherwise, prompt for REGEXP with completion and limit the
+choices to only those regexps used previously with hi-lock
+commands.
+
+If this command is invoked via menu, make no attempt to infer
+REGEXP from text at point."
   (interactive
    (if (and (display-popup-menus-p)
 	    (listp last-nonmenu-event)
@@ -522,15 +529,51 @@ previously inserted by hi-lock interacti
 	  ;; To prevent that, we return an empty string, which will
 	  ;; effectively disable the rest of the function.
 	  (throw 'snafu '(""))))
-     (let ((history-list (mapcar (lambda (p) (car p))
-                                 hi-lock-interactive-patterns)))
+     ;; Un-highlighting triggered via keyboard action.
        (unless hi-lock-interactive-patterns
          (error "No highlighting to remove"))
+     ;; Infer the regexp to un-highlight based on cursor position.
+     (let* (candidate-hi-lock-patterns
+	    (default-regexp
+	      (or
+	       ;; When using overlays, there is no ambiguity on the best
+	       ;; choice of regexp.
+	       (let ((desired-serial (get-char-property
+				      (point) 'hi-lock-overlay-regexp)))
+		 (when desired-serial
+		   (catch 'regexp
+		     (maphash
+		      (lambda (regexp serial)
+			(when (= serial desired-serial)
+			  (throw 'regexp regexp)))
+		      hi-lock-string-serialize-hash))))
+	       ;; With font-locking on, check if the cursor is on an
+	       ;; highlighted text.  Checking for hi-lock face is a
+	       ;; good heuristic.
+	       (and (string-match "\\`hi-lock-" (face-name (face-at-point)))
+		    (let* ((hi-text
+			    (buffer-substring-no-properties
+			     (previous-single-property-change (point) 'face)
+			     (next-single-property-change (point) 'face))))
+		      ;; Compute hi-lock patterns that match the
+		      ;; highlighted text at point.  Use this later in
+		      ;; during completing-read.
+		      (setq candidate-hi-lock-patterns
+			    (delq nil
+				  (mapcar
+				   (lambda (hi-lock-pattern)
+				     (let ((regexp (car hi-lock-pattern)))
+				       (and (string-match regexp hi-text)
+					    hi-lock-pattern)))
+				   hi-lock-interactive-patterns)))
+		      ;; Use regexp from the first matching pattern as
+		      ;; a reasonable default.
+		      (caar candidate-hi-lock-patterns))))))
        (list
         (completing-read "Regexp to unhighlight: "
-                         hi-lock-interactive-patterns nil t
-                         (car (car hi-lock-interactive-patterns))
-                         (cons 'history-list 1))))))
+			 (or candidate-hi-lock-patterns
+			     hi-lock-interactive-patterns)
+			 nil t default-regexp)))))
   (let ((keyword (assoc regexp hi-lock-interactive-patterns)))
     (when keyword
       (font-lock-remove-keywords nil (list keyword))


[-- Attachment #3: Type: text/plain, Size: 5127 bytes --]


> Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
> Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
> current customization I have in my .emacs.
>
> I believe that my proposal is useful in general.  So I request that it
> be folded in to Emacs-24.1.
>
> Part-I: `hi-lock-face-buffer' & Co.
> ----------------------------------
>
> 1) Review the face names used in `hi-lock-face-defaults' and make the
>    faces customizable.  The defaults may not look good on a user's his
>    own font-lock configuration.
>
> 2) Make `hi-lock-face-buffer' use a different face on each invocation.  
>
>    Here is a real-world usecase for the above request.
>
>    As a programmer, I use highlighting to trace variable dependencies
>    within a function.  For example, in the example below, after
>    highlighting the variables in __different__ faces, I will come to the
>    conclusion that "a" depends on "d" and "tmp".
>
>      c = d;
>      b = c + tmp;
>      a = b;
>
>    And I use this very often to track variables and how they get their
>    values from.
>
>    If I were to use the default Emacs provided behaviour then I would
>    have to press M-n multiple times as I highlight more and more
>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>    highlighting.)
>
> See elisp snippet at the end of the mail.
>
>
> Part-II: `unhighlight-regexp'
> ------------------------------
>
> See usecase in Part-I/Item-2 
>
> 1) I want to selectively turn-off highlighting for certain regexps
>    (arguably) that _specific_ highlighted regexp cursor is stationed on.
>    This would happen when I decide that I don't want to factor a
>    particular variable for my current refactoring exercise.
>
>    I find the current behaviour of `unhighlight-regexp' slightly
>    tedious.  
>
>    1. There is no completion for which regexp I want to unhighlight and
>       I have to circle through `hi-lock-interactive-patterns'.
>
>    2. Browsing the `hi-lock-interactive-patterns' is tedious for the
>       following reasons:
>
>       - The order in which unhighlighting happens could totally be
>         unrelated to the order in which highlighting happens.  When I am
>         analyzing the variable flow, I don't want to do a "context
>         switch" trying to find out what item to choose from the
>         `unhighlight-regexp' menu.
>
> 2) I want to unhighlight all regexps.  This happens when I am done with
>    variable analysis.
>
>
> ps: I am not questioning the current defaults.  I am only saying that it
> the current behaviour is too generic to be immediately useful (atleast
> for my usecase) and so needs some sort of extra augmentation.
>
> Part-III: Elisp snippet
> -----------------------
>
> ;; Why should the color of the faces be encoded in the variable name?
> ;; Seems counter-intutitive to me.  I cannot relate to a hi-yellow
> ;; face customized to render in red.
>
> ;; (defvar hi-lock-face-defaults
> ;;   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
> ;;     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
> ;;   "Default faces for hi-lock interactive functions.")
>
> ;; So roll out my own face for highlighting.  Make them
> ;; __customizable__.  Note that the name of the face doesn't say what
> ;; the color of the highlight will be.  Works for the color theme that
> ;; I have.
> (custom-set-faces
>  '(highlight1 ((t (:background "yellow" :foreground "black"))))
>  '(highlight2 ((t (:background "OrangeRed" :foreground "black"))))
>  '(highlight3 ((((class color) (background dark)) (:background "AntiqueWhite" :foreground "black"))))
>  '(highlight4 ((t (:background "SystemHotTrackingColor"))))
>  '(highlight5 ((t (:background "VioletRed" :foreground "black")))))
>
> ;; override the Emacs default
> (setq hi-lock-face-defaults
>       '("highlight1" "highlight2" "highlight3" "highlight4" "highlight5"))
>
> (defvar hi-lock-faces
>   (let ((l (copy-list hi-lock-face-defaults)))
>     (setcdr (last l) l))
>   "Circular list of faces in `hi-lock-face-defaults'.")
>
> ;; make `hi-lock-faces' buffer local
> (make-variable-buffer-local 'hi-lock-faces)
>
> (defun highlight-symbol ()
>   "Highlight symbol at point.  
> For illustartion only.  Note the use of `hi-lock-face-buffer'.
> Choose a new face from `hi-lock-faces' on each invocation.
> Overrides the default behaviour in vanilla Emacs which is to use
> the face at the head of the list."
>   (interactive)
>   (hi-lock-face-buffer
>    (concat "\\_<" (regexp-quote (thing-at-point 'symbol)) "\\_>")
>    ;; rotate the face list
>    (prog1 (car hi-lock-faces)
>      (setq hi-lock-faces (cdr hi-lock-faces)))))
>
> (defun my-unhighlight-regexp (arg)
>   "Wrapper around `unhighlight-regexp'.
> With a prefix argument, turn off all highlighting.
>
> TODO: Check if the symbol is right now on a highlighted regexp.
> If yes, unhighlight only that regexp."
>   (interactive "P")
>   (if arg
>       (mapc (lambda (p)
> 	      (unhighlight-regexp (car p)))
> 	    hi-lock-interactive-patterns)
>     (call-interactively 'unhighlight-regexp)))
>
>
>
>

-- 

bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


I am attaching a patch that is 5-th and last in the series for my
proposal.  This patch applies on top of the 4-th patch visible here:
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11095#26.

The patch adds an ability to un-highlight all highlighted text in the
buffer via a prefix arg.  See Part-II/Item-2 below.

ps: I am following up this mail with a consolidated patch, just in case
someone needs to try stuff out.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-part5.patch --]
[-- Type: text/x-diff, Size: 3121 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-12 16:19:16 +0000
+++ lisp/ChangeLog	2012-10-12 18:03:45 +0000
@@ -1,5 +1,10 @@
 2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
 
+	* hi-lock.el (hi-lock-unface-buffer): With prefix arg, unhighlight
+	all hi-lock patterns in buffer.
+
+2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
+
 	* hi-lock.el (hi-lock-unface-buffer): Prompt user with useful
 	defaults.  Specifically, if cursor is on an highlighted text,
 	offer a regexp that matches that text as the default.  Update

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-12 16:19:16 +0000
+++ lisp/hi-lock.el	2012-10-12 17:35:34 +0000
@@ -491,17 +491,18 @@ updated as you type."
 ;;;###autoload
 (defalias 'unhighlight-regexp 'hi-lock-unface-buffer)
 ;;;###autoload
-(defun hi-lock-unface-buffer (regexp)
+(defun hi-lock-unface-buffer (regexp &optional prefix-arg)
   "Remove highlighting of each match to REGEXP set by hi-lock.
-Interactively, prompt for REGEXP.  If the cursor is on a
-previously highlighted text and if the associated regexp can be
-inferred via simple heuristics, offer that regexp as default.
-Otherwise, prompt for REGEXP with completion and limit the
-choices to only those regexps used previously with hi-lock
-commands.
+Interactively, when PREFIX-ARG is non-nil, unhighlight all
+highlighted text in current buffer.  When PREFIX-ARG is nil,
+prompt for REGEXP.  If the cursor is on a previously highlighted
+text and if the associated regexp can be inferred via simple
+heuristics, offer that regexp as default.  Otherwise, prompt for
+REGEXP with completion and limit the choices to only those
+regexps used previously with hi-lock commands.
 
-If this command is invoked via menu, make no attempt to infer
-REGEXP from text at point."
+If this command is invoked via menu, pop-up a list of currently
+highlighted patterns."
   (interactive
    (if (and (display-popup-menus-p)
 	    (listp last-nonmenu-event)
@@ -570,18 +571,22 @@ REGEXP from text at point."
 		      ;; a reasonable default.
 		      (caar candidate-hi-lock-patterns))))))
        (list
+	(and (not current-prefix-arg)
 	(completing-read "Regexp to unhighlight: "
 			 (or candidate-hi-lock-patterns
 			     hi-lock-interactive-patterns)
-			 nil t default-regexp)))))
-  (let ((keyword (assoc regexp hi-lock-interactive-patterns)))
+			      nil t default-regexp))
+	current-prefix-arg))))
+  (dolist (re (if (not prefix-arg) (list regexp)
+		(mapcar #'car hi-lock-interactive-patterns)))
+    (let ((keyword (assoc re hi-lock-interactive-patterns)))
     (when keyword
       (font-lock-remove-keywords nil (list keyword))
       (setq hi-lock-interactive-patterns
             (delq keyword hi-lock-interactive-patterns))
       (remove-overlays
-       nil nil 'hi-lock-overlay-regexp (hi-lock-string-serialize regexp))
-      (when font-lock-fontified (font-lock-fontify-buffer)))))
+	 nil nil 'hi-lock-overlay-regexp (hi-lock-string-serialize re))
+	(when font-lock-fontified (font-lock-fontify-buffer))))))
 
 ;;;###autoload
 (defun hi-lock-write-interactive-patterns ()


[-- Attachment #3: Type: text/plain, Size: 5127 bytes --]


> Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
> Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
> current customization I have in my .emacs.
>
> I believe that my proposal is useful in general.  So I request that it
> be folded in to Emacs-24.1.
>
> Part-I: `hi-lock-face-buffer' & Co.
> ----------------------------------
>
> 1) Review the face names used in `hi-lock-face-defaults' and make the
>    faces customizable.  The defaults may not look good on a user's his
>    own font-lock configuration.
>
> 2) Make `hi-lock-face-buffer' use a different face on each invocation.  
>
>    Here is a real-world usecase for the above request.
>
>    As a programmer, I use highlighting to trace variable dependencies
>    within a function.  For example, in the example below, after
>    highlighting the variables in __different__ faces, I will come to the
>    conclusion that "a" depends on "d" and "tmp".
>
>      c = d;
>      b = c + tmp;
>      a = b;
>
>    And I use this very often to track variables and how they get their
>    values from.
>
>    If I were to use the default Emacs provided behaviour then I would
>    have to press M-n multiple times as I highlight more and more
>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>    highlighting.)
>
> See elisp snippet at the end of the mail.
>
>
> Part-II: `unhighlight-regexp'
> ------------------------------
>
> See usecase in Part-I/Item-2 
>
> 1) I want to selectively turn-off highlighting for certain regexps
>    (arguably) that _specific_ highlighted regexp cursor is stationed on.
>    This would happen when I decide that I don't want to factor a
>    particular variable for my current refactoring exercise.
>
>    I find the current behaviour of `unhighlight-regexp' slightly
>    tedious.  
>
>    1. There is no completion for which regexp I want to unhighlight and
>       I have to circle through `hi-lock-interactive-patterns'.
>
>    2. Browsing the `hi-lock-interactive-patterns' is tedious for the
>       following reasons:
>
>       - The order in which unhighlighting happens could totally be
>         unrelated to the order in which highlighting happens.  When I am
>         analyzing the variable flow, I don't want to do a "context
>         switch" trying to find out what item to choose from the
>         `unhighlight-regexp' menu.
>
> 2) I want to unhighlight all regexps.  This happens when I am done with
>    variable analysis.
>
>
> ps: I am not questioning the current defaults.  I am only saying that it
> the current behaviour is too generic to be immediately useful (atleast
> for my usecase) and so needs some sort of extra augmentation.
>
> Part-III: Elisp snippet
> -----------------------
>
> ;; Why should the color of the faces be encoded in the variable name?
> ;; Seems counter-intutitive to me.  I cannot relate to a hi-yellow
> ;; face customized to render in red.
>
> ;; (defvar hi-lock-face-defaults
> ;;   '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
> ;;     "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
> ;;   "Default faces for hi-lock interactive functions.")
>
> ;; So roll out my own face for highlighting.  Make them
> ;; __customizable__.  Note that the name of the face doesn't say what
> ;; the color of the highlight will be.  Works for the color theme that
> ;; I have.
> (custom-set-faces
>  '(highlight1 ((t (:background "yellow" :foreground "black"))))
>  '(highlight2 ((t (:background "OrangeRed" :foreground "black"))))
>  '(highlight3 ((((class color) (background dark)) (:background "AntiqueWhite" :foreground "black"))))
>  '(highlight4 ((t (:background "SystemHotTrackingColor"))))
>  '(highlight5 ((t (:background "VioletRed" :foreground "black")))))
>
> ;; override the Emacs default
> (setq hi-lock-face-defaults
>       '("highlight1" "highlight2" "highlight3" "highlight4" "highlight5"))
>
> (defvar hi-lock-faces
>   (let ((l (copy-list hi-lock-face-defaults)))
>     (setcdr (last l) l))
>   "Circular list of faces in `hi-lock-face-defaults'.")
>
> ;; make `hi-lock-faces' buffer local
> (make-variable-buffer-local 'hi-lock-faces)
>
> (defun highlight-symbol ()
>   "Highlight symbol at point.  
> For illustartion only.  Note the use of `hi-lock-face-buffer'.
> Choose a new face from `hi-lock-faces' on each invocation.
> Overrides the default behaviour in vanilla Emacs which is to use
> the face at the head of the list."
>   (interactive)
>   (hi-lock-face-buffer
>    (concat "\\_<" (regexp-quote (thing-at-point 'symbol)) "\\_>")
>    ;; rotate the face list
>    (prog1 (car hi-lock-faces)
>      (setq hi-lock-faces (cdr hi-lock-faces)))))
>
> (defun my-unhighlight-regexp (arg)
>   "Wrapper around `unhighlight-regexp'.
> With a prefix argument, turn off all highlighting.
>
> TODO: Check if the symbol is right now on a highlighted regexp.
> If yes, unhighlight only that regexp."
>   (interactive "P")
>   (if arg
>       (mapc (lambda (p)
> 	      (unhighlight-regexp (car p)))
> 	    hi-lock-interactive-patterns)
>     (call-interactively 'unhighlight-regexp)))
>
>
>
>

-- 

bug#11095: [FINAL] Re: bug#11095: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?

[Jambunathan K]

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


Here goes the final set for this bug.  The revnos. reflect the (local)
bzr revision.

0. bug11095-final-r110525.diff :: Consolidated final patch. 

1. bug11095-r110501.diff :: Introduce hi-lock-* faces.
2. bug11095-r110502.diff :: Let each highlight command choose a new face
3. bug11095-r110503.diff :: By default, highlight symbol at point
4. bug11095-r110504.diff :: Un-highlight text at point
5. bug11095-r110505.diff :: Un-highlight all highlighted text


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug11095-final-r110525.diff --]
[-- Type: text/x-diff, Size: 14898 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-12 01:01:50 +0000
+++ lisp/ChangeLog	2012-10-12 19:15:47 +0000
@@ -1,3 +1,25 @@
+2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
+
+	* replace.el (read-regexp): Tighten the regexp that matches tag.
+	When tag is retrieved with `find-tag-default', use regexp that
+	matches symbol at point.  Also update docstring.
+
+	* hi-lock.el (hi-yellow, hi-pink, hi-green, hi-blue, hi-black-b)
+        (hi-blue-b, hi-green-b, hi-red-b, hi-black-hb): Mark these faces
+	as obsolete.
+        (hi-lock-1, hi-lock-2, hi-lock-3, hi-lock-4, hi-lock-5, hi-lock-6)
+	(hi-lock-7, hi-lock-8, hi-lock-9): New faces.
+	(hi-lock-face-defaults): Use new faces.
+	(hi-lock-auto-select-face): New user variable.
+	(hi-lock-auto-select-face-defaults): New buffer local variable.
+	(hi-lock-read-face-name): Honor `hi-lock-auto-select-face'.
+	(hi-lock-line-face-buffer, hi-lock-face-buffer)
+	(hi-lock-face-phrase-buffer): Don't provide a default to
+	`read-regexp'.  Effectively, the default regexp offered by
+	`read-regexp' matches tag at point.  Use new faces.
+	(hi-lock-unface-buffer): Add support for unhighlighting (a) text
+	at point (b) all highlighted text in buffer. (Bug#11095)
+
 2012-10-12  Glenn Morris  <rgm@gnu.org>
 
 	* mail/rmailsum.el (rmail-header-summary):

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-07 00:27:31 +0000
+++ lisp/hi-lock.el	2012-10-12 19:15:47 +0000
@@ -135,12 +135,20 @@
 ;; It can have a function value.
 (put 'hi-lock-file-patterns-policy 'risky-local-variable t)
 
+(defcustom hi-lock-auto-select-face nil
+  "Non-nil if highlighting commands should not prompt for face names.
+When non-nil, each hi-lock command will cycle through faces in
+`hi-lock-face-defaults'."
+  :type 'boolean
+  :group 'hi-lock
+  :version "24.3")
+
 (defgroup hi-lock-faces nil
   "Faces for hi-lock."
   :group 'hi-lock
   :group 'faces)
 
-(defface hi-yellow
+(defface hi-lock-1
   '((((min-colors 88) (background dark))
      (:background "yellow1" :foreground "black"))
     (((background dark)) (:background "yellow" :foreground "black"))
@@ -149,13 +157,13 @@
   "Default face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-pink
+(defface hi-lock-2
   '((((background dark)) (:background "pink" :foreground "black"))
     (t (:background "pink")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-green
+(defface hi-lock-3
   '((((min-colors 88) (background dark))
      (:background "green1" :foreground "black"))
     (((background dark)) (:background "green" :foreground "black"))
@@ -164,40 +172,50 @@
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-blue
+(defface hi-lock-4
   '((((background dark)) (:background "light blue" :foreground "black"))
     (t (:background "light blue")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-black-b
+(defface hi-lock-5
   '((t (:weight bold)))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-blue-b
+(defface hi-lock-6
   '((((min-colors 88)) (:weight bold :foreground "blue1"))
     (t (:weight bold :foreground "blue")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-green-b
+(defface hi-lock-7
   '((((min-colors 88)) (:weight bold :foreground "green1"))
     (t (:weight bold :foreground "green")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-red-b
+(defface hi-lock-8
   '((((min-colors 88)) (:weight bold :foreground "red1"))
     (t (:weight bold :foreground "red")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-black-hb
+(defface hi-lock-9
   '((t (:weight bold :height 1.67 :inherit variable-pitch)))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
+(define-obsolete-face-alias 'hi-yellow 'hi-lock-1 "24.3")
+(define-obsolete-face-alias 'hi-pink 'hi-lock-2 "24.3")
+(define-obsolete-face-alias 'hi-green 'hi-lock-3 "24.3")
+(define-obsolete-face-alias 'hi-blue 'hi-lock-4 "24.3")
+(define-obsolete-face-alias 'hi-black-b 'hi-lock-5 "24.3")
+(define-obsolete-face-alias 'hi-blue-b 'hi-lock-6 "24.3")
+(define-obsolete-face-alias 'hi-green-b 'hi-lock-7 "24.3")
+(define-obsolete-face-alias 'hi-red-b 'hi-lock-8 "24.3")
+(define-obsolete-face-alias 'hi-black-hb 'hi-lock-9 "24.3")
+
 (defvar hi-lock-file-patterns nil
   "Patterns found in file for hi-lock.  Should not be changed.")
 
@@ -207,12 +225,19 @@
 (define-obsolete-variable-alias 'hi-lock-face-history
                                 'hi-lock-face-defaults "23.1")
 (defvar hi-lock-face-defaults
-  '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
-    "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
+  '("hi-lock-1" "hi-lock-2" "hi-lock-3" "hi-lock-4" "hi-lock-5"
+    "hi-lock-6" "hi-lock-7" "hi-lock-8" "hi-lock-9")
   "Default faces for hi-lock interactive functions.")
 
-;;(dolist (f hi-lock-face-defaults)
-;;  (unless (facep f) (error "%s not a face" f)))
+(defvar hi-lock-auto-select-face-defaults
+  (let ((l (copy-sequence hi-lock-face-defaults)))
+    (setcdr (last l) l))
+  "Circular list of faces used for interactive highlighting.
+When `hi-lock-auto-select-face' is non-nil, use the face at the
+head of this list for next interactive highlighting.  See also
+`hi-lock-read-face-name'.")
+
+(make-variable-buffer-local 'hi-lock-auto-select-face-defaults)
 
 (define-obsolete-variable-alias 'hi-lock-regexp-history
                                 'regexp-history
@@ -408,9 +433,9 @@
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight line" (car regexp-history)))
+     (read-regexp "Regexp to highlight line"))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern
    ;; The \\(?:...\\) grouping construct ensures that a leading ^, +, * or ?
@@ -433,9 +458,9 @@
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight" (car regexp-history)))
+     (read-regexp "Regexp to highlight"))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 
@@ -455,9 +480,9 @@
    (list
     (hi-lock-regexp-okay
      (hi-lock-process-phrase
-      (read-regexp "Phrase to highlight" (car regexp-history))))
+      (read-regexp "Phrase to highlight")))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 
@@ -466,10 +491,18 @@
 ;;;###autoload
 (defalias 'unhighlight-regexp 'hi-lock-unface-buffer)
 ;;;###autoload
-(defun hi-lock-unface-buffer (regexp)
+(defun hi-lock-unface-buffer (regexp &optional prefix-arg)
   "Remove highlighting of each match to REGEXP set by hi-lock.
-Interactively, prompt for REGEXP, accepting only regexps
-previously inserted by hi-lock interactive functions."
+Interactively, when PREFIX-ARG is non-nil, unhighlight all
+highlighted text in current buffer.  When PREFIX-ARG is nil,
+prompt for REGEXP.  If the cursor is on a previously highlighted
+text and if the associated regexp can be inferred via simple
+heuristics, offer that regexp as default.  Otherwise, prompt for
+REGEXP with completion and limit the choices to only those
+regexps used previously with hi-lock commands.
+
+If this command is invoked via menu, pop-up a list of currently
+highlighted patterns."
   (interactive
    (if (and (display-popup-menus-p)
 	    (listp last-nonmenu-event)
@@ -497,23 +530,63 @@
 	  ;; To prevent that, we return an empty string, which will
 	  ;; effectively disable the rest of the function.
 	  (throw 'snafu '(""))))
-     (let ((history-list (mapcar (lambda (p) (car p))
-                                 hi-lock-interactive-patterns)))
-       (unless hi-lock-interactive-patterns
-         (error "No highlighting to remove"))
+     ;; Un-highlighting triggered via keyboard action.
+     (unless hi-lock-interactive-patterns
+       (error "No highlighting to remove"))
+     ;; Infer the regexp to un-highlight based on cursor position.
+     (let* (candidate-hi-lock-patterns
+	    (default-regexp
+	      (or
+	       ;; When using overlays, there is no ambiguity on the best
+	       ;; choice of regexp.
+	       (let ((desired-serial (get-char-property
+				      (point) 'hi-lock-overlay-regexp)))
+		 (when desired-serial
+		   (catch 'regexp
+		     (maphash
+		      (lambda (regexp serial)
+			(when (= serial desired-serial)
+			  (throw 'regexp regexp)))
+		      hi-lock-string-serialize-hash))))
+	       ;; With font-locking on, check if the cursor is on an
+	       ;; highlighted text.  Checking for hi-lock face is a
+	       ;; good heuristic.
+	       (and (string-match "\\`hi-lock-" (face-name (face-at-point)))
+		    (let* ((hi-text
+			    (buffer-substring-no-properties
+			     (previous-single-property-change (point) 'face)
+			     (next-single-property-change (point) 'face))))
+		      ;; Compute hi-lock patterns that match the
+		      ;; highlighted text at point.  Use this later in
+		      ;; during completing-read.
+		      (setq candidate-hi-lock-patterns
+			    (delq nil
+				  (mapcar
+				   (lambda (hi-lock-pattern)
+				     (let ((regexp (car hi-lock-pattern)))
+				       (and (string-match regexp hi-text)
+					    hi-lock-pattern)))
+				   hi-lock-interactive-patterns)))
+		      ;; Use regexp from the first matching pattern as
+		      ;; a reasonable default.
+		      (caar candidate-hi-lock-patterns))))))
        (list
-        (completing-read "Regexp to unhighlight: "
-                         hi-lock-interactive-patterns nil t
-                         (car (car hi-lock-interactive-patterns))
-                         (cons 'history-list 1))))))
-  (let ((keyword (assoc regexp hi-lock-interactive-patterns)))
-    (when keyword
-      (font-lock-remove-keywords nil (list keyword))
-      (setq hi-lock-interactive-patterns
-            (delq keyword hi-lock-interactive-patterns))
-      (remove-overlays
-       nil nil 'hi-lock-overlay-regexp (hi-lock-string-serialize regexp))
-      (when font-lock-fontified (font-lock-fontify-buffer)))))
+	(and (not current-prefix-arg)
+	     (completing-read "Regexp to unhighlight: "
+			      (or candidate-hi-lock-patterns
+				  hi-lock-interactive-patterns)
+			      nil t default-regexp))
+	current-prefix-arg))))
+  (dolist (re (if (not prefix-arg) (list regexp)
+		(mapcar #'car hi-lock-interactive-patterns)))
+    (let ((keyword (assoc re hi-lock-interactive-patterns)))
+      (when keyword
+	(font-lock-remove-keywords nil (list keyword))
+	(setq hi-lock-interactive-patterns
+	      (delq keyword hi-lock-interactive-patterns))
+	(remove-overlays
+	 nil nil 'hi-lock-overlay-regexp (hi-lock-string-serialize re))
+	(when font-lock-fontified (font-lock-fontify-buffer))))))
 
 ;;;###autoload
 (defun hi-lock-write-interactive-patterns ()
@@ -567,20 +640,28 @@
     regexp))
 
 (defun hi-lock-read-face-name ()
-  "Read face name from minibuffer with completion and history."
-  (intern (completing-read
-           "Highlight using face: "
-           obarray 'facep t
-           (cons (car hi-lock-face-defaults)
-                 (let ((prefix
-                        (try-completion
-                         (substring (car hi-lock-face-defaults) 0 1)
-                         hi-lock-face-defaults)))
-                   (if (and (stringp prefix)
-                            (not (equal prefix (car hi-lock-face-defaults))))
-                       (length prefix) 0)))
-           'face-name-history
-	   (cdr hi-lock-face-defaults))))
+  "Return face name for interactive highlighting.
+When `hi-lock-auto-select-face' is non-nil, return head of
+`hi-lock-auto-select-face-defaults'.  Otherwise, read face name
+from minibuffer with completion and history."
+  (if hi-lock-auto-select-face
+      ;; Return current head and rotate the face list.
+      (prog1 (intern (car hi-lock-auto-select-face-defaults))
+	(setq hi-lock-auto-select-face-defaults
+	      (cdr hi-lock-auto-select-face-defaults)))
+    (intern (completing-read
+	     "Highlight using face: "
+	     obarray 'facep t
+	     (cons (car hi-lock-face-defaults)
+		   (let ((prefix
+			  (try-completion
+			   (substring (car hi-lock-face-defaults) 0 1)
+			   hi-lock-face-defaults)))
+		     (if (and (stringp prefix)
+			      (not (equal prefix (car hi-lock-face-defaults))))
+			 (length prefix) 0)))
+	     'face-name-history
+	     (cdr hi-lock-face-defaults)))))
 
 (defun hi-lock-set-pattern (regexp face)
   "Highlight REGEXP with face FACE."

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2012-10-04 19:28:11 +0000
+++ lisp/replace.el	2012-10-12 19:15:47 +0000
@@ -585,27 +585,32 @@
 When PROMPT doesn't end with a colon and space, it adds a final \": \".
 If DEFAULTS is non-nil, it displays the first default in the prompt.
 
-Non-nil optional arg DEFAULTS is a string or a list of strings that
-are prepended to a list of standard default values, which include the
-string at point, the last isearch regexp, the last isearch string, and
-the last replacement regexp.
+Optional arg DEFAULTS is a string or a list of strings that are
+prepended to a list of standard default values, which include the
+tag at point, the last isearch regexp, the last isearch string,
+and the last replacement regexp.
 
 Non-nil HISTORY is a symbol to use for the history list.
 If HISTORY is nil, `regexp-history' is used."
-  (let* ((default (if (consp defaults) (car defaults) defaults))
-	 (defaults
+  (let* ((defaults
 	   (append
 	    (if (listp defaults) defaults (list defaults))
-	    (list (regexp-quote
-		   (or (funcall (or find-tag-default-function
-				    (get major-mode 'find-tag-default-function)
-				    'find-tag-default))
-		       ""))
-		  (car regexp-search-ring)
-		  (regexp-quote (or (car search-ring) ""))
-		  (car (symbol-value
-			query-replace-from-history-variable)))))
+	    (list
+	     ;; Regexp for tag at point.
+	     (let* ((tagf (or find-tag-default-function
+			      (get major-mode 'find-tag-default-function)
+			      'find-tag-default))
+		    (tag (funcall tagf)))
+	       (cond ((not tag) "")
+		     ((eq tagf 'find-tag-default)
+		      (format "\\_<%s\\_>" (regexp-quote tag)))
+		     (t (regexp-quote tag))))
+	     (car regexp-search-ring)
+	     (regexp-quote (or (car search-ring) ""))
+	     (car (symbol-value
+		   query-replace-from-history-variable)))))
 	 (defaults (delete-dups (delq nil (delete "" defaults))))
+	 (default (car defaults))
 	 ;; Do not automatically add default to the history for empty input.
 	 (history-add-new-input nil)
 	 (input (read-from-minibuffer


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: bug11095-r110501.diff --]
[-- Type: text/x-diff, Size: 4905 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-10 14:45:26 +0000
+++ lisp/ChangeLog	2012-10-10 20:23:49 +0000
@@ -1,3 +1,14 @@
+2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-yellow, hi-pink, hi-green, hi-blue, hi-black-b)
+        (hi-blue-b, hi-green-b, hi-red-b, hi-black-hb): Mark these faces
+	as obsolete.
+        (hi-lock-1, hi-lock-2, hi-lock-3, hi-lock-4, hi-lock-5, hi-lock-6)
+	(hi-lock-7, hi-lock-8, hi-lock-9): New faces.
+	(hi-lock-face-defaults, hi-lock-line-face-buffer)
+	(hi-lock-face-buffer, hi-lock-face-phrase-buffer): Propagate above
+	changes.
+
 2012-10-10  Kenichi Handa  <handa@gnu.org>
 
 	* select.el (xselect--encode-string): If a coding is specified for

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-07 00:27:31 +0000
+++ lisp/hi-lock.el	2012-10-10 20:23:49 +0000
@@ -140,7 +140,7 @@
   :group 'hi-lock
   :group 'faces)
 
-(defface hi-yellow
+(defface hi-lock-1
   '((((min-colors 88) (background dark))
      (:background "yellow1" :foreground "black"))
     (((background dark)) (:background "yellow" :foreground "black"))
@@ -149,13 +149,13 @@
   "Default face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-pink
+(defface hi-lock-2
   '((((background dark)) (:background "pink" :foreground "black"))
     (t (:background "pink")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-green
+(defface hi-lock-3
   '((((min-colors 88) (background dark))
      (:background "green1" :foreground "black"))
     (((background dark)) (:background "green" :foreground "black"))
@@ -164,40 +164,50 @@
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-blue
+(defface hi-lock-4
   '((((background dark)) (:background "light blue" :foreground "black"))
     (t (:background "light blue")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-black-b
+(defface hi-lock-5
   '((t (:weight bold)))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-blue-b
+(defface hi-lock-6
   '((((min-colors 88)) (:weight bold :foreground "blue1"))
     (t (:weight bold :foreground "blue")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-green-b
+(defface hi-lock-7
   '((((min-colors 88)) (:weight bold :foreground "green1"))
     (t (:weight bold :foreground "green")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-red-b
+(defface hi-lock-8
   '((((min-colors 88)) (:weight bold :foreground "red1"))
     (t (:weight bold :foreground "red")))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
-(defface hi-black-hb
+(defface hi-lock-9
   '((t (:weight bold :height 1.67 :inherit variable-pitch)))
   "Face for hi-lock mode."
   :group 'hi-lock-faces)
 
+(define-obsolete-face-alias 'hi-yellow 'hi-lock-1 "24.3")
+(define-obsolete-face-alias 'hi-pink 'hi-lock-2 "24.3")
+(define-obsolete-face-alias 'hi-green 'hi-lock-3 "24.3")
+(define-obsolete-face-alias 'hi-blue 'hi-lock-4 "24.3")
+(define-obsolete-face-alias 'hi-black-b 'hi-lock-5 "24.3")
+(define-obsolete-face-alias 'hi-blue-b 'hi-lock-6 "24.3")
+(define-obsolete-face-alias 'hi-green-b 'hi-lock-7 "24.3")
+(define-obsolete-face-alias 'hi-red-b 'hi-lock-8 "24.3")
+(define-obsolete-face-alias 'hi-black-hb 'hi-lock-9 "24.3")
+
 (defvar hi-lock-file-patterns nil
   "Patterns found in file for hi-lock.  Should not be changed.")
 
@@ -207,8 +217,8 @@
 (define-obsolete-variable-alias 'hi-lock-face-history
                                 'hi-lock-face-defaults "23.1")
 (defvar hi-lock-face-defaults
-  '("hi-yellow" "hi-pink" "hi-green" "hi-blue" "hi-black-b"
-    "hi-blue-b" "hi-red-b" "hi-green-b" "hi-black-hb")
+  '("hi-lock-1" "hi-lock-2" "hi-lock-3" "hi-lock-4" "hi-lock-5"
+    "hi-lock-6" "hi-lock-7" "hi-lock-8" "hi-lock-9")
   "Default faces for hi-lock interactive functions.")
 
 ;;(dolist (f hi-lock-face-defaults)
@@ -410,7 +420,7 @@
     (hi-lock-regexp-okay
      (read-regexp "Regexp to highlight line" (car regexp-history)))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern
    ;; The \\(?:...\\) grouping construct ensures that a leading ^, +, * or ?
@@ -435,7 +445,7 @@
     (hi-lock-regexp-okay
      (read-regexp "Regexp to highlight" (car regexp-history)))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 
@@ -457,7 +467,7 @@
      (hi-lock-process-phrase
       (read-regexp "Phrase to highlight" (car regexp-history))))
     (hi-lock-read-face-name)))
-  (or (facep face) (setq face 'hi-yellow))
+  (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
   (hi-lock-set-pattern regexp face))
 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: bug11095-r110502.diff --]
[-- Type: text/x-diff, Size: 3602 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-10 20:23:49 +0000
+++ lisp/ChangeLog	2012-10-10 22:01:18 +0000
@@ -1,5 +1,11 @@
 2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
 
+	* hi-lock.el (hi-lock-auto-select-face): New user variable.
+	(hi-lock-auto-select-face-defaults): New buffer local variable.
+	(hi-lock-read-face-name): Honor `hi-lock-auto-select-face'.
+
+2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
+
 	* hi-lock.el (hi-yellow, hi-pink, hi-green, hi-blue, hi-black-b)
         (hi-blue-b, hi-green-b, hi-red-b, hi-black-hb): Mark these faces
 	as obsolete.

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-10 20:23:49 +0000
+++ lisp/hi-lock.el	2012-10-10 22:01:18 +0000
@@ -135,6 +135,14 @@
 ;; It can have a function value.
 (put 'hi-lock-file-patterns-policy 'risky-local-variable t)
 
+(defcustom hi-lock-auto-select-face nil
+  "Non-nil if highlighting commands should not prompt for face names.
+When non-nil, each hi-lock command will cycle through faces in
+`hi-lock-face-defaults'."
+  :type 'boolean
+  :group 'hi-lock
+  :version "24.3")
+
 (defgroup hi-lock-faces nil
   "Faces for hi-lock."
   :group 'hi-lock
@@ -221,8 +229,15 @@
     "hi-lock-6" "hi-lock-7" "hi-lock-8" "hi-lock-9")
   "Default faces for hi-lock interactive functions.")
 
-;;(dolist (f hi-lock-face-defaults)
-;;  (unless (facep f) (error "%s not a face" f)))
+(defvar hi-lock-auto-select-face-defaults
+  (let ((l (copy-sequence hi-lock-face-defaults)))
+    (setcdr (last l) l))
+  "Circular list of faces used for interactive highlighting.
+When `hi-lock-auto-select-face' is non-nil, use the face at the
+head of this list for next interactive highlighting.  See also
+`hi-lock-read-face-name'.")
+
+(make-variable-buffer-local 'hi-lock-auto-select-face-defaults)
 
 (define-obsolete-variable-alias 'hi-lock-regexp-history
                                 'regexp-history
@@ -577,20 +592,28 @@
     regexp))
 
 (defun hi-lock-read-face-name ()
-  "Read face name from minibuffer with completion and history."
-  (intern (completing-read
-           "Highlight using face: "
-           obarray 'facep t
-           (cons (car hi-lock-face-defaults)
-                 (let ((prefix
-                        (try-completion
-                         (substring (car hi-lock-face-defaults) 0 1)
-                         hi-lock-face-defaults)))
-                   (if (and (stringp prefix)
-                            (not (equal prefix (car hi-lock-face-defaults))))
-                       (length prefix) 0)))
-           'face-name-history
-	   (cdr hi-lock-face-defaults))))
+  "Return face name for interactive highlighting.
+When `hi-lock-auto-select-face' is non-nil, return head of
+`hi-lock-auto-select-face-defaults'.  Otherwise, read face name
+from minibuffer with completion and history."
+  (if hi-lock-auto-select-face
+      ;; Return current head and rotate the face list.
+      (prog1 (car hi-lock-auto-select-face-defaults)
+	(setq hi-lock-auto-select-face-defaults
+	      (cdr hi-lock-auto-select-face-defaults)))
+    (intern (completing-read
+	     "Highlight using face: "
+	     obarray 'facep t
+	     (cons (car hi-lock-face-defaults)
+		   (let ((prefix
+			  (try-completion
+			   (substring (car hi-lock-face-defaults) 0 1)
+			   hi-lock-face-defaults)))
+		     (if (and (stringp prefix)
+			      (not (equal prefix (car hi-lock-face-defaults))))
+			 (length prefix) 0)))
+	     'face-name-history
+	     (cdr hi-lock-face-defaults)))))
 
 (defun hi-lock-set-pattern (regexp face)
   "Highlight REGEXP with face FACE."


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: bug11095-r110503.diff --]
[-- Type: text/x-diff, Size: 4307 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-10 22:01:18 +0000
+++ lisp/ChangeLog	2012-10-11 20:39:05 +0000
@@ -1,3 +1,15 @@
+2012-10-11  Jambunathan K  <kjambunathan@gmail.com>
+
+	* replace.el (read-regexp): Tighten the regexp that matches tag.
+	When tag is retrieved with `find-tag-default', use regexp that
+	matches symbol at point.  Also update docstring.
+	* hi-lock.el (hi-lock-line-face-buffer, hi-lock-face-buffer)
+	(hi-lock-face-phrase-buffer): Don't provide a default to
+	`read-regexp'.  Effectively the default regexp matches tag at
+	point.
+	(hi-lock-read-face-name): Return face as a symbol.  Fix earlier
+	commit.
+
 2012-10-10  Jambunathan K  <kjambunathan@gmail.com>
 
 	* hi-lock.el (hi-lock-auto-select-face): New user variable.

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-10 22:01:18 +0000
+++ lisp/hi-lock.el	2012-10-11 20:39:05 +0000
@@ -433,7 +433,7 @@
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight line" (car regexp-history)))
+     (read-regexp "Regexp to highlight line"))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -458,7 +458,7 @@
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight" (car regexp-history)))
+     (read-regexp "Regexp to highlight"))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -480,7 +480,7 @@
    (list
     (hi-lock-regexp-okay
      (hi-lock-process-phrase
-      (read-regexp "Phrase to highlight" (car regexp-history))))
+      (read-regexp "Phrase to highlight")))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-lock-1))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -598,7 +598,7 @@
 from minibuffer with completion and history."
   (if hi-lock-auto-select-face
       ;; Return current head and rotate the face list.
-      (prog1 (car hi-lock-auto-select-face-defaults)
+      (prog1 (intern (car hi-lock-auto-select-face-defaults))
 	(setq hi-lock-auto-select-face-defaults
 	      (cdr hi-lock-auto-select-face-defaults)))
     (intern (completing-read

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2012-10-04 19:28:11 +0000
+++ lisp/replace.el	2012-10-11 20:39:05 +0000
@@ -585,27 +585,32 @@
 When PROMPT doesn't end with a colon and space, it adds a final \": \".
 If DEFAULTS is non-nil, it displays the first default in the prompt.
 
-Non-nil optional arg DEFAULTS is a string or a list of strings that
-are prepended to a list of standard default values, which include the
-string at point, the last isearch regexp, the last isearch string, and
-the last replacement regexp.
+Optional arg DEFAULTS is a string or a list of strings that are
+prepended to a list of standard default values, which include the
+tag at point, the last isearch regexp, the last isearch string,
+and the last replacement regexp.
 
 Non-nil HISTORY is a symbol to use for the history list.
 If HISTORY is nil, `regexp-history' is used."
-  (let* ((default (if (consp defaults) (car defaults) defaults))
-	 (defaults
+  (let* ((defaults
 	   (append
 	    (if (listp defaults) defaults (list defaults))
-	    (list (regexp-quote
-		   (or (funcall (or find-tag-default-function
-				    (get major-mode 'find-tag-default-function)
-				    'find-tag-default))
-		       ""))
-		  (car regexp-search-ring)
-		  (regexp-quote (or (car search-ring) ""))
-		  (car (symbol-value
-			query-replace-from-history-variable)))))
+	    (list
+	     ;; Regexp for tag at point.
+	     (let* ((tagf (or find-tag-default-function
+			      (get major-mode 'find-tag-default-function)
+			      'find-tag-default))
+		    (tag (funcall tagf)))
+	       (cond ((not tag) "")
+		     ((eq tagf 'find-tag-default)
+		      (format "\\_<%s\\_>" (regexp-quote tag)))
+		     (t (regexp-quote tag))))
+	     (car regexp-search-ring)
+	     (regexp-quote (or (car search-ring) ""))
+	     (car (symbol-value
+		   query-replace-from-history-variable)))))
 	 (defaults (delete-dups (delq nil (delete "" defaults))))
+	 (default (car defaults))
 	 ;; Do not automatically add default to the history for empty input.
 	 (history-add-new-input nil)
 	 (input (read-from-minibuffer


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: bug11095-r110504.diff --]
[-- Type: text/x-diff, Size: 3964 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-11 20:39:05 +0000
+++ lisp/ChangeLog	2012-10-12 16:19:16 +0000
@@ -1,3 +1,10 @@
+2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
+
+	* hi-lock.el (hi-lock-unface-buffer): Prompt user with useful
+	defaults.  Specifically, if cursor is on an highlighted text,
+	offer a regexp that matches that text as the default.  Update
+	docstring.
+
 2012-10-11  Jambunathan K  <kjambunathan@gmail.com>
 
 	* replace.el (read-regexp): Tighten the regexp that matches tag.

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-11 20:39:05 +0000
+++ lisp/hi-lock.el	2012-10-12 16:19:16 +0000
@@ -493,8 +493,15 @@
 ;;;###autoload
 (defun hi-lock-unface-buffer (regexp)
   "Remove highlighting of each match to REGEXP set by hi-lock.
-Interactively, prompt for REGEXP, accepting only regexps
-previously inserted by hi-lock interactive functions."
+Interactively, prompt for REGEXP.  If the cursor is on a
+previously highlighted text and if the associated regexp can be
+inferred via simple heuristics, offer that regexp as default.
+Otherwise, prompt for REGEXP with completion and limit the
+choices to only those regexps used previously with hi-lock
+commands.
+
+If this command is invoked via menu, make no attempt to infer
+REGEXP from text at point."
   (interactive
    (if (and (display-popup-menus-p)
 	    (listp last-nonmenu-event)
@@ -522,15 +529,51 @@
 	  ;; To prevent that, we return an empty string, which will
 	  ;; effectively disable the rest of the function.
 	  (throw 'snafu '(""))))
-     (let ((history-list (mapcar (lambda (p) (car p))
-                                 hi-lock-interactive-patterns)))
-       (unless hi-lock-interactive-patterns
-         (error "No highlighting to remove"))
+     ;; Un-highlighting triggered via keyboard action.
+     (unless hi-lock-interactive-patterns
+       (error "No highlighting to remove"))
+     ;; Infer the regexp to un-highlight based on cursor position.
+     (let* (candidate-hi-lock-patterns
+	    (default-regexp
+	      (or
+	       ;; When using overlays, there is no ambiguity on the best
+	       ;; choice of regexp.
+	       (let ((desired-serial (get-char-property
+				      (point) 'hi-lock-overlay-regexp)))
+		 (when desired-serial
+		   (catch 'regexp
+		     (maphash
+		      (lambda (regexp serial)
+			(when (= serial desired-serial)
+			  (throw 'regexp regexp)))
+		      hi-lock-string-serialize-hash))))
+	       ;; With font-locking on, check if the cursor is on an
+	       ;; highlighted text.  Checking for hi-lock face is a
+	       ;; good heuristic.
+	       (and (string-match "\\`hi-lock-" (face-name (face-at-point)))
+		    (let* ((hi-text
+			    (buffer-substring-no-properties
+			     (previous-single-property-change (point) 'face)
+			     (next-single-property-change (point) 'face))))
+		      ;; Compute hi-lock patterns that match the
+		      ;; highlighted text at point.  Use this later in
+		      ;; during completing-read.
+		      (setq candidate-hi-lock-patterns
+			    (delq nil
+				  (mapcar
+				   (lambda (hi-lock-pattern)
+				     (let ((regexp (car hi-lock-pattern)))
+				       (and (string-match regexp hi-text)
+					    hi-lock-pattern)))
+				   hi-lock-interactive-patterns)))
+		      ;; Use regexp from the first matching pattern as
+		      ;; a reasonable default.
+		      (caar candidate-hi-lock-patterns))))))
        (list
-        (completing-read "Regexp to unhighlight: "
-                         hi-lock-interactive-patterns nil t
-                         (car (car hi-lock-interactive-patterns))
-                         (cons 'history-list 1))))))
+	(completing-read "Regexp to unhighlight: "
+			 (or candidate-hi-lock-patterns
+			     hi-lock-interactive-patterns)
+			 nil t default-regexp)))))
   (let ((keyword (assoc regexp hi-lock-interactive-patterns)))
     (when keyword
       (font-lock-remove-keywords nil (list keyword))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: bug11095-r110505.diff --]
[-- Type: text/x-diff, Size: 3378 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-10-12 16:19:16 +0000
+++ lisp/ChangeLog	2012-10-12 18:05:27 +0000
@@ -1,5 +1,10 @@
 2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
 
+	* hi-lock.el (hi-lock-unface-buffer): With prefix arg, unhighlight
+	all hi-lock patterns in buffer.
+
+2012-10-12  Jambunathan K  <kjambunathan@gmail.com>
+
 	* hi-lock.el (hi-lock-unface-buffer): Prompt user with useful
 	defaults.  Specifically, if cursor is on an highlighted text,
 	offer a regexp that matches that text as the default.  Update

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2012-10-12 16:19:16 +0000
+++ lisp/hi-lock.el	2012-10-12 18:05:27 +0000
@@ -491,17 +491,18 @@
 ;;;###autoload
 (defalias 'unhighlight-regexp 'hi-lock-unface-buffer)
 ;;;###autoload
-(defun hi-lock-unface-buffer (regexp)
+(defun hi-lock-unface-buffer (regexp &optional prefix-arg)
   "Remove highlighting of each match to REGEXP set by hi-lock.
-Interactively, prompt for REGEXP.  If the cursor is on a
-previously highlighted text and if the associated regexp can be
-inferred via simple heuristics, offer that regexp as default.
-Otherwise, prompt for REGEXP with completion and limit the
-choices to only those regexps used previously with hi-lock
-commands.
+Interactively, when PREFIX-ARG is non-nil, unhighlight all
+highlighted text in current buffer.  When PREFIX-ARG is nil,
+prompt for REGEXP.  If the cursor is on a previously highlighted
+text and if the associated regexp can be inferred via simple
+heuristics, offer that regexp as default.  Otherwise, prompt for
+REGEXP with completion and limit the choices to only those
+regexps used previously with hi-lock commands.
 
-If this command is invoked via menu, make no attempt to infer
-REGEXP from text at point."
+If this command is invoked via menu, pop-up a list of currently
+highlighted patterns."
   (interactive
    (if (and (display-popup-menus-p)
 	    (listp last-nonmenu-event)
@@ -570,18 +571,22 @@
 		      ;; a reasonable default.
 		      (caar candidate-hi-lock-patterns))))))
        (list
-	(completing-read "Regexp to unhighlight: "
-			 (or candidate-hi-lock-patterns
-			     hi-lock-interactive-patterns)
-			 nil t default-regexp)))))
-  (let ((keyword (assoc regexp hi-lock-interactive-patterns)))
-    (when keyword
-      (font-lock-remove-keywords nil (list keyword))
-      (setq hi-lock-interactive-patterns
-            (delq keyword hi-lock-interactive-patterns))
-      (remove-overlays
-       nil nil 'hi-lock-overlay-regexp (hi-lock-string-serialize regexp))
-      (when font-lock-fontified (font-lock-fontify-buffer)))))
+	(and (not current-prefix-arg)
+	     (completing-read "Regexp to unhighlight: "
+			      (or candidate-hi-lock-patterns
+				  hi-lock-interactive-patterns)
+			      nil t default-regexp))
+	current-prefix-arg))))
+  (dolist (re (if (not prefix-arg) (list regexp)
+		(mapcar #'car hi-lock-interactive-patterns)))
+    (let ((keyword (assoc re hi-lock-interactive-patterns)))
+      (when keyword
+	(font-lock-remove-keywords nil (list keyword))
+	(setq hi-lock-interactive-patterns
+	      (delq keyword hi-lock-interactive-patterns))
+	(remove-overlays
+	 nil nil 'hi-lock-overlay-regexp (hi-lock-string-serialize re))
+	(when font-lock-fontified (font-lock-fontify-buffer))))))
 
 ;;;###autoload
 (defun hi-lock-write-interactive-patterns ()


[-- Attachment #8: Type: text/plain, Size: 2715 bytes --]


> Proposal is in two parts.  Part-I deals with `hi-lock-face-buffer'.
> Part-II deals with `unhighlight-regexp'.  Part-III has a dump of the
> current customization I have in my .emacs.
>
> I believe that my proposal is useful in general.  So I request that it
> be folded in to Emacs-24.1.
>
> Part-I: `hi-lock-face-buffer' & Co.
> ----------------------------------
>
> 1) Review the face names used in `hi-lock-face-defaults' and make the
>    faces customizable.  The defaults may not look good on a user's his
>    own font-lock configuration.
>
> 2) Make `hi-lock-face-buffer' use a different face on each invocation.  
>
>    Here is a real-world usecase for the above request.
>
>    As a programmer, I use highlighting to trace variable dependencies
>    within a function.  For example, in the example below, after
>    highlighting the variables in __different__ faces, I will come to the
>    conclusion that "a" depends on "d" and "tmp".
>
>      c = d;
>      b = c + tmp;
>      a = b;
>
>    And I use this very often to track variables and how they get their
>    values from.
>
>    If I were to use the default Emacs provided behaviour then I would
>    have to press M-n multiple times as I highlight more and more
>    symbols. (Typically I have 3-5 symbols highlighted before I turn off
>    highlighting.)
>
> See elisp snippet at the end of the mail.
>
>
> Part-II: `unhighlight-regexp'
> ------------------------------
>
> See usecase in Part-I/Item-2 
>
> 1) I want to selectively turn-off highlighting for certain regexps
>    (arguably) that _specific_ highlighted regexp cursor is stationed on.
>    This would happen when I decide that I don't want to factor a
>    particular variable for my current refactoring exercise.
>
>    I find the current behaviour of `unhighlight-regexp' slightly
>    tedious.  
>
>    1. There is no completion for which regexp I want to unhighlight and
>       I have to circle through `hi-lock-interactive-patterns'.
>
>    2. Browsing the `hi-lock-interactive-patterns' is tedious for the
>       following reasons:
>
>       - The order in which unhighlighting happens could totally be
>         unrelated to the order in which highlighting happens.  When I am
>         analyzing the variable flow, I don't want to do a "context
>         switch" trying to find out what item to choose from the
>         `unhighlight-regexp' menu.
>
> 2) I want to unhighlight all regexps.  This happens when I am done with
>    variable analysis.
>
>
> ps: I am not questioning the current defaults.  I am only saying that it
> the current behaviour is too generic to be immediately useful (atleast
> for my usecase) and so needs some sort of extra augmentation.