bug#41934: reverse-region no longer works

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Richard Copley-2
Recipe:
Insert text in a buffer:

abc
def
ghi

Position the mark before the 'a' and point before the 'g'.
Do 'M-x reverse-region'.

An error is signalled, 'There are no full lines in the region'.

See #39376.



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Eli Zaretskii
> From: Richard Copley <[hidden email]>
> Date: Thu, 18 Jun 2020 17:41:19 +0100
>
> Insert text in a buffer:
>
> abc
> def
> ghi
>
> Position the mark before the 'a' and point before the 'g'.
> Do 'M-x reverse-region'.
>
> An error is signalled, 'There are no full lines in the region'.

Thanks.  Does the patch below look good?

diff --git a/lisp/sort.el b/lisp/sort.el
index de0e1b9..6640c8f 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -554,8 +554,8 @@ reverse-region
   (if (> beg end)
       (let (mid) (setq mid end end beg beg mid)))
   (save-excursion
-    (when (or (< (line-beginning-position) beg)
-              (< end (line-end-position)))
+    (when (< (- end beg)
+             (- (line-end-position) (line-beginning-position)))
       (user-error "There are no full lines in the region"))
     ;; Put beg at the start of a line and end and the end of one --
     ;; the largest possible region which fits this criteria.



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Richard Copley-2
On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <[hidden email]> wrote:

> diff --git a/lisp/sort.el b/lisp/sort.el
> index de0e1b9..6640c8f 100644
> --- a/lisp/sort.el
> +++ b/lisp/sort.el
> @@ -554,8 +554,8 @@ reverse-region
>    (if (> beg end)
>        (let (mid) (setq mid end end beg beg mid)))
>    (save-excursion
> -    (when (or (< (line-beginning-position) beg)
> -              (< end (line-end-position)))
> +    (when (< (- end beg)
> +             (- (line-end-position) (line-beginning-position)))
>        (user-error "There are no full lines in the region"))
>      ;; Put beg at the start of a line and end and the end of one --
>      ;; the largest possible region which fits this criteria.

Thanks,
No, that fails on this example:

[mark]abc
def
[point]abcdefghi



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Richard Copley-2
On Thu, 18 Jun 2020 at 18:52, Richard Copley <[hidden email]> wrote:
> On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <[hidden email]> wrote:
> Thanks,
> No, that fails on this example [...]

How's this?

diff --git a/lisp/sort.el b/lisp/sort.el
index de0e1b9519..f878db24a3 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -554,9 +554,6 @@ is the one that ends before END."
   (if (> beg end)
       (let (mid) (setq mid end end beg beg mid)))
   (save-excursion
-    (when (or (< (line-beginning-position) beg)
-              (< end (line-end-position)))
-      (user-error "There are no full lines in the region"))
     ;; Put beg at the start of a line and end and the end of one --
     ;; the largest possible region which fits this criteria.
     (goto-char beg)
@@ -568,6 +565,8 @@ is the one that ends before END."
     ;; reversal; it isn't difficult to add it afterward.
     (or (and (eolp) (not (bolp))) (progn (forward-line -1) (end-of-line)))
     (setq end (point-marker))
+    (when (<= end beg)
+      (user-error "There are no full lines in the region"))
     ;; The real work.  This thing cranks through memory on large regions.
     (let (ll (do t))
       (while do



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Eli Zaretskii
In reply to this post by Richard Copley-2
> From: Richard Copley <[hidden email]>
> Date: Thu, 18 Jun 2020 18:52:48 +0100
> Cc: [hidden email]
>
> No, that fails on this example:
>
> [mark]abc
> def
> [point]abcdefghi

How about this:

diff --git a/lisp/sort.el b/lisp/sort.el
index de0e1b9..8a4a56c 100644
--- a/lisp/sort.el
+++ b/lisp/sort.el
@@ -554,9 +554,18 @@ reverse-region
   (if (> beg end)
       (let (mid) (setq mid end end beg beg mid)))
   (save-excursion
-    (when (or (< (line-beginning-position) beg)
-              (< end (line-end-position)))
-      (user-error "There are no full lines in the region"))
+    (let ((lbeg (save-excursion
+                  (goto-char beg)
+                  (if (bolp)
+                      beg
+                    (line-beginning-position 2))))
+          (lend (save-excursion
+                  (goto-char end)
+                  (if (bolp)
+                      end
+                    (line-beginning-position)))))
+      (when (>= lbeg lend)
+        (user-error "There are no full lines in the region")))
     ;; Put beg at the start of a line and end and the end of one --
     ;; the largest possible region which fits this criteria.
     (goto-char beg)



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Eli Zaretskii
In reply to this post by Richard Copley-2
> From: Richard Copley <[hidden email]>
> Date: Thu, 18 Jun 2020 18:59:21 +0100
> Cc: [hidden email]
>
> On Thu, 18 Jun 2020 at 18:52, Richard Copley <[hidden email]> wrote:
> > On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <[hidden email]> wrote:
> > Thanks,
> > No, that fails on this example [...]
>
> How's this?

Looks good, but what if beg and end are on the same line (at the point
where you added the test)?  Does that not warrant the error message?



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Richard Copley-2
On Thu, 18 Jun 2020 at 19:28, Eli Zaretskii <[hidden email]> wrote:

>
> > From: Richard Copley <[hidden email]>
> > Date: Thu, 18 Jun 2020 18:59:21 +0100
> > Cc: [hidden email]
> >
> > On Thu, 18 Jun 2020 at 18:52, Richard Copley <[hidden email]> wrote:
> > > On Thu, 18 Jun 2020 at 18:42, Eli Zaretskii <[hidden email]> wrote:
> > > Thanks,
> > > No, that fails on this example [...]
> >
> > How's this?
>
> Looks good, but what if beg and end are on the same line (at the point
> where you added the test)?  Does that not warrant the error message?

Not that error message, no, but rather something like "There are fewer
than two lines in the region".

I don't see the point of this error at all: if there are fewer than
two lines in the region-to-be-reversed at that point, in my opinion
the command should have no effect.



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Eli Zaretskii
> From: Richard Copley <[hidden email]>
> Date: Thu, 18 Jun 2020 19:33:57 +0100
> Cc: [hidden email]
>
> > Looks good, but what if beg and end are on the same line (at the point
> > where you added the test)?  Does that not warrant the error message?
>
> Not that error message, no, but rather something like "There are fewer
> than two lines in the region".

The original message says "lines", plural, so I think it kinda hints
on that.  We could make it say something that will cover both cases.

> I don't see the point of this error at all: if there are fewer than
> two lines in the region-to-be-reversed at that point, in my opinion
> the command should have no effect.

Indeed, it doesn't have any effect.  I'm asking whether this should be
flagged, since doing that makes no sense.



Reply | Threaded
Open this post in threaded view
|

bug#41934: reverse-region no longer works

Eli Zaretskii
> From: Richard Copley <[hidden email]>
> Date: Sat, 20 Jun 2020 11:03:34 +0100
> Cc: [hidden email]
>
> > Here's the conundrum: I'd like to install your proposed change, but
> > the sum total of your contributions to Emacs already exceeds the
> > amount we can accept without legal papers.
>
> Damn, sorry about that. You told me otherwise last time the question came up[1].

That was before that additional contribution.

Anyway, I asked RMS, and he said we can accept this patch, since most
of your substantial contribution in the past is now superseded.  So I
installed your patch, and I'm closing the bug.

Thanks!