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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 11095 in the body.
You can then email your comments to 11095 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.0.94; hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Mon, 26 Mar 2012 12:16:55 +0530

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)))

Message #8 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 11 Oct 2012 01:51:08 +0530

[Message part 1 (text/plain, inline)]

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).

[bug11095-part1.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

> 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)))

Message #11 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 11 Oct 2012 03:38:51 +0530

[Message part 1 (text/plain, inline)]

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)

[bug11095-part2.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

> 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)))
>
>
>
>

-- 

Message #14 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 12 Oct 2012 01:54:30 +0530

[Message part 1 (text/plain, inline)]

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.

[bug11095-part3.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

> 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)))
>
>
>
>

-- 

Message #17 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 12 Oct 2012 02:03:11 +0530

[Message part 1 (text/plain, inline)]

Jambunathan K <kjambunathan <at> 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)

[bug11095-part3-changelog.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

-- 

Message #20 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 12 Oct 2012 01:41:04 +0300

> 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.

Message #23 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 12 Oct 2012 10:00:07 +0530

Juri Linkov <juri <at> 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.

Message #26 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 12 Oct 2012 21:47:47 +0530

[Message part 1 (text/plain, inline)]

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.

[bug11095-part4.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

> 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)))
>
>
>
>

-- 

Message #29 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 12 Oct 2012 23:48:55 +0530

[Message part 1 (text/plain, inline)]

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.

[bug11095-part5.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

> 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)))
>
>
>
>

-- 

Message #32 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: 11095 <at> debbugs.gnu.org
Subject: [FINAL] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Sat, 13 Oct 2012 01:02:24 +0530

[Message part 1 (text/plain, inline)]

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

[bug11095-final-r110525.diff (text/x-diff, attachment)]

[bug11095-r110501.diff (text/x-diff, attachment)]

[bug11095-r110502.diff (text/x-diff, attachment)]

[bug11095-r110503.diff (text/x-diff, attachment)]

[bug11095-r110504.diff (text/x-diff, attachment)]

[bug11095-r110505.diff (text/x-diff, attachment)]

[Message part 8 (text/plain, inline)]

> 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.

Message #35 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> jurta.org>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Sat, 13 Oct 2012 19:10:00 +0300

> 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'.

Message #38 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Sat, 13 Oct 2012 22:58:58 +0530

Juri Linkov <juri <at> 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.

Message #43 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Tue, 04 Dec 2012 16:14:41 -0500

> -(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

Message #46 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>,
	"'Jambunathan K'" <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: RE: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Tue, 4 Dec 2012 13:39:21 -0800

> > -(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.

Message #49 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 11095 <at> debbugs.gnu.org, 'Jambunathan K' <kjambunathan <at> gmail.com>
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Tue, 04 Dec 2012 16:57:04 -0500

>> > -(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

Message #52 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org, 'Jambunathan K' <kjambunathan <at> gmail.com>
Subject: RE: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Tue, 4 Dec 2012 14:43:53 -0800

> >> > -(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?

Message #55 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Drew Adams" <drew.adams <at> oracle.com>
Cc: 11095 <at> debbugs.gnu.org, 'Jambunathan K' <kjambunathan <at> gmail.com>
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Tue, 04 Dec 2012 22:46:23 -0500

>> 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

Message #58 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 06 Dec 2012 03:45:54 +0530

Stefan Monnier <monnier <at> 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.

Message #61 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Wed, 05 Dec 2012 20:14:20 -0500

>> 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

Message #64 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 06 Dec 2012 10:36:00 +0530

[Message part 1 (text/plain, inline)]

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("\203\305C\207\306 \203.	<\203.\n\203.\307\310\215\207\204!\311\312!\210\313 \314\f\204-\315\2022\316\317\f@\"\320\305\320\211\f&)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\"BB\"\206.\307\310\311\"\207" [hi-lock-interactive-patterns x-popup-menu t keymap "Select Pattern to Unhighlight" mapcar #[(pattern) "@\301\302@\303A <at> A@A@!#\304\211B@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.

[hi-defaults.png (image/png, attachment)]

[my-hi-faces.png (image/png, attachment)]

Message #67 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 06 Dec 2012 20:20:16 +0530

[Message part 1 (text/plain, inline)]

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.

[bug11095.patch (text/x-diff, inline)]

=== 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 <at> 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 <at> 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

[Message part 3 (text/plain, inline)]

> 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("\203\305C\207\306 \203.	<\203.\n\203.\307\310\215\207\204!\311\312!\210\313 \314\f\204-\315\2022\316\317\f@\"\320\305\320\211\f&)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\"BB\"\206.\307\310\311\"\207" [hi-lock-interactive-patterns x-popup-menu t keymap "Select Pattern to Unhighlight" mapcar #[(pattern) "@\301\302@\303A <at> A@A@!#\304\211B@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.

Message #70 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 06 Dec 2012 14:16:57 -0500

> (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

Message #73 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: "Drew Adams" <drew.adams <at> oracle.com>
To: "'Stefan Monnier'" <monnier <at> iro.umontreal.ca>,
	"'Jambunathan K'" <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: RE: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 6 Dec 2012 11:36:02 -0800

> >     (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.

Message #76 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 07 Dec 2012 02:56:07 +0530

>> (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.

Message #79 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 06 Dec 2012 16:36:29 -0500

>> 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

Message #82 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 07 Dec 2012 03:53:06 +0530

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> 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.

[unhighlight-1.png (image/png, attachment)]

[unhighlight-2.png (image/png, attachment)]

[Message part 4 (text/plain, inline)]

>         Stefan

Message #85 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Thu, 06 Dec 2012 23:07:39 -0500

>>>> 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

Message #88 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 07 Dec 2012 10:16:56 +0530

Stefan Monnier <monnier <at> 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
>
>
>
>

-- 

Message #91 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Fri, 07 Dec 2012 11:55:54 -0500

> 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

Message #94 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Sat, 08 Dec 2012 18:20:03 +0530

[Message part 1 (text/plain, inline)]

Attaching a patch.  See ChangeLog for details.

[bug11095-rev111152-1.diff (text/x-diff, inline)]

=== 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 <at> 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 <at> 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

Message #97 received at 11095 <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 11095 <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Mon, 10 Dec 2012 09:56:47 +0530

[Message part 1 (text/plain, inline)]

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.

[bug11095-rev111152-2.diff (text/x-diff, inline)]

=== 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 <at> 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 <at> 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."

Message #102 received at 11095-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095-done <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Mon, 10 Dec 2012 13:34:25 -0500

> 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

Message #105 received at 11095-done <at> debbugs.gnu.org (full text, mbox):

From: Jambunathan K <kjambunathan <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 11095-done <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Tue, 11 Dec 2012 02:07:58 +0530

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> 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.

[bug11095-rev111175.patch (text/x-diff, inline)]

=== 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 <at> 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 <at> 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)))

[Message part 3 (text/plain, inline)]

> - 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

Message #108 received at 11095-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Jambunathan K <kjambunathan <at> gmail.com>
Cc: 11095-done <at> debbugs.gnu.org
Subject: Re: bug#11095: [PATCH] Re: bug#11095: 24.0.94;
	hi-lock-face-buffer/unhighlight-regexp': Augment?
Date: Mon, 10 Dec 2012 16:27:07 -0500

>  	 (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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.