unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#36967: 27.0.50; Duplicate lines in xref output
@ 2019-08-07 21:52 Juri Linkov
  2019-08-07 22:13 ` Dmitry Gutov
  0 siblings, 1 reply; 10+ messages in thread
From: Juri Linkov @ 2019-08-07 21:52 UTC (permalink / raw)
  To: 36967

I tried to use project-find-regexp more often than rgrep,
but unfortunately xref still has a fundamental flaw:

0. emacs -Q
1. M-x project-find-regexp RET regexp RET
2. The output buffer *xref* contains duplicate lines
   when regexp is found on the same line several times,
   each duplicate output line has separate highlighting
   for every regexp occurrence.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2019-08-07 21:52 bug#36967: 27.0.50; Duplicate lines in xref output Juri Linkov
@ 2019-08-07 22:13 ` Dmitry Gutov
  2020-12-02 21:30   ` Juri Linkov
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Gutov @ 2019-08-07 22:13 UTC (permalink / raw)
  To: Juri Linkov, 36967

On 08.08.2019 0:52, Juri Linkov wrote:
> I tried to use project-find-regexp more often than rgrep,
> but unfortunately xref still has a fundamental flaw:
> 
> 0. emacs -Q
> 1. M-x project-find-regexp RET regexp RET
> 2. The output buffer *xref* contains duplicate lines
>     when regexp is found on the same line several times,
>     each duplicate output line has separate highlighting
>     for every regexp occurrence.

I don't know how "fundamental" it is, but indeed, it's somewhat of a 
drawback. Suggestions for improving it (API change and/or implementation 
change) are welcome.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2019-08-07 22:13 ` Dmitry Gutov
@ 2020-12-02 21:30   ` Juri Linkov
  2020-12-03  1:35     ` Dmitry Gutov
  0 siblings, 1 reply; 10+ messages in thread
From: Juri Linkov @ 2020-12-02 21:30 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 36967

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

>> I tried to use project-find-regexp more often than rgrep,
>> but unfortunately xref still has a fundamental flaw:
>> 0. emacs -Q
>> 1. M-x project-find-regexp RET regexp RET
>> 2. The output buffer *xref* contains duplicate lines
>>     when regexp is found on the same line several times,
>>     each duplicate output line has separate highlighting
>>     for every regexp occurrence.
>
> I don't know how "fundamental" it is, but indeed, it's somewhat of
> a drawback. Suggestions for improving it (API change and/or implementation
> change) are welcome.

Here is the patch that makes the broken project-find-regexp usable:


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

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 3b19debb79..1f5e45f20d 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1461,11 +1461,10 @@ xref--collect-matches
                                  syntax-needed)))))
 
 (defun xref--collect-matches-1 (regexp file line line-beg line-end syntax-needed)
-  (let (matches)
+  (let ((summary (buffer-substring line-beg line-end))
+        matches)
     (when syntax-needed
       (syntax-propertize line-end))
-    ;; FIXME: This results in several lines with the same
-    ;; summary. Solve with composite pattern?
     (while (and
             ;; REGEXP might match an empty string.  Or line.
             (or (null matches)
@@ -1473,12 +1472,12 @@ xref--collect-matches-1
             (re-search-forward regexp line-end t))
       (let* ((beg-column (- (match-beginning 0) line-beg))
              (end-column (- (match-end 0) line-beg))
-             (loc (xref-make-file-location file line beg-column))
-             (summary (buffer-substring line-beg line-end)))
+             (loc (xref-make-file-location file line beg-column)))
         (add-face-text-property beg-column end-column 'xref-match
                                 t summary)
-        (push (xref-make-match summary loc (- end-column beg-column))
-              matches)))
+        (unless matches
+          (push (xref-make-match summary loc (- end-column beg-column))
+                matches))))
     (nreverse matches)))
 
 (defun xref--find-file-buffer (file)

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-02 21:30   ` Juri Linkov
@ 2020-12-03  1:35     ` Dmitry Gutov
  2020-12-03 21:30       ` Juri Linkov
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Gutov @ 2020-12-03  1:35 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 36967

On 02.12.2020 23:30, Juri Linkov wrote:
>>> I tried to use project-find-regexp more often than rgrep,
>>> but unfortunately xref still has a fundamental flaw:
>>> 0. emacs -Q
>>> 1. M-x project-find-regexp RET regexp RET
>>> 2. The output buffer *xref* contains duplicate lines
>>>      when regexp is found on the same line several times,
>>>      each duplicate output line has separate highlighting
>>>      for every regexp occurrence.
>>
>> I don't know how "fundamental" it is, but indeed, it's somewhat of
>> a drawback. Suggestions for improving it (API change and/or implementation
>> change) are welcome.
> 
> Here is the patch that makes the broken

Pretty harsh there.

> project-find-regexp usable:

Okay, impressions:

When the line has two matches, the new code only collects the first 
match. So 'matches' is always an list with one element (or nil).

Upside: repetitions are not shown anymore, but the match highlighting is 
still applied.

Downside: xref-query-replace-in-results won't work in those cases 
anymore; it will only replace one match. Because the list only contains 
one location, and not all of them. And that command is pretty nice to have.

Here's an alternative proposal:

Combine the lines inside the rendering code instead.

So each xref will have a separate location, but then xref--insert-xrefs 
will see that xref-location-line value repeats across some consecutive 
locations, and will combine them into single line with some text 
property magic (basically, copying the summary from one of them, and 
then applying 'xref-item and 'face properties appropriately). This 
retains the xref item semantics (as opposed to, say, associating an xref 
item with multiple locations). And _hopefully_ the replace-related code 
won't need any changes.

As a bonus, 'n' and 'p' should then automatically change behavior to 
jump between locations when they are on the same line.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-03  1:35     ` Dmitry Gutov
@ 2020-12-03 21:30       ` Juri Linkov
  2020-12-03 23:44         ` Dmitry Gutov
  0 siblings, 1 reply; 10+ messages in thread
From: Juri Linkov @ 2020-12-03 21:30 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 36967

>> Here is the patch that makes the broken
>
> Pretty harsh there.

But true: I can't use it in the current form, and was waiting
when someone will fix it.

>> project-find-regexp usable:
>
> Downside: xref-query-replace-in-results won't work in those cases anymore;
> it will only replace one match. Because the list only contains one
> location, and not all of them. And that command is pretty nice to have.

Sorry, I missed this use case because I still know too little about details
of xref.el.

> Here's an alternative proposal:
>
> Combine the lines inside the rendering code instead.
>
> So each xref will have a separate location, but then xref--insert-xrefs
> will see that xref-location-line value repeats across some consecutive
> locations, and will combine them into single line with some text property
> magic (basically, copying the summary from one of them, and then applying
> 'xref-item and 'face properties appropriately). This retains the xref item
> semantics (as opposed to, say, associating an xref item with multiple
> locations). And _hopefully_ the replace-related code won't need
> any changes.

I tried to improve xref--insert-xrefs to group matches by lines
by using the most convenient function seq-group-by.  But then
noticed that xref.el doesn't rely on seq.el.  Even xref--alistify
that groups matches by files could be replaced by seq-group-by.
Is it a requirement to avoid using seq functions in xref.el?





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-03 21:30       ` Juri Linkov
@ 2020-12-03 23:44         ` Dmitry Gutov
  2020-12-06 20:31           ` Juri Linkov
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Gutov @ 2020-12-03 23:44 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 36967

On 03.12.2020 23:30, Juri Linkov wrote:
>>> Here is the patch that makes the broken
>>
>> Pretty harsh there.
> 
> But true: I can't use it in the current form, and was waiting
> when someone will fix it.

It's great to see you get this ball rolling.

>> Here's an alternative proposal:
>>
>> Combine the lines inside the rendering code instead.
>>
>> So each xref will have a separate location, but then xref--insert-xrefs
>> will see that xref-location-line value repeats across some consecutive
>> locations, and will combine them into single line with some text property
>> magic (basically, copying the summary from one of them, and then applying
>> 'xref-item and 'face properties appropriately). This retains the xref item
>> semantics (as opposed to, say, associating an xref item with multiple
>> locations). And _hopefully_ the replace-related code won't need
>> any changes.
> 
> I tried to improve xref--insert-xrefs to group matches by lines
> by using the most convenient function seq-group-by.  But then
> noticed that xref.el doesn't rely on seq.el.  Even xref--alistify
> that groups matches by files could be replaced by seq-group-by.
> Is it a requirement to avoid using seq functions in xref.el?

Not really, no. project.el pulls in seq already, why not have xref do it 
too.

I'm _slightly_ worried about extra garbage if we do seq-group-by twice 
(with an extra list for every line, even those that don't need it), but 
that's what benchmarking is for (can do that later).





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-03 23:44         ` Dmitry Gutov
@ 2020-12-06 20:31           ` Juri Linkov
  2020-12-09  3:53             ` Dmitry Gutov
  0 siblings, 1 reply; 10+ messages in thread
From: Juri Linkov @ 2020-12-06 20:31 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 36967

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

> I'm _slightly_ worried about extra garbage if we do seq-group-by twice
> (with an extra list for every line, even those that don't need it), but
> that's what benchmarking is for (can do that later).

I'm worried about extra garbage too, so this patch doesn't cause extra lists.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multiline-xref-insert-xrefs.patch --]
[-- Type: text/x-diff, Size: 2217 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 9f5fc57142..260623180e 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -832,17 +832,28 @@ xref--insert-xrefs
                                (length (and line (format "%d" line)))))
            for line-format = (and max-line-width
                                   (format "%%%dd: " max-line-width))
+           with prev-line-key = nil
            do
            (xref--insert-propertized '(face xref-file-header xref-group t)
                                      group "\n")
            (cl-loop for (xref . more2) on xrefs do
                     (with-slots (summary location) xref
                       (let* ((line (xref-location-line location))
+                             (line-key (list (xref-location-group location) line))
                              (prefix
                               (if line
                                   (propertize (format line-format line)
                                               'face 'xref-line-number)
                                 "  ")))
+                        (when (equal prev-line-key line-key)
+                          (let ((column (xref-file-location-column location)))
+                            (delete-region
+                             (save-excursion
+                               (forward-line -1)
+                               (move-to-column (+ (length prefix) column))
+                               (point))
+                             (point))
+                            (setq summary (substring summary column) prefix "")))
                         (xref--insert-propertized
                          (list 'xref-item xref
                                'mouse-face 'highlight
@@ -850,7 +861,8 @@ xref--insert-xrefs
                                'help-echo
                                (concat "mouse-2: display in another window, "
                                        "RET or mouse-1: follow reference"))
-                         prefix summary)))
+                         prefix summary)
+                        (setq prev-line-key line-key)))
                     (insert "\n"))))
 
 (defun xref--analyze (xrefs)

[-- Attachment #3: Type: text/plain, Size: 220 bytes --]


However, there is a problem: when xref--insert-xrefs is called by
'M-.' that uses etags, then it signals an error:

  cl-no-applicable-method xref-file-location-column

And I don't know how to fix etags to add columns.

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-06 20:31           ` Juri Linkov
@ 2020-12-09  3:53             ` Dmitry Gutov
  2020-12-09 13:57               ` Dmitry Gutov
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Gutov @ 2020-12-09  3:53 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 36967

On 06.12.2020 22:31, Juri Linkov wrote:
>> I'm _slightly_ worried about extra garbage if we do seq-group-by twice
>> (with an extra list for every line, even those that don't need it), but
>> that's what benchmarking is for (can do that later).
> 
> I'm worried about extra garbage too, so this patch doesn't cause extra lists.

Thanks! Seems to work well. Especially with this addition:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 9f5fc57142..9ad956d496 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -595,7 +595,8 @@ xref-prev-line

  (defun xref--item-at-point ()
    (save-excursion
-    (back-to-indentation)
+    (when (eolp)
+      (forward-char -1))
      (get-text-property (point) 'xref-item)))

  (defun xref-goto-xref (&optional quit)

> However, there is a problem: when xref--insert-xrefs is called by
> 'M-.' that uses etags, then it signals an error:

Does it happen when two symbols do indeed get defined on the same line? 
Sounds like a rare situation.

>    cl-no-applicable-method xref-file-location-column
> 
> And I don't know how to fix etags to add columns.

Using xref-file-location-column there kind of breaks the abstraction. I 
was thinking more along the lines of text-prop search for the column 
inside the summary string:

(setq column
       (with-temp-buffer
         (insert summary)
         (goto-char (point-min))
         (let ((match
                (text-property-search-forward
                 'face 'xref-match
                 (lambda (target value)
                   (or (eq target value)
                       (and (listp value)
                            (member target value)))))))
           (and match
                (1- (prop-match-beginning match))))))

That's quite a bit longer, though. And it still will work only with some 
kinds of xrefs: I'm not sure I know how to add columns to etags either. 
Not to mention third-party xref backends.

The rest won't get deduplication, but that's probably all right.





^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-09  3:53             ` Dmitry Gutov
@ 2020-12-09 13:57               ` Dmitry Gutov
  2020-12-21  1:42                 ` Dmitry Gutov
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Gutov @ 2020-12-09 13:57 UTC (permalink / raw)
  To: Juri Linkov; +Cc: 36967

On 09.12.2020 05:53, Dmitry Gutov wrote:
> Using xref-file-location-column there kind of breaks the abstraction. I 
> was thinking more along the lines of text-prop search for the column 
> inside the summary string:

Although we'll probably make it an optional method, just like 
xref-location-line is already.

Then we won't have to do all that gymnastics with the temp buffer.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#36967: 27.0.50; Duplicate lines in xref output
  2020-12-09 13:57               ` Dmitry Gutov
@ 2020-12-21  1:42                 ` Dmitry Gutov
  0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Gutov @ 2020-12-21  1:42 UTC (permalink / raw)
  To: Juri Linkov; +Cc: Pankaj Jangid, 36967-done

Version: 28.1

On 09.12.2020 15:57, Dmitry Gutov wrote:
> On 09.12.2020 05:53, Dmitry Gutov wrote:
>> Using xref-file-location-column there kind of breaks the abstraction. 
>> I was thinking more along the lines of text-prop search for the column 
>> inside the summary string:
> 
> Although we'll probably make it an optional method, just like 
> xref-location-line is already.

And now done.

I've pushed the slightly tweaked Juri's patch and the change described 
above.

With that, the bug should be fixed now. Please let me know if something 
is still amiss.

Thanks all!





^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-21  1:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-07 21:52 bug#36967: 27.0.50; Duplicate lines in xref output Juri Linkov
2019-08-07 22:13 ` Dmitry Gutov
2020-12-02 21:30   ` Juri Linkov
2020-12-03  1:35     ` Dmitry Gutov
2020-12-03 21:30       ` Juri Linkov
2020-12-03 23:44         ` Dmitry Gutov
2020-12-06 20:31           ` Juri Linkov
2020-12-09  3:53             ` Dmitry Gutov
2020-12-09 13:57               ` Dmitry Gutov
2020-12-21  1:42                 ` Dmitry Gutov

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).