bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Severity: wishlist

When you use xref-query-replace-in-results to replace some references,
it'd be great if the xref buffer showed an overlay arrow (see
xref--set-arrow) pointing to the reference being edited.  That'd give
better context about the reference that is being edited and the global
progress of the query replace operation.

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Dmitry Gutov]

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

On 23/12/2022 21:16, Daniel Martín via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Severity: wishlist
> 
> When you use xref-query-replace-in-results to replace some references,
> it'd be great if the xref buffer showed an overlay arrow (see
> xref--set-arrow) pointing to the reference being edited.  That'd give
> better context about the reference that is being edited and the global
> progress of the query replace operation.

SGTM.

Here's a patch which seems to work. It's a little tricky, though, and 
I'm at a loss with better variable names in some places.

Wish we had some unit tests here.

[-- Attachment #2: xref-replace-with-arrow.diff --]
[-- Type: text/x-patch, Size: 4256 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 0213ab3cc58..49ad3500d15 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -771,19 +771,22 @@ xref-query-replace-in-results
                       (format "Query-replace (regexp) %s with: " fr)
                     "Query-replace all matches with: ")))
      (list fr (read-regexp prompt))))
-  (let* (item xrefs iter)
+  (let* ((positions (make-hash-table :test 'eq))
+         item xrefs iter)
     (save-excursion
       (while (setq item (xref--search-property 'xref-item))
         (when (xref-match-length item)
+          (puthash item (point) positions)
           (push item xrefs))))
     (unwind-protect
         (progn
           (goto-char (point-min))
-          (setq iter (xref--buf-pairs-iterator (nreverse xrefs)))
-          (xref--query-replace-1 from to iter))
+          (setq iter (xref--buf-pairs-iterator (nreverse xrefs)
+                                               positions))
+          (xref--query-replace-1 from to iter (current-buffer)))
       (funcall iter :cleanup))))
 
-(defun xref--buf-pairs-iterator (xrefs)
+(defun xref--buf-pairs-iterator (xrefs positions)
   (let (chunk-done item next-pair file-buf pairs all-pairs)
     (lambda (action)
       (pcase action
@@ -801,9 +804,10 @@ xref--buf-pairs-iterator
                       (beg (xref-location-marker loc))
                       (end (move-marker (make-marker)
                                         (+ beg (xref-match-length item))
-                                        (marker-buffer beg))))
-                 (let ((pair (cons beg end)))
-                   (push pair all-pairs)
+                                        (marker-buffer beg)))
+                      (xref-buf-pos (gethash item positions)))
+                 (let ((positions (list beg end xref-buf-pos)))
+                   (push positions all-pairs)
                    ;; Perform sanity check first.
                    (xref--goto-location loc)
                    (if (xref--outdated-p item)
@@ -811,17 +815,17 @@ xref--buf-pairs-iterator
                      (cond
                       ((null file-buf)
                        (setq file-buf (marker-buffer beg))
-                       (push pair pairs))
+                       (push positions pairs))
                       ((equal file-buf (marker-buffer beg))
-                       (push pair pairs))
+                       (push positions pairs))
                       (t
                        (setq chunk-done t
-                             next-pair pair))))))))
+                             next-pair positions))))))))
            (cons file-buf (nreverse pairs))))
         (:cleanup
          (dolist (pair all-pairs)
-           (move-marker (car pair) nil)
-           (move-marker (cdr pair) nil)))))))
+           (move-marker (nth 0 pair) nil)
+           (move-marker (nth 1 pair) nil)))))))
 
 (defun xref--outdated-p (item)
   "Check that the match location at current position is up-to-date.
@@ -857,7 +861,7 @@ xref--outdated-p
              (funcall check)))))))
 
 ;; FIXME: Write a nicer UI.
-(defun xref--query-replace-1 (from to iter)
+(defun xref--query-replace-1 (from to iter xref-buf)
   (let* ((query-replace-lazy-highlight nil)
          (continue t)
          did-it-once buf-pairs pairs
@@ -872,11 +876,15 @@ xref--query-replace-1
                  (<= end current-end))))
          (replace-re-search-function
           (lambda (from &optional _bound noerror)
-            (let (found pair)
+            (let (found positions)
               (while (and (not found) pairs)
-                (setq pair (pop pairs)
-                      current-beg (car pair)
-                      current-end (cdr pair))
+                (setq positions (pop pairs)
+                      current-beg (nth 0 positions)
+                      current-end (nth 1 positions))
+                (when (buffer-live-p xref-buf)
+                  (with-current-buffer xref-buf
+                    (goto-char (nth 2 positions))
+                    (xref--set-arrow)))
                 (goto-char current-beg)
                 (when (re-search-forward from current-end noerror)
                   (setq found t)))

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 23/12/2022 21:16, Daniel Martín via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> Severity: wishlist
>> When you use xref-query-replace-in-results to replace some
>> references,
>> it'd be great if the xref buffer showed an overlay arrow (see
>> xref--set-arrow) pointing to the reference being edited.  That'd give
>> better context about the reference that is being edited and the global
>> progress of the query replace operation.
>
> SGTM.
>
> Here's a patch which seems to work. It's a little tricky, though, and
> I'm at a loss with better variable names in some places.
>
> Wish we had some unit tests here.

Thanks.  It works well, but there are some corner cases.  For example,
as you can undo or go to the previous match during the query replace,
the arrow should point to the previous result, if possible.

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Dmitry Gutov]

On 01/01/2023 13:23, Daniel Martín wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> On 23/12/2022 21:16, Daniel Martín via Bug reports for GNU Emacs, the
>> Swiss army knife of text editors wrote:
>>> Severity: wishlist
>>> When you use xref-query-replace-in-results to replace some
>>> references,
>>> it'd be great if the xref buffer showed an overlay arrow (see
>>> xref--set-arrow) pointing to the reference being edited.  That'd give
>>> better context about the reference that is being edited and the global
>>> progress of the query replace operation.
>> SGTM.
>>
>> Here's a patch which seems to work. It's a little tricky, though, and
>> I'm at a loss with better variable names in some places.
>>
>> Wish we had some unit tests here.
> Thanks.  It works well, but there are some corner cases.

Thanks for testing.

> For example,
> as you can undo or go to the previous match during the query replace,
> the arrow should point to the previous result, if possible.

Hmm, I'm not sure how to implement that. Some new hook inside 
perform-replace?

Maybe Juri has some ideas.

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Juri Linkov]

>> For example,
>> as you can undo or go to the previous match during the query replace,
>> the arrow should point to the previous result, if possible.
>
> Hmm, I'm not sure how to implement that. Some new hook inside
> perform-replace?
>
> Maybe Juri has some ideas.

Indeed, there is a special hook for such cases:
'replace-update-post-hook'.

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Dmitry Gutov]

On 05/01/2023 19:55, Juri Linkov wrote:
>>> For example,
>>> as you can undo or go to the previous match during the query replace,
>>> the arrow should point to the previous result, if possible.
>> Hmm, I'm not sure how to implement that. Some new hook inside
>> perform-replace?
>>
>> Maybe Juri has some ideas.
> Indeed, there is a special hook for such cases:
> 'replace-update-post-hook'.

That one seems problematic. First of all, I think (?) that we need to 
update the arrow's position when the replacement prompt appears, not 
after a replacement.

Second, it fires in all cases -- whether the replacement is going 
forward, or it being undone, is that right? I suppose this will mean 
we'll need to maintain a reverse mapping: from the markers back to the 
Xref buffer lines. Hopefully they will be in expected position after the 
"undo" actions too.

bug#60285: xref-query-replace-in-results could show an arrow pointing to the current item being replaced

[Juri Linkov]

>> Indeed, there is a special hook for such cases:
>> 'replace-update-post-hook'.
>
> That one seems problematic. First of all, I think (?) that we need to
> update the arrow's position when the replacement prompt appears, not after
> a replacement.

Maybe another pre-hook is needed that will run before the prompt?

> Second, it fires in all cases -- whether the replacement is going forward,
> or it being undone, is that right? I suppose this will mean we'll need to
> maintain a reverse mapping: from the markers back to the Xref buffer
> lines. Hopefully they will be in expected position after the "undo" actions
> too.

This needs more experimentation to see how it works.