bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

[Karthik Chikmagalur]

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

Hello,

The function "reb-update-overlays" (in lisp/emacs-lisp/re-builder.el)
displays matches in the whole of reb-target-buffer starting at
(point-min) even when the region is active. In keeping with the behavior
of commands like query-replace, replace-regexp and query-replace-regexp,
I modified it so that when the region is active re-builder only displays
matches in the active region. It's behavior is unchanged when the region is
inactive.

I think this change makes sense without disrupting the expectations of
re-builder users.

More generally, I think re-builder needs some work:

1. When the user quits re-builder, the point in reb-target-buffer is not
restored correctly.

2. Without an active region, there should be an option to match forward
or backward from reb-target-buffer's point instead of always matching
from (point-min), with the ability to customize the default behavior.

3. re-builder's overlay system (or some modification of it) can be used
by query-replace-regexp to show matches as the user types.

I am waiting to sign the copyright papers from FSF (which I have
applied for) before sending in these larger patches.

Commit log entry:

* lisp/emacs-lisp/re-builder.el (reb-update-overlays): Restrict
  re-builder matches to region when the region is active.
  
Karthik Chikmagalur

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

diff -u emacs-src/lisp/emacs-lisp/re-builder.el emacs-src/lisp/emacs-lisp/re-builder-new.el
--- /lisp/emacs-lisp/re-builder.el	2021-08-02 20:47:39.226669281 -0700
+++ /lisp/emacs-lisp/re-builder-new.el	2021-08-02 20:37:27.442020958 -0700
@@ -642,16 +642,19 @@
 	 (submatches 0)
 	 firstmatch
          here
+         start end
          firstmatch-after-here)
     (with-current-buffer reb-target-buffer
         (setq here
               (if reb-target-window
                   (with-selected-window reb-target-window (window-point))
-                (point)))
+                (point))
+              start (if (region-active-p) (region-beginning) (point-min))
+              end   (if (region-active-p) (region-end) (point-max)))
       (reb-delete-overlays)
-      (goto-char (point-min))
+      (goto-char start)
       (while (and (not (eobp))
-		  (re-search-forward re (point-max) t)
+		  (re-search-forward re end t)
 		  (or (not reb-auto-match-limit)
 		      (< matches reb-auto-match-limit)))
 	(when (and (= 0 (length (match-string 0)))

Diff finished.  Mon Aug  2 20:48:01 2021

bug#49842: re-builder restriction to region

[Karthik Chikmagalur]

Hello,

Please disregard this patch. With further testing this does not behave as expected.

re-builder moves the point around in reb-target-buffer with each search, so (region-beginning) has a different meaning every time reb-update-overlays is called. As a result, this fails when, for example, we type in a regexp (in the re-builder buffer), then delete it and type in a new regexp. The new regexp is matched from the location of the first match of the old regexp instead of from the beginning of the region as originally specified by the user.

IIRC, the right way to restrict this to the region would be to define a variable to hold the bounds of the region as specified by the user before starting re-builder and re-search-forward inside those bounds. Perhaps a closure that's tied to the specific re-builder session can be used to avoid issues with the value of this variable when running multiple simultaneous re-builder sessions.

Karthik

bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

[Lars Ingebrigtsen]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> IIRC, the right way to restrict this to the region would be to define
> a variable to hold the bounds of the region as specified by the user
> before starting re-builder and re-search-forward inside those
> bounds. Perhaps a closure that's tied to the specific re-builder
> session can be used to avoid issues with the value of this variable
> when running multiple simultaneous re-builder sessions.

I'm not very familiar with re-builder.el myself, but I think that sounds
like a promising approach.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

[Karthik Chikmagalur]

I'm working on a few improvements to re-builder including the above, and need to store some state information (bounds of region, etc) that persists until the re-builder session is closed. I see two ways to do this:

1. With a buffer-local variable in reb-target-buffer, with the assumption that only one re-builder session can be run per buffer.

2. With a lexical closure in the re-builder update code.

I'm not very familiar with elisp best practices, is there a reason to prefer one over the other beyond the direct effect on the re-builder code logic/complexity?

Karthik

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:
>
>> IIRC, the right way to restrict this to the region would be to define
>> a variable to hold the bounds of the region as specified by the user
>> before starting re-builder and re-search-forward inside those
>> bounds. Perhaps a closure that's tied to the specific re-builder
>> session can be used to avoid issues with the value of this variable
>> when running multiple simultaneous re-builder sessions.
>
> I'm not very familiar with re-builder.el myself, but I think that sounds
> like a promising approach.
>
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

[Lars Ingebrigtsen]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> I'm working on a few improvements to re-builder including the above,
> and need to store some state information (bounds of region, etc) that
> persists until the re-builder session is closed. I see two ways to do
> this:
>
> 1. With a buffer-local variable in reb-target-buffer, with the
> assumption that only one re-builder session can be run per buffer.
>
> 2. With a lexical closure in the re-builder update code.
>
> I'm not very familiar with elisp best practices, is there a reason to
> prefer one over the other beyond the direct effect on the re-builder
> code logic/complexity?

Traditionally, Emacs Lisp didn't have lexical binding, so people usually
stashed the data in buffer-local values.  Now that all of the in-tree
code uses lexical binding, I think using closures usually gives more
readable and maintainable code.  But it's up to the person implementing
it what they prefer, or what makes most sense to them in that particular
case, really.  (Sometimes using buffer-local values is much more
convenient, and sometimes closures are.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

[Lars Ingebrigtsen]

Karthik Chikmagalur <karthikchikmagalur@gmail.com> writes:

> I'm working on a few improvements to re-builder including the above,
> and need to store some state information (bounds of region, etc) that
> persists until the re-builder session is closed.

This was a year ago -- did you make any further progress here?

bug#49842: re-builder restriction to region (lisp/emacs-lisp/re-builder)

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> I'm working on a few improvements to re-builder including the above,
>> and need to store some state information (bounds of region, etc) that
>> persists until the re-builder session is closed.
>
> This was a year ago -- did you make any further progress here?

And this was a month ago, so I'm guessing there isn't going to be any
further progress in this bug report, so I'm closing it.  If further
progress can be made, please respond to the debbugs address and we'll
reopen.