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

[Jambunathan K]

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

[Jambunathan K]

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)

[Jambunathan K]

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.

[Jambunathan K]

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

[Juri Linkov]

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

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

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

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

what is currently missing is this command:

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

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

[Jambunathan K]

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.

[Jambunathan K]

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.

[Jambunathan K]

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.

[Jambunathan K]

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

[Juri Linkov]

> Have you tried the patches?

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

[Jambunathan K]

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

[Stefan Monnier]

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

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

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

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

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

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

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

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


Stefan

[Drew Adams]

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

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

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

[Stefan Monnier]

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

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

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

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

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


Stefan

[Drew Adams]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[Stefan Monnier]

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

Just one option `hi-lock-faces'.

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

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


Stefan

[Jambunathan K]

Stefan Monnier <mon...@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.

[Stefan Monnier]

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

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

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

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


Stefan

[Jambunathan K]

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.

[Stefan Monnier]

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

I just fixed that a little while ago.

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

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

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

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

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


Stefan

[Drew Adams]

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

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

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

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

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

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

[Jambunathan K]

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

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

My patch has hints on where the brokenness comes from.

[Stefan Monnier]

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

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


Stefan

[Stefan Monnier]

> 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

[Jambunathan K]

Stefan Monnier <mon...@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
>
>
>
>

--

[Stefan Monnier]

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

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

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

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

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

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

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


Stefan

[Jambunathan K]

Attaching a patch. See ChangeLog for details.

[Jambunathan K]

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.

[Stefan Monnier]

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

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


Stefan

[Jambunathan K]

Stefan Monnier <mon...@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.

[Stefan Monnier]

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

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

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

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

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

Why?


Stefan


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

(defvar-local hi-lock--unused-faces nil