bug#68185: [PATCH] Ensure indent-region argument order is correct

bug#68185: [PATCH] Ensure indent-region argument order is correct

[Morgan Willcock]

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

Tags: patch

The attached patch fixes an issue with Tempo template insertion, where
the region start is assumed to be a position before the region end.  If
the user selects a region in a backwards direction the correct
indentation will not be applied:

  (require 'tempo)
  (tempo-define-template "indent-region-test" '(r>))

  (with-temp-buffer
    (emacs-lisp-mode)
    (insert "    one\n    two\n    three")
    (goto-char (point-min))
    (set-mark-command nil)
    (goto-char (point-max))
    (tempo-template-indent-region-test)
    (buffer-string))

  ;; => "one\ntwo\nthree"

  (with-temp-buffer
    (emacs-lisp-mode)
    (insert "    one\n    two\n    three")
    (goto-char (point-max))
    (set-mark-command nil)
    (goto-char (point-min))
    (tempo-template-indent-region-test)
    (buffer-string))

  ;; => "    one\n    two\nthree"

The start and end positions are already identified and stored using
separate variables, so the patch just uses these values instead of
assuming the point and mark order.  I've also removed passing nil as the
third argument since the argument is optional.

In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-07-31 built on inspiron
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --with-native-compilation --with-cairo --with-json
 --with-xml2 --with-x-toolkit=lucid'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-indent-region-argument-order-is-correct.patch --]
[-- Type: text/patch, Size: 1438 bytes --]

From 4d7fdafeee5f6540d5e39549fa0a9927176171bb Mon Sep 17 00:00:00 2001
From: Morgan Willcock <morgan@ice9.digital>
Date: Sun, 31 Dec 2023 20:47:17 +0000
Subject: [PATCH] Ensure indent-region argument order is correct

* lisp/tempo.el (tempo-insert): Call indent-region with the stored
region markers to ensure that the start and end arguments are used in
the correct order.
---
 lisp/tempo.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/tempo.el b/lisp/tempo.el
index 45dc24dcc97..1b6cc4476e1 100644
--- a/lisp/tempo.el
+++ b/lisp/tempo.el
@@ -333,7 +333,8 @@ tempo-insert
     (`(r> . ,rest) (if on-region
                        (progn
                          (goto-char tempo-region-stop)
-                         (indent-region (mark) (point) nil))
+                         (indent-region tempo-region-start
+                                        tempo-region-stop))
                        (tempo-insert-prompt-compat rest)))
     (`(s ,name) (tempo-insert-named name))
     (`(l . ,rest) (dolist (elt rest) (tempo-insert elt on-region)))
@@ -344,7 +345,7 @@ tempo-insert
     ('r> (if on-region
 	     (progn
 	       (goto-char tempo-region-stop)
-	       (indent-region (mark) (point) nil))
+	       (indent-region tempo-region-start tempo-region-stop))
 	   (tempo-insert-mark (point-marker))))
     ('> (indent-according-to-mode))
     ('& (if (not (or (= (current-column) 0)
-- 
2.39.2

bug#68185: [PATCH] Ensure indent-region argument order is correct

[Morgan Willcock]

Apologies, I think the fix is too simplistic and does not account for
the region being shifted.  The existing implementation does account for
that for also assumes that the region was selected in a forwards
direction.

Is it possible to convert this into a normal bug report rather than a
patch request?

bug#68185: [PATCH] Ensure indent-region argument order is correct

[Morgan Willcock]

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

Attached is a replacement patch which I believe fixes the issue with the
region selection direction and with indentation potentially being
incorrectly applied to text that is inserted by the template.

Previously when text was being inserted at the region start it was
actually going into region, which means that it will have been possible
for template contents to have accidentally been indented along with the
region that the user selected.  I've changed the marker insertion type
for the start boundary, which should preserve the original indentation
of text which was inserted by the template, i.e. if text inserted by the
template should be indented this is now back under control of the
template rather than applied by accident.

The changes do mean that the indentation of existing templates may
change, but I think this will only be for templates which accidentally
omitted the indentation directives because indentation was mistakenly
applied as part of the region.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-indent-region-arguments-and-boundaries-are-co.patch --]
[-- Type: text/x-diff, Size: 2050 bytes --]

From e667cd5abbdc7788726ec0da29b95b24a44a7923 Mon Sep 17 00:00:00 2001
From: Morgan Willcock <morgan@ice9.digital>
Date: Sun, 31 Dec 2023 20:47:17 +0000
Subject: [PATCH] Ensure indent-region arguments and boundaries are correct

* lisp/tempo.el: Set marker type for tempo-region-start to move when
  text is inserted at its position.  This prevents the template from
  inserting text into the region.

* lisp/tempo.el (tempo-insert): Call indent-region with the stored
region markers to ensure that the start and end arguments are used in
the correct order.
---
 lisp/tempo.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/tempo.el b/lisp/tempo.el
index 45dc24dcc97..56f8a8389a2 100644
--- a/lisp/tempo.el
+++ b/lisp/tempo.el
@@ -197,6 +197,9 @@ tempo-named-insertions
 
 (defvar-local tempo-region-start (make-marker)
   "Region start when inserting around the region.")
+;; Insertion by the template at the region start position should move
+;; the marker to preserve the original region contents.
+(set-marker-insertion-type tempo-region-start t)
 
 (defvar-local tempo-region-stop (make-marker)
   "Region stop when inserting around the region.")
@@ -333,7 +336,8 @@ tempo-insert
     (`(r> . ,rest) (if on-region
                        (progn
                          (goto-char tempo-region-stop)
-                         (indent-region (mark) (point) nil))
+                         (indent-region tempo-region-start
+                                        tempo-region-stop))
                        (tempo-insert-prompt-compat rest)))
     (`(s ,name) (tempo-insert-named name))
     (`(l . ,rest) (dolist (elt rest) (tempo-insert elt on-region)))
@@ -344,7 +348,7 @@ tempo-insert
     ('r> (if on-region
 	     (progn
 	       (goto-char tempo-region-stop)
-	       (indent-region (mark) (point) nil))
+	       (indent-region tempo-region-start tempo-region-stop))
 	   (tempo-insert-mark (point-marker))))
     ('> (indent-according-to-mode))
     ('& (if (not (or (= (current-column) 0)
-- 
2.39.2

bug#68185: [PATCH] Ensure indent-region argument order is correct

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Date: Sun, 31 Dec 2023 21:20:11 +0000
> 
> The attached patch fixes an issue with Tempo template insertion, where
> the region start is assumed to be a position before the region end.  If
> the user selects a region in a backwards direction the correct
> indentation will not be applied:

Thanks, installed on the master branch, and closing the bug.

bug#68185: [PATCH] Ensure indent-region argument order is correct

[Eli Zaretskii]

> From: Morgan Willcock <morgan@ice9.digital>
> Date: Mon, 01 Jan 2024 17:04:30 +0000
> 
> Attached is a replacement patch which I believe fixes the issue with the
> region selection direction and with indentation potentially being
> incorrectly applied to text that is inserted by the template.
> 
> Previously when text was being inserted at the region start it was
> actually going into region, which means that it will have been possible
> for template contents to have accidentally been indented along with the
> region that the user selected.  I've changed the marker insertion type
> for the start boundary, which should preserve the original indentation
> of text which was inserted by the template, i.e. if text inserted by the
> template should be indented this is now back under control of the
> template rather than applied by accident.
> 
> The changes do mean that the indentation of existing templates may
> change, but I think this will only be for templates which accidentally
> omitted the indentation directives because indentation was mistakenly
> applied as part of the region.

Thanks, installed (the additional part which wasn't in the previous
patch).