bug#47012: xref copies keymap properties to minibuffer

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

0. emacs -Q
1. C-x p g expose_frame RET
2. put point e.g. on "tab_bar_window"
3. C-x p g M-n
4. click mouse on the default value

Instead of moving point to the clicked position, it visits the xref hit.
This is because the copied default value is not stripped from the
text property 'keymap'.

I noticed this problem after adding RET binding to xref--button-map,
so typing RET on the default value doesn't accept it for new search,
but uses its local keymap to visit old reference.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

Hi!

On 08.03.2021 22:03, Juri Linkov wrote:
> 0. emacs -Q
> 1. C-x p g expose_frame RET
> 2. put point e.g. on "tab_bar_window"
> 3. C-x p g M-n
> 4. click mouse on the default value
> 
> Instead of moving point to the clicked position, it visits the xref hit.
> This is because the copied default value is not stripped from the
> text property 'keymap'.
> 
> I noticed this problem after adding RET binding to xref--button-map,
> so typing RET on the default value doesn't accept it for new search,
> but uses its local keymap to visit old reference.

Thanks for the report, should now be fixed in master, commit 8538108132836.

FWIW, it's was project.el's problem, not xref's.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>> 0. emacs -Q
>> 1. C-x p g expose_frame RET
>> 2. put point e.g. on "tab_bar_window"
>> 3. C-x p g M-n
>> 4. click mouse on the default value
>> Instead of moving point to the clicked position, it visits the xref hit.
>> This is because the copied default value is not stripped from the
>> text property 'keymap'.
>> I noticed this problem after adding RET binding to xref--button-map,
>> so typing RET on the default value doesn't accept it for new search,
>> but uses its local keymap to visit old reference.
>
> Thanks for the report, should now be fixed in master, commit 8538108132836.

Confirmed, thanks.

> FWIW, it's was project.el's problem, not xref's.

I thought it's the xref's problem because Help says it's in xref.el.
Typing 'C-h f project--read-regexp RET' shows:

  project--read-regexp is a Lisp function in ‘xref.el’.

But this is because I tried to override this function in the init file:

;; Redefine to use `minibuffer-history':
(with-eval-after-load 'project
  (defun project--read-regexp ()
    (let ((sym (thing-at-point 'symbol t)))
      (read-regexp "Find regexp"
                   (and sym (regexp-quote sym))
                   'minibuffer-history))))

Oh, well, need to find another way to do this
(no feature request for this :-)

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

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

>> FWIW, it's was project.el's problem, not xref's.
>
> I thought it's the xref's problem because Help says it's in xref.el.
> Typing 'C-h f project--read-regexp RET' shows:
>
>   project--read-regexp is a Lisp function in ‘xref.el’.
>
> But this is because I tried to override this function in the init file:
>
> ;; Redefine to use `minibuffer-history':
> (with-eval-after-load 'project
>   (defun project--read-regexp ()
>     (let ((sym (thing-at-point 'symbol t)))
>       (read-regexp "Find regexp"
>                    (and sym (regexp-quote sym))
>                    'minibuffer-history))))
>
> Oh, well, need to find another way to do this
> (no feature request for this :-)

Actually, can't do this without feature request,
and can't use it as is, because never use regexp
while searching in project, whereas it pollutes
the regexp history for commands where I really
use regexps, not just strings.

A simple patch could be a saver to use with e.g.
(setq project-regexp-history-variable 'minibuffer-history):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-regexp-history-variable.patch --]
[-- Type: text/x-diff, Size: 630 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index b6a886f731..280a8d8929 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -781,9 +781,12 @@ project--find-regexp-in-files
       (user-error "No matches for: %s" regexp))
     xrefs))
 
+(defvar project-regexp-history-variable nil)
+
 (defun project--read-regexp ()
   (let ((sym (thing-at-point 'symbol t)))
-    (read-regexp "Find regexp" (and sym (regexp-quote sym)))))
+    (read-regexp "Find regexp" (and sym (regexp-quote sym))
+                 project-regexp-history-variable)))
 
 ;;;###autoload
 (defun project-find-file ()

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 24.03.2021 22:38, Juri Linkov wrote:
> Actually, can't do this without feature request,
> and can't use it as is, because never use regexp
> while searching in project, whereas it pollutes
> the regexp history for commands where I really
> use regexps, not just strings.
> 
> A simple patch could be a saver to use with e.g.
> (setq project-regexp-history-variable 'minibuffer-history):

Hi!

The idea makes sense, but perhaps we shouldn't add one more way of doing 
this and just copy what grep-read-regexp does. Meaning, define a 
project-specific var and then use it. We could use 'grep-regexp-history 
there too, as another alternative.

But having a var called project-regexp-history seems more powerful, and 
even no less powerful than your suggested approach, because one can 
always turn that variable into a defalias, if they so wish.

(I often use regexps when searching in project, but this change doesn't 
seem problematic for this usage either.)

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>> Actually, can't do this without feature request,
>> and can't use it as is, because never use regexp
>> while searching in project, whereas it pollutes
>> the regexp history for commands where I really
>> use regexps, not just strings.
>> A simple patch could be a saver to use with e.g.
>> (setq project-regexp-history-variable 'minibuffer-history):
>
> The idea makes sense, but perhaps we shouldn't add one more way of doing
> this and just copy what grep-read-regexp does. Meaning, define
> a project-specific var and then use it. We could use 'grep-regexp-history
> there too, as another alternative.
>
> But having a var called project-regexp-history seems more powerful, and
> even no less powerful than your suggested approach, because one can always
> turn that variable into a defalias, if they so wish.

Sharing regexp history between grep and project-find-regexp would be
the most desirable.  How better to achieve this?

Some commands already use variable *-history-variable, so adding
project-regexp-history-variable wouldn't cause a new precedent.

Whereas using defalias is something new, maybe it could work too:

  (defalias 'grep-regexp-history 'project-regexp-history)

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 25.03.2021 11:40, Juri Linkov wrote:
> Sharing regexp history between grep and project-find-regexp would be
> the most desirable.  How better to achieve this?

Either by using 'grep-regexp-history directly, or with either of the 
options below.

> Some commands already use variable *-history-variable, so adding
> project-regexp-history-variable wouldn't cause a new precedent.

Could you give an example?

And ideally, we should be also able to answer the question why some 
packages have their own variables, and others have "-variable variables".

If it's just historical reasons, as it often is, maybe

   (defvar project-regexp-history-variable 'grep-regexp-history)

will be good enough.

> Whereas using defalias is something new, maybe it could work too:
> 
>    (defalias 'grep-regexp-history 'project-regexp-history)

Yup, that's what I meant. It does feel a bit hackish, though.

Another possible approach would be some categories based system, 
together with user customization, like completion-category-overrides, 
but it's probably overkill for this problem.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>> Sharing regexp history between grep and project-find-regexp would be
>> the most desirable.  How better to achieve this?
>
> Either by using 'grep-regexp-history directly, or with either of the
> options below.
>
>> Some commands already use variable *-history-variable, so adding
>> project-regexp-history-variable wouldn't cause a new precedent.
>
> Could you give an example?

What I tried is C-h v *-history-variable TAB

  minibuffer-history-variable
  query-replace-from-history-variable
  query-replace-to-history-variable
  y-or-n-p-history-variable

Maybe other commands use another suffix (probably not).

> And ideally, we should be also able to answer the question why some
> packages have their own variables, and others have "-variable variables".

I don't know.

> If it's just historical reasons, as it often is, maybe
>
>   (defvar project-regexp-history-variable 'grep-regexp-history)
>
> will be good enough.

This would a good thing to do.

>> Whereas using defalias is something new, maybe it could work too:
>>    (defalias 'grep-regexp-history 'project-regexp-history)
>
> Yup, that's what I meant. It does feel a bit hackish, though.
>
> Another possible approach would be some categories based system, together
> with user customization, like completion-category-overrides, but it's
> probably overkill for this problem.

I agree on both these statements.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 25.03.2021 23:28, Juri Linkov wrote:
>>> Some commands already use variable *-history-variable, so adding
>>> project-regexp-history-variable wouldn't cause a new precedent.
>> Could you give an example?
> What I tried is C-h v *-history-variable TAB
> 
>    minibuffer-history-variable
>    query-replace-from-history-variable
>    query-replace-to-history-variable
>    y-or-n-p-history-variable
> 
> Maybe other commands use another suffix (probably not).

That's what my search has shown, too: a few instances, all from pretty 
high-level features.

>> And ideally, we should be also able to answer the question why some
>> packages have their own variables, and others have "-variable variables".
> I don't know.
> 
>> If it's just historical reasons, as it often is, maybe
>>
>>    (defvar project-regexp-history-variable 'grep-regexp-history)
>>
>> will be good enough.
> This would a good thing to do.

Let's go with this one, then. At least for now.

Meaning, your patch plus a change of the default value to 
'grep-regexp-history.

Thank you.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>>>    (defvar project-regexp-history-variable 'grep-regexp-history)
>>>
>>> will be good enough.
>> This would a good thing to do.
>
> Let's go with this one, then. At least for now.
>
> Meaning, your patch plus a change of the default value to
> 'grep-regexp-history.

Now patch is pushed.

BTW, it was a big hassle to use project-find-regexp
until I realized where is the problem.  There is
no such problem in grep because in the grep output
file names are placed separately on the left,
and output lines are on the right on the same lines.
So it's easy to scan output lines visually.

But the output of project-find-regexp is a mess
because output lines are interspersed with file names
where output lines are almost indistinguishable
from file lines.  Indeed, file names are currently
highlighted in green color, but such green foreground
doesn't help to distinguish file names from output lines,
so it's very hard to read the output.

Then I realized that this problem is already solved
in diff-mode where the faces 'diff-header' and
'diff-file-header' use the grey background to separate diff hunks.

Using the same solution of 'diff-header' and
'diff-file-header' for 'xref-file-header'
improves the readability significantly:

#+begin_src diff
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index ea52befec5..f2aa8bfba4 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -513,7 +513,7 @@ xref-pop-to-location
 (defconst xref-buffer-name "*xref*"
   "The name of the buffer to show xrefs.")
 
-(defface xref-file-header '((t :inherit compilation-info))
+(defface xref-file-header '((t :background "grey90" :extend t))
   "Face used to highlight file header in the xref buffer."
   :version "27.1")
#+end_src

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 30.03.2021 22:16, Juri Linkov wrote:
> Using the same solution of 'diff-header' and
> 'diff-file-header' for 'xref-file-header'
> improves the readability significantly:
> 
> #+begin_src diff
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index ea52befec5..f2aa8bfba4 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -513,7 +513,7 @@ xref-pop-to-location
>   (defconst xref-buffer-name "*xref*"
>     "The name of the buffer to show xrefs.")
>   
> -(defface xref-file-header '((t :inherit compilation-info))
> +(defface xref-file-header '((t :background "grey90" :extend t))
>     "Face used to highlight file header in the xref buffer."
>     :version "27.1")
> #+end_src

Hi Juri,

I'm not sure I like the result. Simply applying your change to the face 
results in a color-less buffer with grey spots for both file headers and 
match highlights (at least, with my current theme).

It's less of a problem with diff-mode because its basic background is 
grey already.

I've also considered something like:

(defface xref-file-header '((t :background "grey90" :extend t :inherit 
compilation-info))
   "Face used to highlight file header in the xref buffer."
   :version "27.1")

...but it makes the file names harder to read because of the reduced 
contrast.

Then I've tried to see what others do, for instance 
https://github.com/Wilfred/deadgrep, which I'd heard people praise 
usability of.

The only distinction it adds to file names is "bold :t". And diff-mode 
also does that, actually.

So... how do you like this simple change?

(defface xref-file-header '((t :bold t :inherit compilation-info))
   "Face used to highlight file header in the xref buffer."
   :version "27.1")

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 31 Mar 2021 18:47:30 +0300
> Cc: 47012@debbugs.gnu.org
> 
> (defface xref-file-header '((t :bold t :inherit compilation-info))
>    "Face used to highlight file header in the xref buffer."
>    :version "27.1")

As we all know, faces can be customized.  If some attributes are
"magic", in that the display will be hard to read if those attributes'
values are changed, then I think we should document that in the
respective doc strings.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 31.03.2021 18:59, Eli Zaretskii wrote:
> As we all know, faces can be customized.  If some attributes are
> "magic", in that the display will be hard to read if those attributes'
> values are changed, then I think we should document that in the
> respective doc strings.

I'm not sure what you're saying we should or shouldn't do.

Juri's concern is that the default look lacks emphasis. It might be okay 
to alter it, though I'd like to avoid too radical changes.

The comment about making text harder to read that I made can apply to 
almost every face: adding darker background to a face where foreground 
is dark reduces perceived contrast, which makes text harder to real. 
Sometimes it's okay, but in general it's better to look for other 
options first rather than be forced to balance emphasis with readability.

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> Cc: 47012@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 31 Mar 2021 19:10:59 +0300
> 
> On 31.03.2021 18:59, Eli Zaretskii wrote:
> > As we all know, faces can be customized.  If some attributes are
> > "magic", in that the display will be hard to read if those attributes'
> > values are changed, then I think we should document that in the
> > respective doc strings.
> 
> I'm not sure what you're saying we should or shouldn't do.

I'm just worried by the amount of attention this one face suddenly
needs.  Why is that?  No other face I have ever met needed such
elaborate research regarding its defaults.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 31.03.2021 19:57, Eli Zaretskii wrote:
> I'm just worried by the amount of attention this one face suddenly
> needs.

You responded after we exchanged only two messages on the subject. This 
qualifies as "attention", but just barely.

> Why is that?  No other face I have ever met needed such
> elaborate research regarding its defaults.

Is that good or bad?

Not too long ago, we've discussed (and changed!) the defaults of 
diff-mode faces, with very positive results. Not sure we'll be able to 
reach the same level of agreement on this issue, but it shouldn't hurt 
to try.

help-key-binding is another face which was discussed recently, again, 
with excellent result.

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> Cc: 47012@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 1 Apr 2021 03:25:10 +0300
> 
> On 31.03.2021 19:57, Eli Zaretskii wrote:
> > I'm just worried by the amount of attention this one face suddenly
> > needs.
> 
> You responded after we exchanged only two messages on the subject. This 
> qualifies as "attention", but just barely.

It's unusual.  And the discussion isn't over yet.

> > Why is that?  No other face I have ever met needed such
> > elaborate research regarding its defaults.
> 
> Is that good or bad?

Neither, I guess.  It just surprised me that a face needs so much
attention.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>>   -(defface xref-file-header '((t :inherit compilation-info))
>> +(defface xref-file-header '((t :background "grey90" :extend t))
>>     "Face used to highlight file header in the xref buffer."
>>     :version "27.1")
>> #+end_src
>
> I'm not sure I like the result. Simply applying your change to the face
> results in a color-less buffer with grey spots for both file headers and

It's good that the buffer is color-less since this improves readability
because in such cases colors add distraction.

> match highlights (at least, with my current theme).

Indeed, it adds grey lines for file headers, but there are
no grey lines for match highlights by default.

> It's less of a problem with diff-mode because its basic background is
> grey already.

The proposed change is for the default theme.
If the proposed change doesn't fit your personal theme customization,
this is fine.  At least, please mention in the docstring a suggestion
how the users could improve the readability of this face.

> I've also considered something like:
>
> (defface xref-file-header '((t :background "grey90" :extend t :inherit compilation-info))
>   "Face used to highlight file header in the xref buffer."
>   :version "27.1")
>
> ...but it makes the file names harder to read because of the
> reduced contrast.

I agree, this is not an improvement.

> Then I've tried to see what others do, for instance
> https://github.com/Wilfred/deadgrep, which I'd heard people praise
> usability of.
>
> The only distinction it adds to file names is "bold :t". And diff-mode also
> does that, actually.
>
> So... how do you like this simple change?
>
> (defface xref-file-header '((t :bold t :inherit compilation-info))
>   "Face used to highlight file header in the xref buffer."
>   :version "27.1")

This is exactly what I used as customization for a long time,
and it was a pain with so much of "visual garbage", until I realized
there is the existing solution in diff-mode faces.

It seems you're inclined to keep the green color because this is what
is used in grep, right?  But in grep, the green color on file names
doesn't add distraction because file names are located on the left,
whereas matches are on the right part of the output.  But in xref output
lines with file names and lines with matches are interlaced.

Another suggestion how to remove "visual garbage" is to truncate
duplicate prefixes: currently the prefixes of long absolute file names
are repeated for all file names.  It would improve readability
to display shorter relative file names without duplicate project root part.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 31.03.2021 20:05, Juri Linkov wrote:
>>>    -(defface xref-file-header '((t :inherit compilation-info))
>>> +(defface xref-file-header '((t :background "grey90" :extend t))
>>>      "Face used to highlight file header in the xref buffer."
>>>      :version "27.1")
>>> #+end_src
>>
>> I'm not sure I like the result. Simply applying your change to the face
>> results in a color-less buffer with grey spots for both file headers and
> 
> It's good that the buffer is color-less since this improves readability
> because in such cases colors add distraction.

But colors add emphasis as well, don't they? If something is written in 
a different color, that makes it stand out. Use too many colors - we get 
into the "angry fruit said" territory, but use too few, and you're 
leaving some tools on the table.

>> match highlights (at least, with my current theme).
> 
> Indeed, it adds grey lines for file headers, but there are
> no grey lines for match highlights by default.

Fair point, I've tried your proposal again with the default theme.

Highlighting color is not grey, so there's less of that color-less 
impression. The headers still get uncomfortably dark, kinda lifeless. I 
don't get the same impression in diff-mode buffers.

And in diff-mode buffers there is a lot of grey background near and 
around the file names already. So making their background a darker grey 
doesn't take away much from readability while providing the needed 
emphasis. And the color green is not an option there.

>> It's less of a problem with diff-mode because its basic background is
>> grey already.
> 
> The proposed change is for the default theme.
> If the proposed change doesn't fit your personal theme customization,
> this is fine.  At least, please mention in the docstring a suggestion
> how the users could improve the readability of this face.

The above is a comparison with another core package.

I'm fairly sure your suggestion is not going to improve readability, 
though it certainly does make lines with file names stand out more.

A carefully worded recommendation wouldn't hurt, but I wonder what would 
be the best place for it.

>> Then I've tried to see what others do, for instance
>> https://github.com/Wilfred/deadgrep, which I'd heard people praise
>> usability of.
>>
>> The only distinction it adds to file names is "bold :t". And diff-mode also
>> does that, actually.
>>
>> So... how do you like this simple change?
>>
>> (defface xref-file-header '((t :bold t :inherit compilation-info))
>>    "Face used to highlight file header in the xref buffer."
>>    :version "27.1")
> 
> This is exactly what I used as customization for a long time,
> and it was a pain with so much of "visual garbage", until I realized
> there is the existing solution in diff-mode faces.

The above was actually a mistake on my part, because the default theme 
already puts ':bold t' on the 'success' face, which is in turn inherited 
by 'compilation-info' and 'xref-file-header'. Thus my suggestion was a 
no-op.

Could you elaborate on the "visual garbage" part? If you mean the use of 
colors, then the default theme is not too shy about having bright 
colors. So at least that is consistent with other looks.

> It seems you're inclined to keep the green color because this is what
> is used in grep, right?  But in grep, the green color on file names
> doesn't add distraction because file names are located on the left,
> whereas matches are on the right part of the output.  But in xref output
> lines with file names and lines with matches are interlaced.

But it does create an analogy with Grep, which is one of the main places 
where we're used to seeing file names in a list. Thus looking at the 
green color we're likely to make a connection and see that this line 
shows a file name.

I'm not as much beholden to this color exactly, as to the idea of using 
*some* color for this purpose. And I don't know of better prior art.

Regarding file names on the left, Grep does that, but look at ripgrep in 
the console, or the deadgrep Emacs package. Neither add any extra 
decorations to the line except emphasizing it with a different color 
(admittedly not green in these two cases) and (maybe) bold font weight.

Or look at ack or SilverSearcher, which both use green for file names, 
while using the same grouping layout that we do (they had been an 
inspiration, even though we've inherited the xref UI from SLIME):

https://altbox.dev/ack/screenshot.png
https://spinorlab.wordpress.com/2015/08/15/the-silver-searcher/

> Another suggestion how to remove "visual garbage" is to truncate
> duplicate prefixes: currently the prefixes of long absolute file names
> are repeated for all file names.  It would improve readability
> to display shorter relative file names without duplicate project root part.

Please try (setq xref-file-name-display 'project-relative).

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

> But it does create an analogy with Grep, which is one of the main places
> where we're used to seeing file names in a list. Thus looking at the green
> color we're likely to make a connection and see that this line shows
> a file name.
>
> I'm not as much beholden to this color exactly, as to the idea of using
> *some* color for this purpose. And I don't know of better prior art.
>
> Regarding file names on the left, Grep does that, but look at ripgrep in
> the console, or the deadgrep Emacs package. Neither add any extra
> decorations to the line except emphasizing it with a different color
> (admittedly not green in these two cases) and (maybe) bold font weight.
>
> Or look at ack or SilverSearcher, which both use green for file names,
> while using the same grouping layout that we do (they had been an
> inspiration, even though we've inherited the xref UI from SLIME):
>
> https://altbox.dev/ack/screenshot.png
> https://spinorlab.wordpress.com/2015/08/15/the-silver-searcher/

But all of them display green foreground file names
with yellow background matches.  This is exactly
what grep.el does by default, so it's the reason
why output of grep.el is readable, but output of xref.el is not.

The problem is that currently in xref both file names and matches
use the same color green.  The only difference is that file names
are displayed with green foreground, and matches with green background.

What causes the difficulty in readability is the effect similar to
https://en.wikipedia.org/wiki/Stroop_effect that causes the
delay in reaction time between congruent and incongruent stimuli.

Using exactly the same grep colors in xref by changing
'xref-match' to inherit from the 'match' face
completely solves this problem.

>> Another suggestion how to remove "visual garbage" is to truncate
>> duplicate prefixes: currently the prefixes of long absolute file names
>> are repeated for all file names.  It would improve readability
>> to display shorter relative file names without duplicate project root part.
>
> Please try (setq xref-file-name-display 'project-relative).

Thanks, I didn't know about this.  Shouldn't this be the default value
since this is what's displayed by grep and ripgrep.  Actually,
there is no exact option for what grep and ripgrep do,
because they display file names relative to the search directory.
But currently there is no xref option to display file names
relative to the subdirectory specified by 'C-u C-x p g'.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 01.04.2021 11:43, Juri Linkov wrote:

>> https://altbox.dev/ack/screenshot.png
>> https://spinorlab.wordpress.com/2015/08/15/the-silver-searcher/
> 
> But all of them display green foreground file names
> with yellow background matches.  This is exactly
> what grep.el does by default, so it's the reason
> why output of grep.el is readable, but output of xref.el is not.

3/4 use something yellow-ish, yes (ripgrep in the console uses red 
foreground, for some reason).

> The problem is that currently in xref both file names and matches
> use the same color green.  The only difference is that file names
> are displayed with green foreground, and matches with green background.

Okay. Guess I haven't experienced that because in my theme highlight 
looks grey-ish. *sigh*

> What causes the difficulty in readability is the effect similar to
> https://en.wikipedia.org/wiki/Stroop_effect that causes the
> delay in reaction time between congruent and incongruent stimuli.
> 
> Using exactly the same grep colors in xref by changing
> 'xref-match' to inherit from the 'match' face
> completely solves this problem.

You are right. I could even say "unfortunately", because IMHO the bright 
yellow highlights are too much. Too strong emphasis, visually.

So let's change it to inherit from 'match', because that's what that 
face is documented to be used for.

Additionally, what do you think about toning down 'match''s background 
color? Maybe use some subtler yellow like "lemon chiffon" or "khaki1"? 
Or "light goldenrod".

ag and deadgrep use some darker khaki tones on the screenshots, but both 
probably assume dark background.

>>> Another suggestion how to remove "visual garbage" is to truncate
>>> duplicate prefixes: currently the prefixes of long absolute file names
>>> are repeated for all file names.  It would improve readability
>>> to display shorter relative file names without duplicate project root part.
>>
>> Please try (setq xref-file-name-display 'project-relative).
> 
> Thanks, I didn't know about this.  Shouldn't this be the default value
> since this is what's displayed by grep and ripgrep.

I wouldn't mind, personally.

> Actually,
> there is no exact option for what grep and ripgrep do,
> because they display file names relative to the search directory.
> But currently there is no xref option to display file names
> relative to the subdirectory specified by 'C-u C-x p g'.

This issue is tricky because xref-find-definitions does not assume the 
presence of a project, or even of any kind of containing directory. And 
yet, it's handy to show its results with relative file names when 
possible, too. So I picked "relative to the project" as the option 
value, and the corresponding logic.

I think what you're talking about is only a problem when the directory 
has no containing project at all. In that case we could probably default 
to the value of default-directory as the reference.

Not sure the logic is applicable to general xref output, though (e.g. 
xref-find-definitions search in a directory with TAGS but without .git). 
So perhaps that behavior should be opt-in, e.g. with an extra arg for 
xref--show-xrefs.

Or just keep it an exception, hoping people don't use 'C-u C-x p g' 
outside of projects often enough for this to be a nuisance.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

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

>> Using exactly the same grep colors in xref by changing
>> 'xref-match' to inherit from the 'match' face
>> completely solves this problem.
>
> You are right. I could even say "unfortunately", because IMHO the bright
> yellow highlights are too much. Too strong emphasis, visually.
>
> So let's change it to inherit from 'match', because that's what that face
> is documented to be used for.
>
> Additionally, what do you think about toning down 'match''s background
> color? Maybe use some subtler yellow like "lemon chiffon" or "khaki1"? Or
> "light goldenrod".

Such toning down is welcome since currently match's background is too intense.
Actually, I customized it long ago to "#ffff88" on one display,
and to "#ffffbb" on another display.  I guess "#ffffbb" is too radical,
but "#ffff88" should be fine and close to "khaki1" that is nicely looking as well.
Another variant is to update gradually, i.e. start with "#ffff66",
then after some time to "#ffff88".

>>> Please try (setq xref-file-name-display 'project-relative).
>> Thanks, I didn't know about this.  Shouldn't this be the default value
>> since this is what's displayed by grep and ripgrep.
>
> I wouldn't mind, personally.

This is added to the patch below too.

>> Actually, there is no exact option for what grep and ripgrep do,
>> because they display file names relative to the search directory.
>> But currently there is no xref option to display file names
>> relative to the subdirectory specified by 'C-u C-x p g'.
>
> This issue is tricky because xref-find-definitions does not assume the
> presence of a project, or even of any kind of containing directory. And
> yet, it's handy to show its results with relative file names when possible,
> too. So I picked "relative to the project" as the option value, and the
> corresponding logic.
>
> I think what you're talking about is only a problem when the directory has
> no containing project at all. In that case we could probably default to the
> value of default-directory as the reference.

Maybe it would be nice to default to default-directory even when
'C-u C-x p g' is used in a project.

What is the real problem for me is that after navigating to
a project's subdirectory (with e.g. dired) and typing 'C-u C-x p g',
it doesn't provide the current directory as the default value.
It inserts the project root by default, not its subdirectory:

  Base directory: /project/root/

whereas 'M-x rgrep' conveniently provides default-directory as default.

BTW, is it possible to make 'project-find-regexp' more compatible with 'rgrep'
in other features too?  What is missing is a way to modify the constructed
command line.  For example, often I need to add "-w" to the constructed command
to match words only.  In 'C-u M-x rgrep', this is easy to do,
but not in 'C-u C-x p g'.


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

diff --git a/lisp/replace.el b/lisp/replace.el
index f131d263ec..07b2d59a25 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1433,7 +1433,7 @@ occur-next-error
 \f
 (defface match
   '((((class color) (min-colors 88) (background light))
-     :background "yellow1")
+     :background "#ffff66")
     (((class color) (min-colors 88) (background dark))
      :background "RoyalBlue3")
     (((class color) (min-colors 8) (background light))
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index ea52befec5..cada1f1109 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -103,7 +103,7 @@ xref-match-length
 
 ;;;; Commonly needed location classes are defined here:
 
-(defcustom xref-file-name-display 'abs
+(defcustom xref-file-name-display 'project-relative
   "Style of file name display in *xref* buffers.
 
 If the value is the symbol `abs', the default, show the file names
@@ -521,7 +521,7 @@ xref-line-number
   "Face for displaying line numbers in the xref buffer."
   :version "27.1")
 
-(defface xref-match '((t :inherit highlight))
+(defface xref-match '((t :inherit match))
   "Face used to highlight matches in the xref buffer."
   :version "27.1")
 

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 01 Apr 2021 21:45:38 +0300
> Cc: 47012@debbugs.gnu.org
> 
> diff --git a/lisp/replace.el b/lisp/replace.el
> index f131d263ec..07b2d59a25 100644
> --- a/lisp/replace.el
> +++ b/lisp/replace.el
> @@ -1433,7 +1433,7 @@ occur-next-error
>  \f
>  (defface match
>    '((((class color) (min-colors 88) (background light))
> -     :background "yellow1")
> +     :background "#ffff66")

Please don't make this change, it makes the matches very hard to find.
Personally, I don't understand how you can use such a background
color, unless you also customize the default background.

In general, when discussing faces used by some feature (in this case
Xref and project.el), please don't change the defaults of unrelated
faces, let alone more general ones.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 01.04.2021 22:06, Eli Zaretskii wrote:
> Please don't make this change, it makes the matches very hard to find.
> Personally, I don't understand how you can use such a background
> color, unless you also customize the default background.

Did you like any of the options I suggested?

> In general, when discussing faces used by some feature (in this case
> Xref and project.el), please don't change the defaults of unrelated
> faces, let alone more general ones.

I agree it should be a separate change/discussion.

Should we continue that in a new bug report or on emacs-devel?

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> Cc: 47012@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 2 Apr 2021 00:28:25 +0300
> 
> On 01.04.2021 22:06, Eli Zaretskii wrote:
> > Please don't make this change, it makes the matches very hard to find.
> > Personally, I don't understand how you can use such a background
> > color, unless you also customize the default background.
> 
> Did you like any of the options I suggested?

I didn't try them, and re-reading the thread now I don't think I
understand which options are you referring to here.  There was one
suggestion, to add the bold attribute to the existing face, which you
later retracted because that face is already bold, but I see no other
concrete suggestions for changes in that face.  What am I missing?
Could you please list those options for which you'd like my opinion?

> > In general, when discussing faces used by some feature (in this case
> > Xref and project.el), please don't change the defaults of unrelated
> > faces, let alone more general ones.
> 
> I agree it should be a separate change/discussion.
> 
> Should we continue that in a new bug report or on emacs-devel?

If it's an important issue, probably the latter.  If it's just a tool
to fix some other face, I'd prefer not to change Occur faces at all,
and instead find an independent way of doing TRT with Xref and/or
project.el faces.  There's nothing wrong with the default Occur faces,
and I saw no complaints about it.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 02.04.2021 09:08, Eli Zaretskii wrote:

>> Did you like any of the options I suggested?
> 
> I didn't try them, and re-reading the thread now I don't think I
> understand which options are you referring to here.  There was one
> suggestion, to add the bold attribute to the existing face, which you
> later retracted because that face is already bold, but I see no other
> concrete suggestions for changes in that face.  What am I missing?
> Could you please list those options for which you'd like my opinion?

See bug#47574 for a self-contained explanation.

>>> In general, when discussing faces used by some feature (in this case
>>> Xref and project.el), please don't change the defaults of unrelated
>>> faces, let alone more general ones.
>>
>> I agree it should be a separate change/discussion.
>>
>> Should we continue that in a new bug report or on emacs-devel?
> 
> If it's an important issue, probably the latter.

Important enough for me to file a bug, but not to spend much time 
fighting over it.

> If it's just a tool
> to fix some other face, I'd prefer not to change Occur faces at all,
> and instead find an independent way of doing TRT with Xref and/or
> project.el faces.
Yes, of course. I'm suggesting to change the 'match' face definition 
because I think all its uses will benefit from it (including Occur and 
Grep which I made sure to try with all proposed colors).

'xref-match' face is customizable separately, but I don't think it will 
be productive to change only it but not 'match'.

The faces are used similarly enough, so it's hard to justify making them 
look different in the default configuration.

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> Cc: 47012@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 3 Apr 2021 02:50:52 +0300
> 
> On 02.04.2021 09:08, Eli Zaretskii wrote:
> 
> >> Did you like any of the options I suggested?
> > 
> > I didn't try them, and re-reading the thread now I don't think I
> > understand which options are you referring to here.  There was one
> > suggestion, to add the bold attribute to the existing face, which you
> > later retracted because that face is already bold, but I see no other
> > concrete suggestions for changes in that face.  What am I missing?
> > Could you please list those options for which you'd like my opinion?
> 
> See bug#47574 for a self-contained explanation.

That's about the 'match' face.  I thought you were asking about xref
and project.el, I guess I was confused.

The colors you suggest seem okay to me, with a possible exception of
lemon chiffon, but I only tried them on the default light background.

> 'xref-match' face is customizable separately, but I don't think it will 
> be productive to change only it but not 'match'.
> 
> The faces are used similarly enough, so it's hard to justify making them 
> look different in the default configuration.

I'm not sure I agree with "used similarly".  Are you talking ab out
the faces in the XREF buffer?  If so, they don't show matches within
context of a source line, they show symbols.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 03.04.2021 10:24, Eli Zaretskii wrote:

>> See bug#47574 for a self-contained explanation.
> 
> That's about the 'match' face.  I thought you were asking about xref
> and project.el, I guess I was confused.
> 
> The colors you suggest seem okay to me, with a possible exception of
> lemon chiffon, but I only tried them on the default light background.

Perhaps you will want to add that reply to the newer bug's comments. 
Because the current comment seems more like a rejection.

lemon chiffon is indeed more subtle than the other colors in the default 
palette, so I agree it will be better used as personal customization.

>> 'xref-match' face is customizable separately, but I don't think it will
>> be productive to change only it but not 'match'.
>>
>> The faces are used similarly enough, so it's hard to justify making them
>> look different in the default configuration.
> 
> I'm not sure I agree with "used similarly".  Are you talking ab out
> the faces in the XREF buffer?  If so, they don't show matches within
> context of a source line, they show symbols.

Please take a look at the output of xref-find-references or 
project-find-regexp.

bug#47012: xref copies keymap properties to minibuffer

[Eli Zaretskii]

> Cc: 47012@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 3 Apr 2021 21:12:05 +0300
> 
> On 03.04.2021 10:24, Eli Zaretskii wrote:
> 
> >> See bug#47574 for a self-contained explanation.
> > 
> > That's about the 'match' face.  I thought you were asking about xref
> > and project.el, I guess I was confused.
> > 
> > The colors you suggest seem okay to me, with a possible exception of
> > lemon chiffon, but I only tried them on the default light background.
> 
> Perhaps you will want to add that reply to the newer bug's comments. 
> Because the current comment seems more like a rejection.

It isn't a rejection, not at all.

> > I'm not sure I agree with "used similarly".  Are you talking ab out
> > the faces in the XREF buffer?  If so, they don't show matches within
> > context of a source line, they show symbols.
> 
> Please take a look at the output of xref-find-references or 
> project-find-regexp.

Ah, okay.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>> diff --git a/lisp/replace.el b/lisp/replace.el
>> index f131d263ec..07b2d59a25 100644
>> --- a/lisp/replace.el
>> +++ b/lisp/replace.el
>> @@ -1433,7 +1433,7 @@ occur-next-error
>>  \f
>>  (defface match
>>    '((((class color) (min-colors 88) (background light))
>> -     :background "yellow1")
>> +     :background "#ffff66")
>
> Please don't make this change, it makes the matches very hard to find.
> Personally, I don't understand how you can use such a background
> color, unless you also customize the default background.

The visual perception depends on the display device and its settings indeed.
For example, I have different color customization for different display types.
So better to leave this unchanged.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 01.04.2021 21:45, Juri Linkov wrote:

>> Additionally, what do you think about toning down 'match''s background
>> color? Maybe use some subtler yellow like "lemon chiffon" or "khaki1"? Or
>> "light goldenrod".
> 
> Such toning down is welcome since currently match's background is too intense.
> Actually, I customized it long ago to "#ffff88" on one display,
> and to "#ffffbb" on another display.  I guess "#ffffbb" is too radical,
> but "#ffff88" should be fine and close to "khaki1" that is nicely looking as well.
> Another variant is to update gradually, i.e. start with "#ffff66",
> then after some time to "#ffff88".

I like either of these, just not #ffff66 (still too bright). Perhaps 
some darker variant would be a better option if being easy-to-notice 
becomes a problem (as Eli's response seems to indicate). Maybe we should 
ask others as well.

Anyway, let's hold off on this part of the change until further discussion.

>>>> Please try (setq xref-file-name-display 'project-relative).
>>> Thanks, I didn't know about this.  Shouldn't this be the default value
>>> since this is what's displayed by grep and ripgrep.
>>
>> I wouldn't mind, personally.
> 
> This is added to the patch below too.

LGTM.

>>> Actually, there is no exact option for what grep and ripgrep do,
>>> because they display file names relative to the search directory.
>>> But currently there is no xref option to display file names
>>> relative to the subdirectory specified by 'C-u C-x p g'.
>>
>> This issue is tricky because xref-find-definitions does not assume the
>> presence of a project, or even of any kind of containing directory. And
>> yet, it's handy to show its results with relative file names when possible,
>> too. So I picked "relative to the project" as the option value, and the
>> corresponding logic.
>>
>> I think what you're talking about is only a problem when the directory has
>> no containing project at all. In that case we could probably default to the
>> value of default-directory as the reference.
> 
> Maybe it would be nice to default to default-directory even when
> 'C-u C-x p g' is used in a project.

That's not the question, though. The question is whether we default to 
default-directory when outside of recognizable projects. Or how to 
differentiate 'C-u C-x p g' from other cases, such as xref-find-definitions.

If this is not urgent, let's leave it for a separate bug report.

> What is the real problem for me is that after navigating to
> a project's subdirectory (with e.g. dired) and typing 'C-u C-x p g',
> it doesn't provide the current directory as the default value.
> It inserts the project root by default, not its subdirectory:
> 
>    Base directory: /project/root/
> 
> whereas 'M-x rgrep' conveniently provides default-directory as default.

Makes sense, fixed in 4798dc0c51, please check it out.

> BTW, is it possible to make 'project-find-regexp' more compatible with 'rgrep'
> in other features too?  What is missing is a way to modify the constructed
> command line.  For example, often I need to add "-w" to the constructed command
> to match words only.  In 'C-u M-x rgrep', this is easy to do,
> but not in 'C-u C-x p g'.

That's not so easy to do: the exact command is concealed inside the 
helper function in another package (xref). I suppose it's possible to 
rearrange things such that command creation and its execution are two 
different phases, but TBH I wouldn't love the result. Though I agree it 
might be handy.

What I've been thinking we should have instead, is some kind of 
graphical prompt with multiple fields, where you can by default input 
the regexp and press RET, but you can also see the other options (like 
the file name glob to filter by, whether to search the "external roots" 
or not, whether to search only a particular directory, whether to ignore 
case, whether to search in the project-ignored files as well; options 
which modify the regexp or matching logic like your -w could be added too).

Note that several of the options enumerated above are not something we 
could expose in the "edit the command" interface, because the command 
gets the list of files from stdin.

Maybe it would be presented like one-line prompt where you can reach 
further fields using TAB, and maybe expand into some multiline pane 
(still inside the minibuffer) if some options can't fit on the same 
line, and you reach the end of that line by TAB-bing.

To sum up, if we managed to create some visual interface for specifying 
the options that project-find-regexp has control over, maybe it would 
both result in a less complex interaction between packages, as well as 
in a more powerful UI which more people will be happy with.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>>>>> Please try (setq xref-file-name-display 'project-relative).
>>>> Thanks, I didn't know about this.  Shouldn't this be the default value
>>>> since this is what's displayed by grep and ripgrep.
>>>
>>> I wouldn't mind, personally.
>> This is added to the patch below too.
>
> LGTM.

Pushed now.

>> What is the real problem for me is that after navigating to
>> a project's subdirectory (with e.g. dired) and typing 'C-u C-x p g',
>> it doesn't provide the current directory as the default value.
>> It inserts the project root by default, not its subdirectory:
>>    Base directory: /project/root/
>> whereas 'M-x rgrep' conveniently provides default-directory as default.
>
> Makes sense, fixed in 4798dc0c51, please check it out.

Thanks, now it's more handy.

>> BTW, is it possible to make 'project-find-regexp' more compatible with 'rgrep'
>> in other features too?  What is missing is a way to modify the constructed
>> command line.  For example, often I need to add "-w" to the constructed command
>> to match words only.  In 'C-u M-x rgrep', this is easy to do,
>> but not in 'C-u C-x p g'.
>
> That's not so easy to do: the exact command is concealed inside the helper
> function in another package (xref). I suppose it's possible to rearrange
> things such that command creation and its execution are two different
> phases, but TBH I wouldn't love the result. Though I agree it might
> be handy.

This is the simplest implementation:

#+begin_src emacs-lisp
(defun project-find-word (regexp)
  "Word-based version of ‘project-find-regexp’.
Modifies the ‘xref-search-program-alist’ template
to add the option ‘-w’ that matches whole words."
  (interactive (list (project--read-regexp)))
  (let ((xref-search-program-alist
         (mapcar (lambda (p)
                   (cons (car p) (replace-regexp-in-string "<C>" "-w \\&" (cdr p))))
                 xref-search-program-alist)))
    (project-find-regexp regexp)))
#+end_src

It has one minor issue:
while it correctly filters out lines without word matches,
when a line with a word match contains also the same string
that is not a complete word, then both are highlighted as matches.
There is no such problem in grep where matches are highlighted
by the grep program itself.

BTW, the above implementation was based on a similar command for rgrep:

#+begin_src emacs-lisp
(defun wrgrep ()
  "Word-based version of ‘rgrep’.
Modifies the grep-find template to add the option ‘-w’ that matches whole words."
  (interactive)
  (let ((grep-host-defaults-alist nil)
        (grep-find-template
         (replace-regexp-in-string "<C>" "-w \\&" grep-find-template)))
    (call-interactively 'rgrep)))
#+end_src

> What I've been thinking we should have instead, is some kind of graphical
> prompt with multiple fields, where you can by default input the regexp and
> press RET, but you can also see the other options (like the file name glob
> to filter by, whether to search the "external roots" or not, whether to
> search only a particular directory, whether to ignore case, whether to
> search in the project-ignored files as well; options which modify the
> regexp or matching logic like your -w could be added too).
>
> Note that several of the options enumerated above are not something we
> could expose in the "edit the command" interface, because the command gets
> the list of files from stdin.
>
> Maybe it would be presented like one-line prompt where you can reach
> further fields using TAB, and maybe expand into some multiline pane (still
> inside the minibuffer) if some options can't fit on the same line, and you
> reach the end of that line by TAB-bing.
>
> To sum up, if we managed to create some visual interface for specifying the
> options that project-find-regexp has control over, maybe it would both
> result in a less complex interaction between packages, as well as in a more
> powerful UI which more people will be happy with.

Sounds like a widget-based form-filling with fields.  Actually, such fields
already exist in xref-search-program-alist template with placeholders
<D>, <X>, <F>, <C>, <R> that are expanded by grep-expand-template.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 02.04.2021 11:25, Juri Linkov wrote:
>>>>>> Please try (setq xref-file-name-display 'project-relative).
>>>>> Thanks, I didn't know about this.  Shouldn't this be the default value
>>>>> since this is what's displayed by grep and ripgrep.
>>>>
>>>> I wouldn't mind, personally.
>>> This is added to the patch below too.
>>
>> LGTM.
> 
> Pushed now.

Thanks.

>> That's not so easy to do: the exact command is concealed inside the helper
>> function in another package (xref). I suppose it's possible to rearrange
>> things such that command creation and its execution are two different
>> phases, but TBH I wouldn't love the result. Though I agree it might
>> be handy.
> 
> This is the simplest implementation:
> 
> #+begin_src emacs-lisp
> (defun project-find-word (regexp)
>    "Word-based version of ‘project-find-regexp’.
> Modifies the ‘xref-search-program-alist’ template
> to add the option ‘-w’ that matches whole words."
>    (interactive (list (project--read-regexp)))
>    (let ((xref-search-program-alist
>           (mapcar (lambda (p)
>                     (cons (car p) (replace-regexp-in-string "<C>" "-w \\&" (cdr p))))
>                   xref-search-program-alist)))
>      (project-find-regexp regexp)))
> #+end_src

Wouldn't it work the same if you instead modify the regexp to be 
surrounded with \b...\b?

> It has one minor issue:
> while it correctly filters out lines without word matches,
> when a line with a word match contains also the same string
> that is not a complete word, then both are highlighted as matches.
> There is no such problem in grep where matches are highlighted
> by the grep program itself.
> 
> BTW, the above implementation was based on a similar command for rgrep:

If you use the above suggestion, it should fix highlighting as well.

>> To sum up, if we managed to create some visual interface for specifying the
>> options that project-find-regexp has control over, maybe it would both
>> result in a less complex interaction between packages, as well as in a more
>> powerful UI which more people will be happy with.
> 
> Sounds like a widget-based form-filling with fields.  Actually, such fields
> already exist in xref-search-program-alist template with placeholders
> <D>, <X>, <F>, <C>, <R> that are expanded by grep-expand-template.

As you can see, there are only two fields in the actual entries there. 
But we want to be able to modify more parameters.

In the spirit of Emacs, it should more keyboard-driven than, say, 
Customize widgets. I don't have the full workflow in mind yet, but it 
could use the same separator as query-replace history, only with more 
complex completion logic.

bug#47012: xref copies keymap properties to minibuffer

[Juri Linkov]

>> This is the simplest implementation:
>> #+begin_src emacs-lisp
>> (defun project-find-word (regexp)
>>    "Word-based version of ‘project-find-regexp’.
>> Modifies the ‘xref-search-program-alist’ template
>> to add the option ‘-w’ that matches whole words."
>>    (interactive (list (project--read-regexp)))
>>    (let ((xref-search-program-alist
>>           (mapcar (lambda (p)
>>                     (cons (car p) (replace-regexp-in-string "<C>" "-w \\&" (cdr p))))
>>                   xref-search-program-alist)))
>>      (project-find-regexp regexp)))
>> #+end_src
>
> Wouldn't it work the same if you instead modify the regexp to be surrounded
> with \b...\b?

Indeed, with more typing.  Ideally, there should be an isearch command
that will send the constructed regexp to 'project-find-regexp' from
isearch word-mode, symbol-mode like:

#+begin_src emacs-lisp
(define-key isearch-mode-map "\C-xpg" 'isearch-project-find-regexp)

(defun isearch-project-find-regexp ()
  (interactive)
  (let ((isearch-recursive-edit nil))
    (isearch-done nil t)
    (isearch-clean-overlays))
  (let ((regexp (cond ((functionp isearch-regexp-function)
                       (funcall isearch-regexp-function isearch-string))
		      (isearch-regexp-function (word-search-regexp isearch-string))
		      (isearch-regexp isearch-string)
		      (t (regexp-quote isearch-string)))))
    (project-find-regexp regexp))
  (and isearch-recursive-edit (exit-recursive-edit)))
#+end_src

But unfortunately it fails on ripgrep with:

  Search failed with status 123: regex parse error

Maybe because ripgrep can't swallow Emacs regexps.

bug#47012: xref copies keymap properties to minibuffer

[Dmitry Gutov]

On 05.04.2021 01:55, Juri Linkov wrote:
>>> This is the simplest implementation:
>>> #+begin_src emacs-lisp
>>> (defun project-find-word (regexp)
>>>     "Word-based version of ‘project-find-regexp’.
>>> Modifies the ‘xref-search-program-alist’ template
>>> to add the option ‘-w’ that matches whole words."
>>>     (interactive (list (project--read-regexp)))
>>>     (let ((xref-search-program-alist
>>>            (mapcar (lambda (p)
>>>                      (cons (car p) (replace-regexp-in-string "<C>" "-w \\&" (cdr p))))
>>>                    xref-search-program-alist)))
>>>       (project-find-regexp regexp)))
>>> #+end_src
>>
>> Wouldn't it work the same if you instead modify the regexp to be surrounded
>> with \b...\b?
> 
> Indeed, with more typing.

I meant that your project-find-word would do that to the regexp, instead 
of trying to alter the template.

> Ideally, there should be an isearch command
> that will send the constructed regexp to 'project-find-regexp' from
> isearch word-mode, symbol-mode like:
> 
> #+begin_src emacs-lisp
> (define-key isearch-mode-map "\C-xpg" 'isearch-project-find-regexp)
> 
> (defun isearch-project-find-regexp ()
>    (interactive)
>    (let ((isearch-recursive-edit nil))
>      (isearch-done nil t)
>      (isearch-clean-overlays))
>    (let ((regexp (cond ((functionp isearch-regexp-function)
>                         (funcall isearch-regexp-function isearch-string))
> 		      (isearch-regexp-function (word-search-regexp isearch-string))
> 		      (isearch-regexp isearch-string)
> 		      (t (regexp-quote isearch-string)))))
>      (project-find-regexp regexp))
>    (and isearch-recursive-edit (exit-recursive-edit)))
> #+end_src
> 
> But unfortunately it fails on ripgrep with:
> 
>    Search failed with status 123: regex parse error
> 
> Maybe because ripgrep can't swallow Emacs regexps.

That's right: it doesn't understand constructs like "\\<ChangeLog\\>".

I've been meaning to experiment with removing Emacs-specific 
instructions from the regexp, using the result in a search, and then 
postprocessing with "correct" regexp later, but still haven't gotten 
around to that.

FIXME in xref--regexp-to-extended is somewhat related.