bug#47012: xref copies keymap properties to minibuffer

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
>> 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 :-)



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
>> 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):


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 ()
Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
>> 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)



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
>>>    (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



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Eli Zaretskii
> From: Dmitry Gutov <[hidden email]>
> Date: Wed, 31 Mar 2021 18:47:30 +0300
> Cc: [hidden email]
>
> (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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
In reply to this post by Juri Linkov-2
>>   -(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.



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
>> 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'.


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
 
 (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")
 
Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Eli Zaretskii
> From: Juri Linkov <[hidden email]>
> Date: Thu, 01 Apr 2021 21:45:38 +0300
> Cc: [hidden email]
>
> 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
>  
>  (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.



Reply | Threaded
Open this post in threaded view
|

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?



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Dmitry Gutov
In reply to this post by Juri Linkov-2
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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Eli Zaretskii
In reply to this post by Dmitry Gutov
> Cc: [hidden email]
> From: Dmitry Gutov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Juri Linkov-2
In reply to this post by Dmitry Gutov
>>>>> 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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Dmitry Gutov
In reply to this post by Eli Zaretskii
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.



Reply | Threaded
Open this post in threaded view
|

bug#47012: xref copies keymap properties to minibuffer

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> 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.



12