bug#30343: make transpose-regions interactive

bug#30343: make transpose-regions interactive

[Charles A. Roelli]

Package: emacs
Version: 27.0.50

bug#30343: make transpose-regions interactive

[Charles A. Roelli]

What follows is a patch to make transpose-regions interactive.

I also noticed that transpose-sentences/paragraphs are not documented,
so I'll add those in another commit.


From 8468059f55ea459dc56c3dae6c6667950c6767d2 Mon Sep 17 00:00:00 2001
From: "Charles A. Roelli" <charles@aurox.ch>
Date: Sun, 4 Feb 2018 10:41:00 +0100
Subject: [PATCH] Make transpose-regions interactive (Bug#30343)

* doc/emacs/fixit.texi (Transpose): Mention and explain the new
command.
* editfns.c (Ftranspose_regions): Add an interactive calling
specification, and add documentation for it.
---
 doc/emacs/fixit.texi | 12 +++++++++++-
 src/editfns.c        | 11 +++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/doc/emacs/fixit.texi b/doc/emacs/fixit.texi
index ced1ef9..a35ef6b 100644
--- a/doc/emacs/fixit.texi
+++ b/doc/emacs/fixit.texi
@@ -146,6 +146,8 @@ Transpose
 Transpose two balanced expressions (@code{transpose-sexps}).
 @item C-x C-t
 Transpose two lines (@code{transpose-lines}).
+@item M-x transpose-regions
+Transpose two regions.
 @end table
 
 @kindex C-t
@@ -183,7 +185,7 @@ Transpose
 (@code{transpose-lines}) exchanges lines.  They work like @kbd{M-t}
 except as regards what units of text they transpose.
 
-  A numeric argument to a transpose command serves as a repeat count: it
+  A numeric argument to most transpose commands serves as a repeat count: it
 tells the transpose command to move the character (word, expression, line)
 before or containing point across several other characters (words,
 expressions, lines).  For example, @kbd{C-u 3 C-t} moves the character before
@@ -198,6 +200,14 @@ Transpose
 transpose the character (word, expression, line) ending after point
 with the one ending after the mark.
 
+@findex transpose-regions
+  @kbd{M-x transpose-regions} transposes the text between point and
+mark with the text between the first two elements of the mark ring.
+Unlike the other transpose commands, it does not behave differently
+when given a prefix argument.  This command is best used for
+transposing multiple units of text (words, sentences, paragraphs) in
+one go.
+
 @node Fixing Case
 @section Case Conversion
 
diff --git a/src/editfns.c b/src/editfns.c
index 96bb271..66dd2a4 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -5112,7 +5112,10 @@ transpose_markers (ptrdiff_t start1, ptrdiff_t end1,
     }
 }
 
-DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
+DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
+       "(if (< (length mark-ring) 2)\
+            (error \"Mark ring must contain at least two elements\")\
+          (list (point) (mark) (car mark-ring) (cadr mark-ring)))",
        doc: /* Transpose region STARTR1 to ENDR1 with STARTR2 to ENDR2.
 The regions should not be overlapping, because the size of the buffer is
 never changed in a transposition.
@@ -5120,7 +5123,11 @@ never changed in a transposition.
 Optional fifth arg LEAVE-MARKERS, if non-nil, means don't update
 any markers that happen to be located in the regions.
 
-Transposing beyond buffer boundaries is an error.  */)
+Transposing beyond buffer boundaries is an error.
+
+Interactively, STARTR1 and ENDR1 are point and mark.  STARTR2 and
+ENDR2 are the first and second markers in the mark ring.
+LEAVE-MARKERS is nil.  */)
   (Lisp_Object startr1, Lisp_Object endr1, Lisp_Object startr2, Lisp_Object endr2, Lisp_Object leave_markers)
 {
   register ptrdiff_t start1, end1, start2, end2;
-- 
2.9.4

bug#30343: make transpose-regions interactive

[Eli Zaretskii]

> Date: Sun, 04 Feb 2018 10:45:45 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> 
> What follows is a patch to make transpose-regions interactive.
> 
> I also noticed that transpose-sentences/paragraphs are not documented,
> so I'll add those in another commit.

Thanks.

> @@ -183,7 +185,7 @@ Transpose
>  (@code{transpose-lines}) exchanges lines.  They work like @kbd{M-t}
>  except as regards what units of text they transpose.
>  
> -  A numeric argument to a transpose command serves as a repeat count: it
> +  A numeric argument to most transpose commands serves as a repeat count: it
> [...]
> +@findex transpose-regions
> +  @kbd{M-x transpose-regions} transposes the text between point and
> +mark with the text between the first two elements of the mark ring.

I would say "... with the text between the last two marks pushed to
the mark ring."  I would also add a cross-reference here to where
set-mark-command is described.

> +Unlike the other transpose commands, it does not behave differently
> +when given a prefix argument.  This command is best used for
> +transposing multiple units of text (words, sentences, paragraphs) in
> +one go.

Would it make sense to make the new command interpret the prefix
argument the same way as the other transpose commands?  E.g., why not
use pairs of marks further back in the mark ring?

> -DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
> +DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
> +       "(if (< (length mark-ring) 2)\
> +            (error \"Mark ring must contain at least two elements\")\
> +          (list (point) (mark) (car mark-ring) (cadr mark-ring)))",

The error message could be made more clear, e.g. by saying that the
other region should be marked first.  Talking about the size of the
mark ring is too technical, IMO.

> +Interactively, STARTR1 and ENDR1 are point and mark.  STARTR2 and
> +ENDR2 are the first and second markers in the mark ring.
> +LEAVE-MARKERS is nil.  */)

"First and second" is ambiguous, since you don't tell from which end
they are counted.  Also, these 3 sentences all talk about interactive
invocation, so they should probably be a single sentence separated
with semi-colons instead.

bug#30343: make transpose-regions interactive

[Charles A. Roelli]

> Date: Sat, 10 Feb 2018 12:58:23 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > @@ -183,7 +185,7 @@ Transpose
> >  (@code{transpose-lines}) exchanges lines.  They work like @kbd{M-t}
> >  except as regards what units of text they transpose.
> >  
> > -  A numeric argument to a transpose command serves as a repeat count: it
> > +  A numeric argument to most transpose commands serves as a repeat count: it
> > [...]
> > +@findex transpose-regions
> > +  @kbd{M-x transpose-regions} transposes the text between point and
> > +mark with the text between the first two elements of the mark ring.
> 
> I would say "... with the text between the last two marks pushed to
> the mark ring."  I would also add a cross-reference here to where
> set-mark-command is described.
> 
> > +Unlike the other transpose commands, it does not behave differently
> > +when given a prefix argument.  This command is best used for
> > +transposing multiple units of text (words, sentences, paragraphs) in
> > +one go.
> 
> Would it make sense to make the new command interpret the prefix
> argument the same way as the other transpose commands?  E.g., why not
> use pairs of marks further back in the mark ring?
> 
> > -DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
> > +DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
> > +       "(if (< (length mark-ring) 2)\
> > +            (error \"Mark ring must contain at least two elements\")\
> > +          (list (point) (mark) (car mark-ring) (cadr mark-ring)))",
> 
> The error message could be made more clear, e.g. by saying that the
> other region should be marked first.  Talking about the size of the
> mark ring is too technical, IMO.
> 
> > +Interactively, STARTR1 and ENDR1 are point and mark.  STARTR2 and
> > +ENDR2 are the first and second markers in the mark ring.
> > +LEAVE-MARKERS is nil.  */)
> 
> "First and second" is ambiguous, since you don't tell from which end
> they are counted.  Also, these 3 sentences all talk about interactive
> invocation, so they should probably be a single sentence separated
> with semi-colons instead.

Thanks for the review.  Could you please look at the following second
iteration?  All your points should be addressed.

diff --git a/doc/emacs/fixit.texi b/doc/emacs/fixit.texi
index 7cacac4..eb783d1 100644
--- a/doc/emacs/fixit.texi
+++ b/doc/emacs/fixit.texi
@@ -149,6 +149,8 @@ Transpose
 Transpose two balanced expressions (@code{transpose-sexps}).
 @item C-x C-t
 Transpose two lines (@code{transpose-lines}).
+@item M-x transpose-regions
+Transpose two regions.
 @end table
 
 @kindex C-t
@@ -204,6 +206,15 @@ Transpose
 transpose the character (or word or expression or line) ending after
 point with the one ending after the mark.
 
+@findex transpose-regions
+  @kbd{M-x transpose-regions} transposes the text between point and
+mark with the text between the last two marks pushed to the mark ring
+(@pxref{Setting Mark}).  With a numeric prefix argument, it transposes
+the text between point and mark with the text between two successive
+marks that many entries back in the mark ring.  This command is best
+used for transposing multiple characters (or words or sentences or
+paragraphs) in one go.
+
 @node Fixing Case
 @section Case Conversion
 
diff --git a/src/editfns.c b/src/editfns.c
index 96bb271..9183bd1 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -5112,7 +5112,16 @@ transpose_markers (ptrdiff_t start1, ptrdiff_t end1,
     }
 }
 
-DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, 0,
+DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
+       "(if (< (length mark-ring) 2)\
+	    (error \"Other region must be marked before transposing two regions\")\
+	  (let* ((num (if current-prefix-arg\
+			 (prefix-numeric-value current-prefix-arg)\
+			0))\
+		 (ring-length (length mark-ring))\
+		 (eltnum (mod num ring-length))\
+		 (eltnum2 (mod (1+ num) ring-length)))\
+	    (list (point) (mark) (elt mark-ring eltnum) (elt mark-ring eltnum2))))",
        doc: /* Transpose region STARTR1 to ENDR1 with STARTR2 to ENDR2.
 The regions should not be overlapping, because the size of the buffer is
 never changed in a transposition.
@@ -5120,7 +5129,14 @@ never changed in a transposition.
 Optional fifth arg LEAVE-MARKERS, if non-nil, means don't update
 any markers that happen to be located in the regions.
 
-Transposing beyond buffer boundaries is an error.  */)
+Transposing beyond buffer boundaries is an error.
+
+Interactively, STARTR1 and ENDR1 are point and mark; STARTR2 and ENDR2
+are the last two marks pushed to the mark ring; LEAVE-MARKERS is nil.
+If a prefix argument N is given, STARTR2 and ENDR2 are the two
+successive marks N entries backwards in the mark ring.  A negative
+prefix argument instead counts forwards from the oldest mark in the
+mark ring.  */)
   (Lisp_Object startr1, Lisp_Object endr1, Lisp_Object startr2, Lisp_Object endr2, Lisp_Object leave_markers)
 {
   register ptrdiff_t start1, end1, start2, end2;

bug#30343: make transpose-regions interactive

[Eli Zaretskii]

> Date: Wed, 07 Mar 2018 21:56:28 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> CC: 30343@debbugs.gnu.org
> 
> Thanks for the review.  Could you please look at the following second
> iteration?  All your points should be addressed.

This LGTM, thanks.  One minor comment:

> +Interactively, STARTR1 and ENDR1 are point and mark; STARTR2 and ENDR2
> +are the last two marks pushed to the mark ring; LEAVE-MARKERS is nil.
> +If a prefix argument N is given, STARTR2 and ENDR2 are the two
> +successive marks N entries backwards in the mark ring.  A negative
> +prefix argument instead counts forwards from the oldest mark in the

Please use "back" in "forward" here, instead of "backwards" and
"forwards".  ("Backwards" has a meaning that might confuse readers
here.)

bug#30343: make transpose-regions interactive

[Eli Zaretskii]

> Date: Sat, 10 Mar 2018 14:02:01 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 30343@debbugs.gnu.org
> 
> Please use "back" in "forward" here
                    ^^
I meant "and", of course.  Sorry.

bug#30343: make transpose-regions interactive

[Charles A. Roelli]

> Date: Sat, 10 Mar 2018 14:02:01 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 30343@debbugs.gnu.org
> Reply-to: Eli Zaretskii <eliz@gnu.org>
> 
> This LGTM, thanks.  One minor comment:
> 
> > +Interactively, STARTR1 and ENDR1 are point and mark; STARTR2 and ENDR2
> > +are the last two marks pushed to the mark ring; LEAVE-MARKERS is nil.
> > +If a prefix argument N is given, STARTR2 and ENDR2 are the two
> > +successive marks N entries backwards in the mark ring.  A negative
> > +prefix argument instead counts forwards from the oldest mark in the
> 
> Please use "back" in "forward" here, instead of "backwards" and
> "forwards".  ("Backwards" has a meaning that might confuse readers
> here.)

Thanks again for the review, it's pushed now with your doc fix.

commit b88e7c8bcd523f1f503f87a1734679405322b5ec
Date:   Sun Mar 11 11:59:01 2018 +0100

  Make transpose-regions interactive (Bug#30343)
  
  * doc/emacs/fixit.texi (Transpose): Mention and explain the new
  command.
  * editfns.c (Ftranspose_regions): Add an interactive calling
  specification, and add documentation for it.