bug#55234: 28.1; replace-string in rectangle regio

bug#55234: 28.1; replace-string in rectangle regio

[Paulo Sousa]

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

(read text between "```" in monospaced font)

I opened a rectangle region with `C-x SPC`. I traversed left and down
(i.e. the rectangle was started in the top-right corner). Then I
replaced region and not all the occurrences were replaced. Only the
occurrences between the starting and ending position of the cursor and
inside the region.

I replaced `]` by `+1]` as:

```
                   xx]xx*<Start of the region
                   xx]xx
End of the region>*xx]xx
```
and the result was
```
xx]xx
xx+1]xx
xx]xx
```
where I would expect it to be
```
xx+1]xx
xx+1]xx
xx+1]xx
```


In GNU Emacs 28.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.33,
cairo version 1.17.6)
 of 2022-04-20 built on frederik
Windowing system distributor 'The X.Org Foundation', version 11.0.12101003
System Description: Arch Linux

Configured using:
 'configure --sysconfdir=/etc --prefix=/usr --libexecdir=/usr/lib
 --localstatedir=/var --with-cairo --with-harfbuzz --with-modules
 --with-wide-int --with-x-toolkit=gtk3 --with-xft 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2
 -Wformat -Werror=format-security -fstack-clash-protection
 -fcf-protection -g -ffile-prefix-map=/build/emacs/src=/usr/src/debug
 -flto=auto' 'LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now
 -flto=auto''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE
XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: C++//l

Minor modes in effect:
  shell-dirtrack-mode: t
  smartparens-strict-mode: t
  smartparens-mode: t
  global-semantic-idle-scheduler-mode: t
  semantic-idle-scheduler-mode: t
  global-semanticdb-minor-mode: t
  semantic-mode: t
  delete-selection-mode: t
  global-display-line-numbers-mode: t
  display-line-numbers-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  abbrev-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug sendmail cl-print debug backtrace rect
xref shortdoc help-fns radix-tree vc-git diff-mode vc-dispatcher
flymake-cc flymake-proc flymake project warnings semantic/bovine/make
semantic/bovine/make-by make-mode misearch multi-isearch mhtml-mode
css-mode smie smartparens-javascript js smartparens-html sgml-mode
facemenu ox-odt rng-loc rng-uri rng-parse rng-match rng-dt rng-util
rng-pttrn nxml-parse nxml-ns nxml-enc xmltok nxml-util ox-latex
ox-icalendar org-agenda org-refile ox-html table ox-ascii ox-publish ox
org-element avl-tree generator ol-eww eww xdg url-queue mm-url ol-rmail
ol-mhe ol-irc ol-info ol-gnus nnselect gnus-search eieio-opt gnus-art
mm-uu mml2015 mm-view mml-smime smime dig gnus-sum shr kinsoku svg dom
gnus-group gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 netrc nnoo gnus-spec gnus-int gnus-range message
rmc puny rfc822 mml mml-sec epa derived epg rfc6068 epg-config mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
gnus-win gnus nnheader gnus-util rmail rmail-loaddefs rfc2047 rfc2045
ietf-drums mail-utils mm-util mail-prsvr wid-edit ol-docview doc-view
jka-compr image-mode exif dired dired-loaddefs ol-bibtex ol-bbdb ol-w3m
ol-doi org-link-doi smartparens-org org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-footnote org-src ob-comint org-pcomplete
org-list org-faces org-entities noutline outline easy-mmode org-version
ob-emacs-lisp ob-core ob-eval org-table oc-basic bibtex ol rx org-keys
oc org-compat org-macs org-loaddefs cal-menu calendar cal-loaddefs
cus-start cus-load semantic/tag-write novice dabbrev face-remap
semantic/bovine/c hideif semantic/bovine/c-by semantic/lex-spp
semantic/bovine/gcc semantic/bovine semantic/analyze/refs smartparens-c
cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine
cc-vars cc-defs semantic/imenu semantic/sb imenu semantic/edit
semantic/tag-file semantic/db-file data-debug cedet-files
semantic/wisent/python semantic/decorate/include semantic/db-find
semantic/db-ref compile text-property-search cl-extra semantic/dep
semantic/wisent/python-wy semantic/wisent semantic/wisent/wisent
smartparens-python python tramp-sh tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat shell pcomplete parse-time
iso8601 time-date ls-lisp format-spec comint ring ansi-color edmacro
kmacro smartparens-config smartparens-text smartparens advice help-mode
thingatpt dash semantic/decorate/mode semantic/decorate pulse color
semantic/idle semantic/analyze semantic/sort semantic/scope
semantic/analyze/fcn semantic/format semantic/tag-ls semantic/find
semantic/ctxt semantic/db-mode semantic/db eieio-base
semantic/util-modes semantic/util semantic pp semantic/tag semantic/lex
semantic/fw mode-local find-func cedet delsel display-line-numbers
speedbar ezimage dframe modus-vivendi-theme modus-themes pcase info
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib iso-transl tooltip eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 456867 46490)
 (symbols 48 38376 4)
 (strings 32 131759 4269)
 (string-bytes 1 4455167)
 (vectors 16 65136)
 (vector-slots 8 721867 71564)
 (floats 8 500 134)
 (intervals 56 2845 0)
 (buffers 992 33))

[-- Attachment #2: Type: text/html, Size: 7614 bytes --]

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

> I opened a rectangle region with `C-x SPC`. I traversed left and down
> (i.e. the rectangle was started in the top-right corner). Then I
> replaced region and not all the occurrences were replaced. Only the
> occurrences between the starting and ending position of the cursor and
> inside the region.

Thanks for the bug report.  Actually this is a more fundamental problem.
`replace-string` just uses `region-beginning` and `region-end`.
So the question is why `region-beginning` and `region-end`
don't return rectangular region boundaries.  The second question is
why `region-beginning` and `region-end` are implemented in C,
so it's more tricky to fix them to use rectangular region boundaries.

bug#55234: 28.1; replace-string in rectangle regio

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> So the question is why `region-beginning` and `region-end`
> don't return rectangular region boundaries.

This is implemented with `rectangle--mark-crutches', right?  Note that
these positions can be purely "virtual" (i.e., not exist in the buffer).

BTW, my solution to the original problem: I implemented a command
that lets me edit a rectangular region in a separate buffer (similar to
`string-edit').

Michael.

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

>> So the question is why `region-beginning` and `region-end`
>> don't return rectangular region boundaries.
>
> This is implemented with `rectangle--mark-crutches', right?  Note that
> these positions can be purely "virtual" (i.e., not exist in the buffer).

I thought that the implementation of region-beginning could be:

  (mapcar 'car (region-bounds))

or better for backward-compatibility:

  (caar (region-bounds))

and region-end:

  (cdar (last (region-bounds)))

Please try these on the original bug report - they completely fix the problem.

> BTW, my solution to the original problem: I implemented a command
> that lets me edit a rectangular region in a separate buffer (similar to
> `string-edit').

Isn't this a workaround?

PS: I'll reply to your other posts after more testing of search/replace problems.

bug#55234: 28.1; replace-string in rectangle regio

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

>   (mapcar 'car (region-bounds))

Sounds reasonable.

> > BTW, my solution to the original problem: I implemented a command
> > that lets me edit a rectangular region in a separate buffer (similar to
> > `string-edit').
>
> Isn't this a workaround?

A different approach.  Why I did this is: first, it's sometimes nice to
have a separate buffer that is like the buffer "narrowed to the
rectangular region".  Second, depending on the text structure, in some
cases the end result should be a proper rectangle again, or even a
rectangle of exactly the same dimensions.  But when you query replace,
rectangle lines may become different lengths, some may be empty, some
even longer than before etc.  My commands asks me what to do in such a
situation.

Michael.

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

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

>>   (mapcar 'car (region-bounds))
>
> Sounds reasonable.

Then here is the implementation that is more less in line with
how other rectangular functions are implemented:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rectangle--region-beginning.patch --]
[-- Type: text/x-diff, Size: 1698 bytes --]

diff --git a/lisp/rect.el b/lisp/rect.el
index 15d636f074..cb545f473a 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -754,17 +754,42 @@ rectangle-previous-line
     (rectangle--col-pos col 'point)))
 
 
+(defun rectangle--region-beginning (orig)
+  "Like `region-beginning' but supports rectangular regions."
+  (cond
+   ((not rectangle-mark-mode)
+    (funcall orig))
+   (t
+    (caar (region-bounds)))))
+
+(advice-add 'region-beginning :around #'rectangle--region-beginning)
+
+(defun rectangle--region-end (orig)
+  "Like `region-end' but supports rectangular regions."
+  (cond
+   ((not rectangle-mark-mode)
+    (funcall orig))
+   (t
+    (cdar (last (region-bounds))))))
+
+(advice-add 'region-end :around #'rectangle--region-end)
+
 (defun rectangle--extract-region (orig &optional delete)
   (cond
    ((not rectangle-mark-mode)
     (funcall orig delete))
    ((eq delete 'bounds)
-    (extract-rectangle-bounds (region-beginning) (region-end)))
+    (extract-rectangle-bounds
+     ;; Avoid recursive calls
+     (let (rectangle-mark-mode) (region-beginning))
+     (let (rectangle-mark-mode) (region-end))))
    (t
     (let* ((strs (funcall (if delete
                               #'delete-extract-rectangle
                             #'extract-rectangle)
-                          (region-beginning) (region-end)))
+                          ;; Avoid recursive calls
+                          (let (rectangle-mark-mode) (region-beginning))
+                          (let (rectangle-mark-mode) (region-end))))
            (str (mapconcat #'identity strs "\n")))
       (when (eq last-command 'kill-region)
         ;; Try to prevent kill-region from appending this to some

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

>> > BTW, my solution to the original problem: I implemented a command
>> > that lets me edit a rectangular region in a separate buffer (similar to
>> > `string-edit').
>>
>> Isn't this a workaround?
>
> A different approach.  Why I did this is: first, it's sometimes nice to
> have a separate buffer that is like the buffer "narrowed to the
> rectangular region".  Second, depending on the text structure, in some
> cases the end result should be a proper rectangle again, or even a
> rectangle of exactly the same dimensions.  But when you query replace,
> rectangle lines may become different lengths, some may be empty, some
> even longer than before etc.  My commands asks me what to do in such a
> situation.

Does this approach have more side-effects?  It seems e.g. the undo history
of replacements is not maintained: when copied back from the temporary buffer
to the original buffer, the undo command will undo the whole rectangular region,
not each replacement?

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

>>>   (mapcar 'car (region-bounds))
>>
>> Sounds reasonable.
>
> Then here is the implementation that is more less in line with
> how other rectangular functions are implemented:
>
> diff --git a/lisp/rect.el b/lisp/rect.el
> +(advice-add 'region-beginning :around #'rectangle--region-beginning)

This causes a strange compilation error:

  Loading lisp/mouse.el (source)...
  Eager macro-expansion failure: (void-function easy-menu-define)
  Eager macro-expansion failure: (void-function easy-menu-define)

  Error: void-function (easy-menu-define)
    debug-early(error (void-function easy-menu-define))
    (easy-menu-define help-mode-menu help-mode-map "Menu for Help mode." ...)
    require(help-mode)
    require(cl-extra)
    require(comp)
    advice--add-function(:around ... rectangle--region-beginning nil)
    advice-add(region-beginning :around rectangle--region-beginning)
    require(rect)
    load("mouse")
    load("loadup.el")

  Symbol's function definition is void: easy-menu-define

bug#55234: 28.1; replace-string in rectangle regio

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Does this approach have more side-effects?  It seems e.g. the undo
> history of replacements is not maintained: when copied back from the
> temporary buffer to the original buffer, the undo command will undo
> the whole rectangular region, not each replacement?

Yes, that's how it works.

Michael.

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

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

>>>>   (mapcar 'car (region-bounds))
>>>
>>> Sounds reasonable.
>>
>> Then here is the implementation that is more less in line with
>> how other rectangular functions are implemented:
>>
>> diff --git a/lisp/rect.el b/lisp/rect.el
>> +(advice-add 'region-beginning :around #'rectangle--region-beginning)
>
> This causes a strange compilation error:
>   Symbol's function definition is void: easy-menu-define

This patch avoids such problem:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rectangle--region-beginning.patch --]
[-- Type: text/x-diff, Size: 1939 bytes --]

diff --git a/lisp/rect.el b/lisp/rect.el
index 15d636f074..334c6f59fd 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -656,6 +656,8 @@ rectangle-mark-mode
   :lighter nil
   (rectangle--reset-crutches)
   (when rectangle-mark-mode
+    (advice-add 'region-beginning :around #'rectangle--region-beginning)
+    (advice-add 'region-end :around #'rectangle--region-end)
     (add-hook 'deactivate-mark-hook
               (lambda () (rectangle-mark-mode -1)))
     (unless (region-active-p)
@@ -754,17 +756,38 @@ rectangle-previous-line
     (rectangle--col-pos col 'point)))
 
 
+(defun rectangle--region-beginning (orig)
+  "Like `region-beginning' but supports rectangular regions."
+  (cond
+   ((not rectangle-mark-mode)
+    (funcall orig))
+   (t
+    (caar (region-bounds)))))
+
+(defun rectangle--region-end (orig)
+  "Like `region-end' but supports rectangular regions."
+  (cond
+   ((not rectangle-mark-mode)
+    (funcall orig))
+   (t
+    (cdar (last (region-bounds))))))
+
 (defun rectangle--extract-region (orig &optional delete)
   (cond
    ((not rectangle-mark-mode)
     (funcall orig delete))
    ((eq delete 'bounds)
-    (extract-rectangle-bounds (region-beginning) (region-end)))
+    (extract-rectangle-bounds
+     ;; Avoid recursive calls
+     (let (rectangle-mark-mode) (region-beginning))
+     (let (rectangle-mark-mode) (region-end))))
    (t
     (let* ((strs (funcall (if delete
                               #'delete-extract-rectangle
                             #'extract-rectangle)
-                          (region-beginning) (region-end)))
+                          ;; Avoid recursive calls
+                          (let (rectangle-mark-mode) (region-beginning))
+                          (let (rectangle-mark-mode) (region-end))))
            (str (mapconcat #'identity strs "\n")))
       (when (eq last-command 'kill-region)
         ;; Try to prevent kill-region from appending this to some

bug#55234: 28.1; replace-string in rectangle regio

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

>  (defun rectangle--extract-region (orig &optional delete)
>    (cond
>     ((not rectangle-mark-mode)
>      (funcall orig delete))
>     ((eq delete 'bounds)
> -    (extract-rectangle-bounds (region-beginning) (region-end)))
> +    (extract-rectangle-bounds
> +     ;; Avoid recursive calls
> +     (let (rectangle-mark-mode) (region-beginning))
> +     (let (rectangle-mark-mode) (region-end))))
>     (t
>      (let* ((strs (funcall (if delete
>                                #'delete-extract-rectangle
>                              #'extract-rectangle)
> -                          (region-beginning) (region-end)))
> +                          ;; Avoid recursive calls
> +                          (let (rectangle-mark-mode) (region-beginning))
> +                          (let (rectangle-mark-mode) (region-end))))
>             (str (mapconcat #'identity strs "\n")))
>        (when (eq last-command 'kill-region)
>          ;; Try to prevent kill-region from appending this to some

Is it intended to - at the end - call the original functions here?

TIA,

Michael.

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

>>  (defun rectangle--extract-region (orig &optional delete)
>>   ...
>>      (let* ((strs (funcall (if delete
>>                                #'delete-extract-rectangle
>>                              #'extract-rectangle)
>> -                          (region-beginning) (region-end)))
>> +                          ;; Avoid recursive calls
>> +                          (let (rectangle-mark-mode) (region-beginning))
>> +                          (let (rectangle-mark-mode) (region-end))))
>>             (str (mapconcat #'identity strs "\n")))
>>        (when (eq last-command 'kill-region)
>>          ;; Try to prevent kill-region from appending this to some
>
> Is it intended to - at the end - call the original functions here?

Wouldn't it go to infinite recursion when the advice on region-beginning
will call region-bounds that calls rectangle--extract-region again?

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

close 55234 29.0.50
thanks

> I opened a rectangle region with `C-x SPC`. I traversed left and down
> (i.e. the rectangle was started in the top-right corner). Then I
> replaced region and not all the occurrences were replaced. Only the
> occurrences between the starting and ending position of the cursor and
> inside the region.

This is fixed now in 29.0.

bug#55234: 28.1; replace-string in rectangle regio

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Wouldn't it go to infinite recursion when the advice on
> region-beginning will call region-bounds that calls
> rectangle--extract-region again?

I must admit that I haven't checked when which calls are made (and there
are a lot).  But I would expect that a binding to avoid the infinite
recursion would be made in the advice itself, not outside.

Michael.

bug#55234: 28.1; replace-string in rectangle regio

[Juri Linkov]

>> Wouldn't it go to infinite recursion when the advice on
>> region-beginning will call region-bounds that calls
>> rectangle--extract-region again?
>
> I must admit that I haven't checked when which calls are made (and there
> are a lot).  But I would expect that a binding to avoid the infinite
> recursion would be made in the advice itself, not outside.

The advice needs to get rectangular region boundaries
using rectangle--extract-region.