bug#61340: 29.0.60; Extra space in xref buffer

bug#61340: 29.0.60; Extra space in xref buffer

[Juri Linkov]

This is a small problem and I didn't notice it until now when
there was an important distinction whether there is a space
character at the beginning of the line, and it was misleading
to see that xref wrongly says there is a leading space.
This is because currently xref output differs from grep and occur
where there is no space between colon and the text from file.
Here is the fix:

```
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index f01b8a1af18..6160f217afb 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1108,7 +1108,7 @@ xref--insert-xrefs
                                    maximize (xref-location-line
                                              (xref-item-location xref)))
            for line-format = (and max-line
-                                  (format "%%%dd: " (1+ (floor (log max-line 10)))))
+                                  (format "%%%dd:" (1+ (floor (log max-line 10)))))
            with item-text-props = (list 'mouse-face 'highlight
                                         'keymap xref--button-map
                                         'help-echo
```

bug#61340: 29.0.60; Extra space in xref buffer

[Dmitry Gutov]

Hi!

On 07/02/2023 09:40, Juri Linkov wrote:
> This is a small problem and I didn't notice it until now when
> there was an important distinction whether there is a space
> character at the beginning of the line, and it was misleading
> to see that xref wrongly says there is a leading space.
> This is because currently xref output differs from grep and occur
> where there is no space between colon and the text from file.
> Here is the fix:
> 
> ```
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index f01b8a1af18..6160f217afb 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -1108,7 +1108,7 @@ xref--insert-xrefs
>                                      maximize (xref-location-line
>                                                (xref-item-location xref)))
>              for line-format = (and max-line
> -                                  (format "%%%dd: " (1+ (floor (log max-line 10)))))
> +                                  (format "%%%dd:" (1+ (floor (log max-line 10)))))
>              with item-text-props = (list 'mouse-face 'highlight
>                                           'keymap xref--button-map
>                                           'help-echo
> ```

This was originally an effort to give the outputted text some "breathing 
room", and I think it looks a little better.

But if it can cause problems sometimes, I don't mind the proposed 
change. After all, both Grep and Occur don't have this space, and 
consistency is good.

bug#61340: 29.0.60; Extra space in xref buffer

[Juri Linkov]

close 61340 30.0.50
thanks

>> @@ -1108,7 +1108,7 @@ xref--insert-xrefs
>>                                      maximize (xref-location-line
>>                                                (xref-item-location xref)))
>>              for line-format = (and max-line
>> -                                  (format "%%%dd: " (1+ (floor (log max-line 10)))))
>> +                                  (format "%%%dd:" (1+ (floor (log max-line 10)))))
>>              with item-text-props = (list 'mouse-face 'highlight
>>                                           'keymap xref--button-map
>>                                           'help-echo
>> ```
>
> This was originally an effort to give the outputted text some "breathing
> room", and I think it looks a little better.

Indeed, it looks better but unfortunately at the cost of caused ambiguity.
I guess this is the reason why Grep and Occur don't add space.

> But if it can cause problems sometimes, I don't mind the proposed
> change. After all, both Grep and Occur don't have this space, and
> consistency is good.

Thanks, so now pushed to master.

bug#61340: 29.0.60; Extra space in xref buffer

[Dmitry Gutov]

On 08/02/2023 09:35, Juri Linkov wrote:
> close 61340 30.0.50
> thanks
> 
>>> @@ -1108,7 +1108,7 @@ xref--insert-xrefs
>>>                                       maximize (xref-location-line
>>>                                                 (xref-item-location xref)))
>>>               for line-format = (and max-line
>>> -                                  (format "%%%dd: " (1+ (floor (log max-line 10)))))
>>> +                                  (format "%%%dd:" (1+ (floor (log max-line 10)))))
>>>               with item-text-props = (list 'mouse-face 'highlight
>>>                                            'keymap xref--button-map
>>>                                            'help-echo
>>> ```
>> This was originally an effort to give the outputted text some "breathing
>> room", and I think it looks a little better.
> Indeed, it looks better but unfortunately at the cost of caused ambiguity.
> I guess this is the reason why Grep and Occur don't add space.
> 

I suppose we could try to tone down the colon. Occur uses 'shadow' for 
both the number and the colon; Grep uses the color from 'default' (with 
an underline).

If we fontify it with 'shadow' rather than 'xref-line-number', that 
might look a little better.

bug#61340: 29.0.60; Extra space in xref buffer

[Juri Linkov]

>>> This was originally an effort to give the outputted text some "breathing
>>> room", and I think it looks a little better.
>> Indeed, it looks better but unfortunately at the cost of caused ambiguity.
>> I guess this is the reason why Grep and Occur don't add space.
>
> I suppose we could try to tone down the colon. Occur uses 'shadow' for both
> the number and the colon; Grep uses the color from 'default' (with an
> underline).
>
> If we fontify it with 'shadow' rather than 'xref-line-number', that might
> look a little better.

I already forgot that the default color is too glaring since I customized
it long ago to '(xref-line-number ((t (:inherit shadow)))).  But I also
customized other xref faces e.g. '(xref-file-header ((t (:extend t
:background "grey90")))), so these faces provide a better look.
OTOH, I'm not sure if the default of xref-line-number can be changed
because on the one hand the purple color from compilation-line-number
is needed in grep to make them more noticeable because the line numbers
are in the middle of the line unlike in occur.  But on the other hand
the current theme of xref buffers matches all colors of grep, so any
change will make their default themes different.

bug#61340: 29.0.60; Extra space in xref buffer

[Dmitry Gutov]

On 08/02/2023 20:51, Juri Linkov wrote:
>>>> This was originally an effort to give the outputted text some "breathing
>>>> room", and I think it looks a little better.
>>> Indeed, it looks better but unfortunately at the cost of caused ambiguity.
>>> I guess this is the reason why Grep and Occur don't add space.
>> I suppose we could try to tone down the colon. Occur uses 'shadow' for both
>> the number and the colon; Grep uses the color from 'default' (with an
>> underline).
>>
>> If we fontify it with 'shadow' rather than 'xref-line-number', that might
>> look a little better.
> I already forgot that the default color is too glaring since I customized
> it long ago to '(xref-line-number ((t (:inherit shadow)))).  But I also
> customized other xref faces e.g. '(xref-file-header ((t (:extend t
> :background "grey90")))), so these faces provide a better look.
> OTOH, I'm not sure if the default of xref-line-number can be changed
> because on the one hand the purple color from compilation-line-number
> is needed in grep to make them more noticeable because the line numbers
> are in the middle of the line unlike in occur.  But on the other hand
> the current theme of xref buffers matches all colors of grep, so any
> change will make their default themes different.

Here's the change I was suggesting.

The difference is subtle, but seems like an improvement:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 63e065e696e..581eda0513e 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1126,7 +1126,9 @@ xref--insert-xrefs
                                     maximize (xref-location-line
                                               (xref-item-location xref)))
             for line-format = (and max-line
-                                  (format "%%%dd:" (1+ (floor (log 
max-line 10)))))
+                                  (format
+                                   #("%%%dd:" 0 4 (face 
xref-line-number) 5 6 (face shadow))
+                                   (1+ (floor (log max-line 10)))))
             with item-text-props = (list 'mouse-face 'highlight
                                          'keymap xref--button-map
                                          'help-echo
@@ -1146,8 +1148,7 @@ xref--insert-xrefs
                          ((and (equal line prev-line)
                                (equal prev-group group))
                           "")
-                        (t (propertize (format line-format line)
-                                       'face 'xref-line-number)))))
+                        (t (format line-format line)))))
                   ;; Render multiple matches on the same line, together.
                   (when (and (equal prev-group group)
                              (or (null line)

bug#61340: 29.0.60; Extra space in xref buffer

[Juri Linkov]

> Here's the change I was suggesting.
>
> The difference is subtle, but seems like an improvement:
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 63e065e696e..581eda0513e 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -1126,7 +1126,9 @@ xref--insert-xrefs
>                                     maximize (xref-location-line
>                                               (xref-item-location xref)))
>             for line-format = (and max-line
> -                                  (format "%%%dd:" (1+ (floor (log
>                                    max-line 10)))))
> +                                  (format
> +                                   #("%%%dd:" 0 4 (face xref-line-number)
> 5 6 (face shadow))

Thanks, this looks nice and closer to the grep output fontification.

bug#61340: 29.0.60; Extra space in xref buffer

[Dmitry Gutov]

On 09/02/2023 19:51, Juri Linkov wrote:
>> Here's the change I was suggesting.
>>
>> The difference is subtle, but seems like an improvement:
>>
>> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
>> index 63e065e696e..581eda0513e 100644
>> --- a/lisp/progmodes/xref.el
>> +++ b/lisp/progmodes/xref.el
>> @@ -1126,7 +1126,9 @@ xref--insert-xrefs
>>                                      maximize (xref-location-line
>>                                                (xref-item-location xref)))
>>              for line-format = (and max-line
>> -                                  (format "%%%dd:" (1+ (floor (log
>>                                     max-line 10)))))
>> +                                  (format
>> +                                   #("%%%dd:" 0 4 (face xref-line-number)
>> 5 6 (face shadow))
> Thanks, this looks nice and closer to the grep output fontification.

Thanks for testing, pushed to master.

bug#61340: 29.0.60; Extra space in xref buffer

[Dmitry Gutov]

Hi Eli,

Should we backport this to emacs-29? The improvement is minor, but the 
risk is very small as well.

On 09/02/2023 21:58, Dmitry Gutov wrote:
> On 09/02/2023 19:51, Juri Linkov wrote:
>>> Here's the change I was suggesting.
>>>
>>> The difference is subtle, but seems like an improvement:
>>>
>>> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
>>> index 63e065e696e..581eda0513e 100644
>>> --- a/lisp/progmodes/xref.el
>>> +++ b/lisp/progmodes/xref.el
>>> @@ -1126,7 +1126,9 @@ xref--insert-xrefs
>>>                                      maximize (xref-location-line
>>>                                                (xref-item-location 
>>> xref)))
>>>              for line-format = (and max-line
>>> -                                  (format "%%%dd:" (1+ (floor (log
>>>                                     max-line 10)))))
>>> +                                  (format
>>> +                                   #("%%%dd:" 0 4 (face 
>>> xref-line-number)
>>> 5 6 (face shadow))
>> Thanks, this looks nice and closer to the grep output fontification.

bug#61340: 29.0.60; Extra space in xref buffer

[Eli Zaretskii]

> Date: Sun, 19 Feb 2023 00:04:05 +0200
> From: Dmitry Gutov <dgutov@yandex.ru>
> Cc: 61340@debbugs.gnu.org, Juri Linkov <juri@linkov.net>
> 
> Should we backport this to emacs-29? The improvement is minor, but the 
> risk is very small as well.

Fine by me, thanks.

bug#61340: 29.0.60; Extra space in xref buffer

[Dmitry Gutov]

On 19/02/2023 08:20, Eli Zaretskii wrote:
>> Date: Sun, 19 Feb 2023 00:04:05 +0200
>> From: Dmitry Gutov<dgutov@yandex.ru>
>> Cc:61340@debbugs.gnu.org, Juri Linkov<juri@linkov.net>
>>
>> Should we backport this to emacs-29? The improvement is minor, but the
>> risk is very small as well.
> Fine by me, thanks.

Thank you, done.