unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
@ 2019-10-11 11:51 Mattias Engdegård
  2019-10-11 12:19 ` martin rudalics
  2019-10-11 12:26 ` Eli Zaretskii
  0 siblings, 2 replies; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-11 11:51 UTC (permalink / raw)
  To: 37700

Undoing mouse-drag-and-drop-region doesn't work:

1. (setq mouse-drag-and-drop-region t)
2. mark a region
3. use the mouse to drag the region elsewhere
4. Undo (twice)

Result: the dragged text disappears from its destination, but does not reappear where it was dragged from.
Instead, we get the "No further undo information for region" error.

The action can be undone by moving point and backing up a different undo branch a few steps, but that is hardly ideal.






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 11:51 bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective Mattias Engdegård
@ 2019-10-11 12:19 ` martin rudalics
  2019-10-11 12:51   ` Mattias Engdegård
  2019-10-11 12:26 ` Eli Zaretskii
  1 sibling, 1 reply; 28+ messages in thread
From: martin rudalics @ 2019-10-11 12:19 UTC (permalink / raw)
  To: Mattias Engdegård, 37700

 > Undoing mouse-drag-and-drop-region doesn't work:
 >
 > 1. (setq mouse-drag-and-drop-region t)
 > 2. mark a region
 > 3. use the mouse to drag the region elsewhere
 > 4. Undo (twice)
 >
 > Result: the dragged text disappears from its destination, but does not reappear where it was dragged from.
 > Instead, we get the "No further undo information for region" error.
 >
 > The action can be undone by moving point and backing up a different undo branch a few steps, but that is hardly ideal.

I suppose this the effect of undo working on the active region only.
A very nasty invention.  Try deactivating the region right after 3.

martin





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 11:51 bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective Mattias Engdegård
  2019-10-11 12:19 ` martin rudalics
@ 2019-10-11 12:26 ` Eli Zaretskii
  2019-10-11 12:53   ` Mattias Engdegård
  1 sibling, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-11 12:26 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 11 Oct 2019 13:51:38 +0200
> 
> Undoing mouse-drag-and-drop-region doesn't work:
> 
> 1. (setq mouse-drag-and-drop-region t)
> 2. mark a region
> 3. use the mouse to drag the region elsewhere
> 4. Undo (twice)
> 
> Result: the dragged text disappears from its destination, but does not reappear where it was dragged from.
> Instead, we get the "No further undo information for region" error.

Why is that a problem?  Emacs cannot possibly "un-dnd" text, it can
only remove the copy, which AFAIU it does.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 12:19 ` martin rudalics
@ 2019-10-11 12:51   ` Mattias Engdegård
  2019-10-11 14:43     ` Eli Zaretskii
  2019-10-11 18:26     ` martin rudalics
  0 siblings, 2 replies; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-11 12:51 UTC (permalink / raw)
  To: martin rudalics; +Cc: 37700

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

11 okt. 2019 kl. 14.19 skrev martin rudalics <rudalics@gmx.at>:
> 
> I suppose this the effect of undo working on the active region only.
> A very nasty invention.  Try deactivating the region right after 3.

Right, thank you. I wonder if anything would break if that were changed.

Proof-of-concept patch attached. (Manual and NEWS changes not included.) It does solve this particular problem.

Do you know of any drawbacks for someone whose editing habits are not dependent on the region confinement of undo?


[-- Attachment #2: 0001-Make-undo-confinement-to-active-region-user-settable.patch --]
[-- Type: application/octet-stream, Size: 2424 bytes --]

From e2e3a60fcc42eea4703c6f7b2877eae7188a73bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 11 Oct 2019 14:42:48 +0200
Subject: [PATCH] Make undo confinement to active region user-settable
 (bug#37700)

* lisp/simple.el (undo-confined-to-active-region, undo):
Add defcustom `undo-confined-to-active-region' that controls whether
the effects of `undo' are confined to the region when it is active.
The default value is t.
---
 lisp/simple.el | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 597278ae2b..10dec26200 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2464,6 +2464,14 @@ minibuffer-error-function
 ;Put this on C-x u, so we can force that rather than C-_ into startup msg
 (define-obsolete-function-alias 'advertised-undo 'undo "23.2")
 
+(defcustom undo-confined-to-active-region t
+  "If non-nil, the effects of `undo' without argument are confined to
+the active region when there is one.  If nil, the effects of `undo'
+are independent of the region."
+  :type 'boolean
+  :group 'undo
+  :version "27.1")
+
 (defconst undo-equiv-table (make-hash-table :test 'eq :weakness t)
   "Table mapping redo records to the corresponding undo one.
 A redo record for undo-in-region maps to t.
@@ -2485,7 +2493,9 @@ undo
 A numeric ARG serves as a repeat count.
 
 In Transient Mark mode when the mark is active, only undo changes within
-the current region.  Similarly, when not in Transient Mark mode, just \\[universal-argument]
+the current region; this behaviour can be changed by customizing
+the variable `undo-confined-to-active-region'.
+Similarly, when not in Transient Mark mode, just \\[universal-argument]
 as an argument limits undo to changes within the current region."
   (interactive "*P")
   ;; Make last-command indicate for the next command that this was an undo.
@@ -2517,7 +2527,8 @@ undo
 		       ;; it shows nothing else happened in between.
 		       (gethash list undo-equiv-table))))
       (setq undo-in-region
-	    (or (region-active-p) (and arg (not (numberp arg)))))
+	    (or (and (region-active-p) undo-confined-to-active-region)
+                (and arg (not (numberp arg)))))
       (if undo-in-region
 	  (undo-start (region-beginning) (region-end))
 	(undo-start))
-- 
2.21.0 (Apple Git-122)


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 12:26 ` Eli Zaretskii
@ 2019-10-11 12:53   ` Mattias Engdegård
  0 siblings, 0 replies; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-11 12:53 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 37700

11 okt. 2019 kl. 14.26 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Why is that a problem?  Emacs cannot possibly "un-dnd" text, it can
> only remove the copy, which AFAIU it does.

It does not undo the deletion of the text at its original position, which is what I believe a user would have expected (at least this user).






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 12:51   ` Mattias Engdegård
@ 2019-10-11 14:43     ` Eli Zaretskii
  2019-10-11 17:18       ` Mattias Engdegård
  2019-10-11 18:26     ` martin rudalics
  1 sibling, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-11 14:43 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 11 Oct 2019 14:51:01 +0200
> Cc: 37700@debbugs.gnu.org
> 
> 11 okt. 2019 kl. 14.19 skrev martin rudalics <rudalics@gmx.at>:
> > 
> > I suppose this the effect of undo working on the active region only.
> > A very nasty invention.  Try deactivating the region right after 3.
> 
> Right, thank you. I wonder if anything would break if that were changed.
> 
> Proof-of-concept patch attached. (Manual and NEWS changes not included.) It does solve this particular problem.

Given that the region can be easily deactivated, I'm not sure I see
the sense in introducing options that selectively disable
region-sensitive behavior of specific commands.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 14:43     ` Eli Zaretskii
@ 2019-10-11 17:18       ` Mattias Engdegård
  2019-10-11 18:36         ` Eli Zaretskii
  2019-10-11 18:57         ` Eli Zaretskii
  0 siblings, 2 replies; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-11 17:18 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 37700

11 okt. 2019 kl. 16.43 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Given that the region can be easily deactivated, I'm not sure I see
> the sense in introducing options that selectively disable
> region-sensitive behavior of specific commands.

When the user doesn't like the result of an operation, be it a change of mind or a mistake, the first logical reaction is to undo. Many users if not most are unaware that undo in Emacs is restricted to an active region, and even if they know, they are not likely to think of it before instinctively pressing the well-learned undo key.

One reason is that no other editor works like this; another is that almost all other operations that can be undone either deactivate the region or take place wholly inside it, so there is no reason for the user to care about that quirk.

Moreover, users are often aware from experience with other software that the time to undo an action is immediately afterwards, with no other potentially inhibiting action in-between. Requiring a specific undo-enabling action is therefore doubly hostile.






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 12:51   ` Mattias Engdegård
  2019-10-11 14:43     ` Eli Zaretskii
@ 2019-10-11 18:26     ` martin rudalics
  2019-10-11 19:12       ` Eli Zaretskii
  2019-10-12  1:55       ` Tak Kunihiro
  1 sibling, 2 replies; 28+ messages in thread
From: martin rudalics @ 2019-10-11 18:26 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700

 > Do you know of any drawbacks for someone whose editing habits are
 > not dependent on the region confinement of undo?

Here I once had my own 'undo' function since I disliked the one in
simple.el.  Eventually I gave up because it was too tedious to keep up
with the changes of the original.  Nowadays I'm just using

(defun my-undo (&optional arg)
   (interactive)
   (if mark-active
       ;; No `undo-in-region'.
       (let (mark-active)
	(undo arg)
	(setq mark-active t)
	;; The following might be harmful, let's see.
	(setq deactivate-mark nil))
     (undo arg)))

But I also faintly remember a discussion with the author of
'mouse-drag-and-drop-region' on whether it is a good idea to activate
the mark after dropping - IIRC he did have some argument in favor of
it.  Maybe it would be easier to add an option for not activating the
region when dropping than adding one for 'undo'.

martin





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 17:18       ` Mattias Engdegård
@ 2019-10-11 18:36         ` Eli Zaretskii
  2019-10-11 18:51           ` Eli Zaretskii
  2019-10-12  8:24           ` martin rudalics
  2019-10-11 18:57         ` Eli Zaretskii
  1 sibling, 2 replies; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-11 18:36 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 11 Oct 2019 19:18:51 +0200
> Cc: martin rudalics <rudalics@gmx.at>, 37700@debbugs.gnu.org
> 
> 11 okt. 2019 kl. 16.43 skrev Eli Zaretskii <eliz@gnu.org>:
> > 
> > Given that the region can be easily deactivated, I'm not sure I see
> > the sense in introducing options that selectively disable
> > region-sensitive behavior of specific commands.
> 
> When the user doesn't like the result of an operation, be it a change of mind or a mistake, the first logical reaction is to undo. Many users if not most are unaware that undo in Emacs is restricted to an active region, and even if they know, they are not likely to think of it before instinctively pressing the well-learned undo key.
> 
> One reason is that no other editor works like this; another is that almost all other operations that can be undone either deactivate the region or take place wholly inside it, so there is no reason for the user to care about that quirk.
> 
> Moreover, users are often aware from experience with other software that the time to undo an action is immediately afterwards, with no other potentially inhibiting action in-between. Requiring a specific undo-enabling action is therefore doubly hostile.

You are arguing that 'undo' should never be sensitive to the active
region.  If that was so, we'd have gobs of bug reports, because this
behavior is quite old, since 2007 AFAICT.

So evidently this feature is not as hostile as you seem to imply.
Moreover, the situation is not irrecoverable, even if the user does
make mistakes you describe: one can undo the undo, then disable the
region, and undo again.

Your change, however, was for an option that would disable this
behavior only optionally, not altogether.  And my response was to the
idea that we should have knobs that selectively disable
region-sensitive behavior of specific commands one by one.  I still
maintain that doing that for individual commands makes little sense to
me.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 18:36         ` Eli Zaretskii
@ 2019-10-11 18:51           ` Eli Zaretskii
  2019-10-12  8:24           ` martin rudalics
  1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-11 18:51 UTC (permalink / raw)
  To: mattiase; +Cc: 37700

> Date: Fri, 11 Oct 2019 21:36:05 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 37700@debbugs.gnu.org
> 
> You are arguing that 'undo' should never be sensitive to the active
> region.  If that was so, we'd have gobs of bug reports, because this
> behavior is quite old, since 2007 AFAICT.

Actually, more like since 1998: it was introduced in Emacs 20.3.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 17:18       ` Mattias Engdegård
  2019-10-11 18:36         ` Eli Zaretskii
@ 2019-10-11 18:57         ` Eli Zaretskii
  1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-11 18:57 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 11 Oct 2019 19:18:51 +0200
> Cc: martin rudalics <rudalics@gmx.at>, 37700@debbugs.gnu.org
> 
> another [reason] is that almost all other operations that can be undone either deactivate the region or take place wholly inside it, so there is no reason for the user to care about that quirk.

So perhaps a better way to resolve this situation is to teach 'undo'
about drag-and-drop, so that it doesn't undo selectively immediately
after drag-and-drop?

IOW, the selective undo feature AFAIU assumes that the region is set
up by the user before invoking 'undo', and that is not so in this
case.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 18:26     ` martin rudalics
@ 2019-10-11 19:12       ` Eli Zaretskii
  2019-10-12  1:55       ` Tak Kunihiro
  1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-11 19:12 UTC (permalink / raw)
  To: martin rudalics; +Cc: mattiase, 37700

> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 11 Oct 2019 20:26:23 +0200
> Cc: 37700@debbugs.gnu.org
> 
> But I also faintly remember a discussion with the author of
> 'mouse-drag-and-drop-region' on whether it is a good idea to activate
> the mark after dropping - IIRC he did have some argument in favor of
> it.  Maybe it would be easier to add an option for not activating the
> region when dropping than adding one for 'undo'.

Yes, that would be a possible solution, I think, since that command is
quite new.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 18:26     ` martin rudalics
  2019-10-11 19:12       ` Eli Zaretskii
@ 2019-10-12  1:55       ` Tak Kunihiro
  2019-10-12  8:24         ` martin rudalics
  1 sibling, 1 reply; 28+ messages in thread
From: Tak Kunihiro @ 2019-10-12  1:55 UTC (permalink / raw)
  To: martin rudalics; +Cc: Mattias Engdegård, 37700, tkk

> But I also faintly remember a discussion with the author of
> 'mouse-drag-and-drop-region' on whether it is a good idea to activate
> the mark after dropping - IIRC he did have some argument in favor of
> it.  Maybe it would be easier to add an option for not activating the
> region when dropping than adding one for 'undo'.

On revision of writing, I often want to move a sentence around to fit
into right place, without loosing the sentence from sight.  Most of the
time I cannot relocate the sentence to the best place by single
drag-and-drop operation thus I want to maintain region active.

I found that I did not notice problem pointed by Mattias because I
assign `undo' by `redo+.el' to C-/.  I agree that undo behavior by C-/
with Emacs -Q is inconvenient!

https://www.emacswiki.org/emacs/redo+.el





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-11 18:36         ` Eli Zaretskii
  2019-10-11 18:51           ` Eli Zaretskii
@ 2019-10-12  8:24           ` martin rudalics
  1 sibling, 0 replies; 28+ messages in thread
From: martin rudalics @ 2019-10-12  8:24 UTC (permalink / raw)
  To: Eli Zaretskii, Mattias Engdegård; +Cc: 37700

 > And my response was to the
 > idea that we should have knobs that selectively disable
 > region-sensitive behavior of specific commands one by one.  I still
 > maintain that doing that for individual commands makes little sense to
 > me.

Full agreement here.

martin





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-12  1:55       ` Tak Kunihiro
@ 2019-10-12  8:24         ` martin rudalics
  2019-10-12 16:42           ` Mattias Engdegård
  0 siblings, 1 reply; 28+ messages in thread
From: martin rudalics @ 2019-10-12  8:24 UTC (permalink / raw)
  To: Tak Kunihiro; +Cc: Mattias Engdegård, 37700, tkk

 > On revision of writing, I often want to move a sentence around to fit
 > into right place, without loosing the sentence from sight.  Most of the
 > time I cannot relocate the sentence to the best place by single
 > drag-and-drop operation thus I want to maintain region active.
 >
 > I found that I did not notice problem pointed by Mattias because I
 > assign `undo' by `redo+.el' to C-/.  I agree that undo behavior by C-/
 > with Emacs -Q is inconvenient!

I think that we should provide an option to not enable the region
after dropping and maybe even make it the default.

martin





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-12  8:24         ` martin rudalics
@ 2019-10-12 16:42           ` Mattias Engdegård
  2019-10-12 17:17             ` Eli Zaretskii
  0 siblings, 1 reply; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-12 16:42 UTC (permalink / raw)
  To: 37700; +Cc: Tak Kunihiro, tkk

Tak Kunihiro wrote:

> On revision of writing, I often want to move a sentence around to fit
> into right place, without loosing the sentence from sight.  Most of the
> time I cannot relocate the sentence to the best place by single
> drag-and-drop operation thus I want to maintain region active.

Thank you, very much my experience.

> I found that I did not notice problem pointed by Mattias because I
> assign `undo' by `redo+.el' to C-/.

Ah yes; apparently, region handling is mentioned in a TODO comment in redo+.el but nobody has bothered to implement it. Looks like the demand isn't there.

Martin Rudalics wrote:

> I think that we should provide an option to not enable the region
> after dropping and maybe even make it the default.

That would seriously degrade usability of the drag-and-drop feature. Selecting the text at its final position both highlights it, and allows the user to drag it again or do other region-related operations. (Other editors work the same way.)

This behaviour is definitely more important to the drag-and-drop user than the region-confinement of undo.

Eli Zaretskii wrote:

> So perhaps a better way to resolve this situation is to teach 'undo'
> about drag-and-drop, so that it doesn't undo selectively immediately
> after drag-and-drop?

Maybe --- can it be done within the current 'undo' framework, or would drag-and-drop need to be special-cased? Did you have a particular approach in mind?






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-12 16:42           ` Mattias Engdegård
@ 2019-10-12 17:17             ` Eli Zaretskii
  2019-10-12 17:53               ` Eli Zaretskii
  0 siblings, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-12 17:17 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700, tkk, homeros.misasa

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 12 Oct 2019 18:42:30 +0200
> Cc: Tak Kunihiro <homeros.misasa@gmail.com>, tkk@misasa.okayama-u.ac.jp,
>         Eli Zaretskii <eliz@gnu.org>, martin rudalics <rudalics@gmx.at>
> 
> > So perhaps a better way to resolve this situation is to teach 'undo'
> > about drag-and-drop, so that it doesn't undo selectively immediately
> > after drag-and-drop?
> 
> Maybe --- can it be done within the current 'undo' framework, or would drag-and-drop need to be special-cased? Did you have a particular approach in mind?

I didn't figure out the details, I only had a general idea.  I thought
about doing that inside 'undo', perhaps have a list of commands for
which an immediate 'undo' will disregard an active region.






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-12 17:17             ` Eli Zaretskii
@ 2019-10-12 17:53               ` Eli Zaretskii
  2019-10-16 15:27                 ` Mattias Engdegård
  0 siblings, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-12 17:53 UTC (permalink / raw)
  To: mattiase; +Cc: 37700, tkk, homeros.misasa

> Date: Sat, 12 Oct 2019 20:17:02 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 37700@debbugs.gnu.org, tkk@misasa.okayama-u.ac.jp, homeros.misasa@gmail.com
> 
> perhaps have a list of commands for which an immediate 'undo' will
> disregard an active region.

Or maybe have those commands have an 'undo' property which would tell
'undo' to treat them specially.  Like we do with delete-selection.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-12 17:53               ` Eli Zaretskii
@ 2019-10-16 15:27                 ` Mattias Engdegård
  2019-10-26 10:15                   ` Eli Zaretskii
  2019-10-28 20:01                   ` Stefan Monnier
  0 siblings, 2 replies; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-16 15:27 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 37700, tkk, homeros.misasa

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

12 okt. 2019 kl. 19.53 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Or maybe have those commands have an 'undo' property which would tell
> 'undo' to treat them specially.  Like we do with delete-selection.

After trying various approaches, the attached patch looks somewhat promising. It adds a new element type to buffer-undo-list, `unconfined', which disables selective undo for one record. Consider it a proof-of-concept.

While it has the advantage of not requiring the user to change any settings, I still prefer the first approach (an option to disable undo confinement), as it's less intrusive and more generally useful. However, this should work as well.


[-- Attachment #2: 0001-Don-t-confine-undo-of-mouse-drag-and-drop-to-the-reg.patch --]
[-- Type: application/octet-stream, Size: 4497 bytes --]

From 307e5c6a19eda9d464e75f73b73f57fc1cf9e42e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 16 Oct 2019 15:32:22 +0200
Subject: [PATCH] Don't confine undo of mouse-drag-and-drop to the region
 (bug#37700)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since the region is active after a mouse-drag-and-drop, the change
cannot immediately be undone.  To get around this, tell the undo
machinery to ignore the region confinement for this operation.

* lisp/simple.el (primitive-undo, undo-make-selective-list):
* src/buffer.c (buffer-undo-list):
New `buffer-undo-list’ element `unconfined'.
* lisp/mouse.el (mouse-drag-and-drop-region):
Mark the operation as unconfined upon undo.
---
 lisp/mouse.el  | 7 ++++++-
 lisp/simple.el | 9 +++++++--
 src/buffer.c   | 3 +++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 76fec507e7..533ad606fb 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -2657,7 +2657,12 @@ mouse-drag-and-drop-region
                 (let (deactivate-mark)
                   (dolist (overlay mouse-drag-and-drop-overlays)
                     (delete-region (overlay-start overlay)
-                                   (overlay-end overlay)))))
+                                   (overlay-end overlay))))
+                ;; Since we will leave the destination text selected,
+                ;; make sure an undo operation disregards the region
+                ;; or the operation will only be partially undone.
+                (when (consp buffer-undo-list)
+                  (push 'unconfined buffer-undo-list)))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
diff --git a/lisp/simple.el b/lisp/simple.el
index b733f76ac7..ea580f651d 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2748,6 +2748,7 @@ primitive-undo
              (set-marker marker
                          (- marker offset)
                          (marker-buffer marker))))
+          (unconfined)                  ; Ignore this directive here.
           (_ (error "Unrecognized entry in undo list %S" next))))
       (setq arg (1- arg)))
     ;; Make sure an apply entry produces at least one undo entry,
@@ -2850,6 +2851,7 @@ undo-make-selective-list
   (let ((ulist buffer-undo-list)
         ;; A list of position adjusted undo elements in the region.
         (selective-list (list nil))
+        (unconfined nil)   ; Whether to include whole record unconditionally.
         ;; A list of undo-deltas for out of region undo elements.
         undo-deltas
         undo-elt)
@@ -2862,7 +2864,10 @@ undo-make-selective-list
        ((null undo-elt)
         ;; Don't put two nils together in the list
         (when (car selective-list)
-          (push nil selective-list)))
+          (push nil selective-list))
+        (setq unconfined nil))
+       ((eq undo-elt 'unconfined)
+        (setq unconfined t))
        ((and (consp undo-elt) (eq (car undo-elt) t))
         ;; This is a "was unmodified" element.  Keep it
         ;; if we have kept everything thus far.
@@ -2875,7 +2880,7 @@ undo-make-selective-list
        (t
         (let ((adjusted-undo-elt (undo-adjust-elt undo-elt
                                                   undo-deltas)))
-          (if (undo-elt-in-region adjusted-undo-elt start end)
+          (if (or (undo-elt-in-region adjusted-undo-elt start end) unconfined)
               (progn
                 (setq end (+ end (cdr (undo-delta adjusted-undo-elt))))
                 (push adjusted-undo-elt selective-list)
diff --git a/src/buffer.c b/src/buffer.c
index 8cb28d8aa7..978d4576df 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -6128,6 +6128,9 @@ from (abs POSITION).  If POSITION is positive, point was at the front
 Entries with value nil mark undo boundaries.  The undo command treats
 the changes between two undo boundaries as a single step to be undone.
 
+An entry with the value `unconfined' disables selective undo until the
+next undo boundary even if the region is active.
+
 If the value of the variable is t, undo information is not recorded.  */);
 
   DEFVAR_PER_BUFFER ("mark-active", &BVAR (current_buffer, mark_active), Qnil,
-- 
2.21.0 (Apple Git-122)


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-16 15:27                 ` Mattias Engdegård
@ 2019-10-26 10:15                   ` Eli Zaretskii
  2019-10-28 20:01                   ` Stefan Monnier
  1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-26 10:15 UTC (permalink / raw)
  To: Mattias Engdegård, Stefan Monnier; +Cc: 37700, tkk, homeros.misasa

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 16 Oct 2019 17:27:05 +0200
> Cc: 37700@debbugs.gnu.org, tkk@misasa.okayama-u.ac.jp,
>         homeros.misasa@gmail.com
> 
> After trying various approaches, the attached patch looks somewhat promising. It adds a new element type to buffer-undo-list, `unconfined', which disables selective undo for one record. Consider it a proof-of-concept.
> 
> While it has the advantage of not requiring the user to change any settings, I still prefer the first approach (an option to disable undo confinement), as it's less intrusive and more generally useful. However, this should work as well.

Thanks.  I don't consider myself an expert on undo, but the patch
looks reasonable to me.  I think we should describe this new element
in the ELisp manual, but other than that, I think it can go in.

Stefan, any comments?





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-16 15:27                 ` Mattias Engdegård
  2019-10-26 10:15                   ` Eli Zaretskii
@ 2019-10-28 20:01                   ` Stefan Monnier
  2019-10-30 19:09                     ` Mattias Engdegård
  1 sibling, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2019-10-28 20:01 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: tkk, 37700, homeros.misasa

> > On revision of writing, I often want to move a sentence around to fit
> > into right place, without loosing the sentence from sight.  Most of the
> > time I cannot relocate the sentence to the best place by single
> > drag-and-drop operation thus I want to maintain region active.
> Thank you, very much my experience.

Indeed, that's also my experience [ and as a result I stay away from
text's drag-and-drop since I find C-w ... C-y to be easier since it
avoids this trial-and-error problem ;-) ]

> -                                   (overlay-end overlay)))))
> +                                   (overlay-end overlay))))
> +                ;; Since we will leave the destination text selected,
> +                ;; make sure an undo operation disregards the region
> +                ;; or the operation will only be partially undone.
> +                (when (consp buffer-undo-list)
> +                  (push 'unconfined buffer-undo-list)))

Rather than add a special new kind of entry you can use some version of
(apply DELTA BEG END FUN-NAME . ARGS), presumably with DELTA=0 and
BEG=END, so you don't need to modify the docstring of `buffer-undo-list`
nor the implementation of primitive-undo.

> @@ -2862,7 +2864,10 @@ undo-make-selective-list
>         ((null undo-elt)
>          ;; Don't put two nils together in the list
>          (when (car selective-list)
> -          (push nil selective-list)))
> +          (push nil selective-list))
> +        (setq unconfined nil))
> +       ((eq undo-elt 'unconfined)
> +        (setq unconfined t))
>         ((and (consp undo-elt) (eq (car undo-elt) t))
>          ;; This is a "was unmodified" element.  Keep it
>          ;; if we have kept everything thus far.
> @@ -2875,7 +2880,7 @@ undo-make-selective-list
>         (t
>          (let ((adjusted-undo-elt (undo-adjust-elt undo-elt
>                                                    undo-deltas)))
> -          (if (undo-elt-in-region adjusted-undo-elt start end)
> +          (if (or (undo-elt-in-region adjusted-undo-elt start end) unconfined)
>                (progn
>                  (setq end (+ end (cdr (undo-delta adjusted-undo-elt))))
>                  (push adjusted-undo-elt selective-list)

As a user of undo-in-region, I think I'd be surprised if my undo started
to modify parts of the buffer outside the region, so I think a better
approach would be to only pay attention to the `unconfined' marker when
it appears at the top of the `buffer-undo-list` (i.e. only if the
drag-and-drop was the very last operation).  Also I think it would
deserve a message in the minibuffer explaining that the undo is not
confined to the region.


        Stefan






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-28 20:01                   ` Stefan Monnier
@ 2019-10-30 19:09                     ` Mattias Engdegård
  2019-10-30 19:56                       ` Stefan Monnier
  0 siblings, 1 reply; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-30 19:09 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: tkk, 37700, homeros.misasa

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

28 okt. 2019 kl. 21.01 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Indeed, that's also my experience [ and as a result I stay away from
> text's drag-and-drop since I find C-w ... C-y to be easier since it
> avoids this trial-and-error problem ;-) ]

Thanks for taking a look. It seems that the only happy users are those that use redo+.el or similar packages that do not restrict undo to region.

Unfortunately, even with the patch, undoing a drag-and-drop does not leave the region active the way it was before the undo, so the user has to reselect the text in order to try again. Reactivating the region automatically on undo is tempting but incompatible with undo-in-region (the same problem but in the other direction). Partly because of this, I believe that providing an option to disable undo-in-region altogether is a better solution.

> Rather than add a special new kind of entry you can use some version of
> (apply DELTA BEG END FUN-NAME . ARGS), presumably with DELTA=0 and
> BEG=END, so you don't need to modify the docstring of `buffer-undo-list`
> nor the implementation of primitive-undo.

I'm not sure how that would be done in practice since 'undo-elt-in-region' is nil for any (apply ...) element. This could be remedied, of course, but that would entail undo machinery changes which we wanted to avoid in the first place. In addition, it is unclear how the 'apply' mechanism could be used in a way that is sensitive to whether it's the first record to be undone.

> As a user of undo-in-region, I think I'd be surprised if my undo started
> to modify parts of the buffer outside the region, so I think a better
> approach would be to only pay attention to the `unconfined' marker when
> it appears at the top of the `buffer-undo-list` (i.e. only if the
> drag-and-drop was the very last operation).

The patch has now been modified to that effect. (I'm still not sure it's the right solution.)

>  Also I think it would
> deserve a message in the minibuffer explaining that the undo is not
> confined to the region.

It was tricky to do that cleanly, given the structure of the undo code, so it went into a separate patch.


[-- Attachment #2: 0001-Don-t-confine-undo-of-mouse-drag-and-drop-to-the-reg.patch --]
[-- Type: application/octet-stream, Size: 5001 bytes --]

From a857757e9f7d92b2acb9817399c9a4af49a4154c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 16 Oct 2019 15:32:22 +0200
Subject: [PATCH 1/2] Don't confine undo of mouse-drag-and-drop to the region
 (bug#37700)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since the region is active after a mouse-drag-and-drop, the change
cannot immediately be undone.  To get around this, tell the undo
machinery to ignore the region confinement for this operation if
it is undone right away.

* lisp/simple.el (primitive-undo, undo-make-selective-list):
* src/buffer.c (buffer-undo-list):
New `buffer-undo-list’ element `unconfined'.
* lisp/mouse.el (mouse-drag-and-drop-region):
Mark the operation as unconfined upon undo.
---
 lisp/mouse.el  |  7 ++++++-
 lisp/simple.el | 13 +++++++++++--
 src/buffer.c   |  4 ++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 76fec507e7..533ad606fb 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -2657,7 +2657,12 @@ mouse-drag-and-drop-region
                 (let (deactivate-mark)
                   (dolist (overlay mouse-drag-and-drop-overlays)
                     (delete-region (overlay-start overlay)
-                                   (overlay-end overlay)))))
+                                   (overlay-end overlay))))
+                ;; Since we will leave the destination text selected,
+                ;; make sure an undo operation disregards the region
+                ;; or the operation will only be partially undone.
+                (when (consp buffer-undo-list)
+                  (push 'unconfined buffer-undo-list)))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
diff --git a/lisp/simple.el b/lisp/simple.el
index 29e195bca6..83be410074 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2753,6 +2753,7 @@ primitive-undo
              (set-marker marker
                          (- marker offset)
                          (marker-buffer marker))))
+          (unconfined)                  ; Ignore this directive here.
           (_ (error "Unrecognized entry in undo list %S" next))))
       (setq arg (1- arg)))
     ;; Make sure an apply entry produces at least one undo entry,
@@ -2855,9 +2856,15 @@ undo-make-selective-list
   (let ((ulist buffer-undo-list)
         ;; A list of position adjusted undo elements in the region.
         (selective-list (list nil))
+        (unconfined nil)   ; Whether to include whole record unconditionally.
         ;; A list of undo-deltas for out of region undo elements.
         undo-deltas
         undo-elt)
+    ;; The `unconfined' directive only applies to the very first record.
+    (pcase ulist
+      (`(nil unconfined . ,rest)
+       (setq unconfined t)
+       (setq ulist rest)))
     (while ulist
       (when undo-no-redo
         (while (gethash ulist undo-equiv-table)
@@ -2867,7 +2874,8 @@ undo-make-selective-list
        ((null undo-elt)
         ;; Don't put two nils together in the list
         (when (car selective-list)
-          (push nil selective-list)))
+          (push nil selective-list))
+        (setq unconfined nil))
        ((and (consp undo-elt) (eq (car undo-elt) t))
         ;; This is a "was unmodified" element.  Keep it
         ;; if we have kept everything thus far.
@@ -2877,10 +2885,11 @@ undo-make-selective-list
        ;; on finding them after (TEXT . POS) elements
        ((markerp (car-safe undo-elt))
         nil)
+       ((eq undo-elt 'unconfined))  ; This directive has no further effect.
        (t
         (let ((adjusted-undo-elt (undo-adjust-elt undo-elt
                                                   undo-deltas)))
-          (if (undo-elt-in-region adjusted-undo-elt start end)
+          (if (or (undo-elt-in-region adjusted-undo-elt start end) unconfined)
               (progn
                 (setq end (+ end (cdr (undo-delta adjusted-undo-elt))))
                 (push adjusted-undo-elt selective-list)
diff --git a/src/buffer.c b/src/buffer.c
index 80eaa971a3..16012a97a2 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -6128,6 +6128,10 @@ from (abs POSITION).  If POSITION is positive, point was at the front
 Entries with value nil mark undo boundaries.  The undo command treats
 the changes between two undo boundaries as a single step to be undone.
 
+An entry with the value `unconfined' disables selective undo until the
+next undo boundary even if the region is active.  It only has effect
+on the first record to be undone.
+
 If the value of the variable is t, undo information is not recorded.  */);
 
   DEFVAR_PER_BUFFER ("mark-active", &BVAR (current_buffer, mark_active), Qnil,
-- 
2.21.0 (Apple Git-122)


[-- Attachment #3: 0002-Adapt-undo-message-when-undoing-unconfined-actions-i.patch --]
[-- Type: application/octet-stream, Size: 4154 bytes --]

From 71bc4e8352c6662458d5d18f328bd955054a91cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 30 Oct 2019 19:13:47 +0100
Subject: [PATCH 2/2] Adapt undo message when undoing unconfined actions in
 region

* lisp/simple.el (undo): Don't say "Undo in region" when the action
undone explicitly ignores region confinement (bug#37700).
(undo--first-record-unconfined): New.
(undo-make-selective-list): Use undo--first-record-unconfined.
---
 lisp/simple.el | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 83be410074..e08c9b6a89 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2508,6 +2508,8 @@ undo
 	 (base-buffer (or (buffer-base-buffer) (current-buffer)))
 	 (recent-save (with-current-buffer base-buffer
 			(recent-auto-save-p)))
+         (unconfined nil)
+         (count (if (numberp arg) (prefix-numeric-value arg) 1))
 	 message)
     ;; If we get an error in undo-start,
     ;; the next command should not be a "consecutive undo".
@@ -2527,7 +2529,9 @@ undo
       (setq undo-in-region
 	    (or (region-active-p) (and arg (not (numberp arg)))))
       (if undo-in-region
-	  (undo-start (region-beginning) (region-end))
+          (progn
+            (setq unconfined (undo--first-record-unconfined buffer-undo-list))
+            (undo-start (region-beginning) (region-end)))
 	(undo-start))
       ;; get rid of initial undo boundary
       (undo-more 1))
@@ -2537,20 +2541,20 @@ undo
     ;; so, ask the user whether she wants to skip the redo/undo pair.
     (let ((equiv (gethash pending-undo-list undo-equiv-table)))
       (or (eq (selected-window) (minibuffer-window))
-	  (setq message (format "%s%s"
-                                (if (or undo-no-redo (not equiv))
-                                    "Undo" "Redo")
-                                (if undo-in-region " in region" ""))))
+	  (setq message (concat
+                         (if (or undo-no-redo (not equiv))
+                             "Undo" "Redo")
+                         (cond ((and unconfined (> count 1))
+                                " in region (except first action)")
+                               ((and undo-in-region (not unconfined))
+                                " in region")))))
       (when (and (consp equiv) undo-no-redo)
 	;; The equiv entry might point to another redo record if we have done
 	;; undo-redo-undo-redo-... so skip to the very last equiv.
 	(while (let ((next (gethash equiv undo-equiv-table)))
 		 (if next (setq equiv next))))
 	(setq pending-undo-list equiv)))
-    (undo-more
-     (if (numberp arg)
-	 (prefix-numeric-value arg)
-       1))
+    (undo-more count)
     ;; Record the fact that the just-generated undo records come from an
     ;; undo operation--that is, they are redo records.
     ;; In the ordinary case (not within a region), map the redo
@@ -2847,6 +2851,13 @@ undo-start
 ;; "ccaabad", as though the first "d" became detached from the
 ;; original "ddd" insertion.  This quirk is a FIXME.
 
+(defun undo--first-record-unconfined (undo-list)
+  "If the first record of UNDO-LIST is marked unconfined, return the argument
+without the leading `unconfined' directive; otherwise nil."
+  (pcase undo-list
+    (`(nil unconfined . ,rest)
+     rest)))
+
 (defun undo-make-selective-list (start end)
   "Return a list of undo elements for the region START to END.
 The elements come from `buffer-undo-list', but we keep only the
@@ -2861,10 +2872,10 @@ undo-make-selective-list
         undo-deltas
         undo-elt)
     ;; The `unconfined' directive only applies to the very first record.
-    (pcase ulist
-      (`(nil unconfined . ,rest)
-       (setq unconfined t)
-       (setq ulist rest)))
+    (let ((ul (undo--first-record-unconfined ulist)))
+      (when ul
+        (setq ulist ul)
+        (setq unconfined t)))
     (while ulist
       (when undo-no-redo
         (while (gethash ulist undo-equiv-table)
-- 
2.21.0 (Apple Git-122)


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-30 19:09                     ` Mattias Engdegård
@ 2019-10-30 19:56                       ` Stefan Monnier
  2019-10-31 11:00                         ` Mattias Engdegård
  0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2019-10-30 19:56 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: tkk, 37700, homeros.misasa

> Unfortunately, even with the patch, undoing a drag-and-drop does not leave
> the region active the way it was before the undo, so the user has to
> reselect the text in order to try again.

If the undo-list is built right, reselecting the text should be just
`C-x C-x`, which isn't that bad.

> Partly because of this, I believe that providing an option to disable
> undo-in-region altogether is a better solution.

I agree that disabling it right after a drag-and-drop is a better choice.

> I'm not sure how that would be done in practice since 'undo-elt-in-region'
> is nil for any (apply ...) element.  This could be remedied, of course, but
> that would entail undo machinery changes which we wanted to avoid in the
> first place.

Yes, that's a long-standing missing feature, but I think it's orthogonal
to the current problem.

> In addition, it is unclear how the 'apply' mechanism could be used in
> a way that is sensitive to whether it's the first record to be undone.

With ad-hoc code looking for it at the beginning of `undo`.

But now that I think about it, maybe a better option would be to check

    (when (symbolp last-command)
      (get last-command 'undo-inhibit-region))

and then put the `undo-inhibit-region` property on
`mouse-drag-and-drop-region`.

Of course, I wouldn't oppose adding

    (defcustom undo-use-region-when-active t ...)
    
so users can turn it off, but I think it's more important to make sure
that users never need to set such a var to nil.


        Stefan






^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-30 19:56                       ` Stefan Monnier
@ 2019-10-31 11:00                         ` Mattias Engdegård
  2019-10-31 14:45                           ` Eli Zaretskii
  0 siblings, 1 reply; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-31 11:00 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: tkk, 37700, homeros.misasa

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

30 okt. 2019 kl. 20.56 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>> Unfortunately, even with the patch, undoing a drag-and-drop does not leave
>> the region active the way it was before the undo, so the user has to
>> reselect the text in order to try again.
> 
> If the undo-list is built right, reselecting the text should be just
> `C-x C-x`, which isn't that bad.

Right, second nature to the Emacs user, but it would still be nice not having to go through that step. kill-region and delete-selection-mode are similarly affected.

> But now that I think about it, maybe a better option would be to check
> 
>    (when (symbolp last-command)
>      (get last-command 'undo-inhibit-region))
> 
> and then put the `undo-inhibit-region` property on
> `mouse-drag-and-drop-region`.

Thank you, this looks like the best idea so far. A very simple change, yet effective in practice. Not perfect --- last-command is not buffer-local, and even switching to a different frame and back will change it --- but good enough.

Patch attached.


[-- Attachment #2: 0001-Inhibit-undo-in-region-for-mouse-drag-region-bug-377.patch --]
[-- Type: application/octet-stream, Size: 2582 bytes --]

From 7cf5f680ab1d933fe1cecb862afa3b0976045f1d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 31 Oct 2019 10:31:27 +0100
Subject: [PATCH] Inhibit undo-in-region for mouse-drag-region (bug#37700)

'mouse-drag-region' leaves the region active around the dragged text,
so a straight undo did not revert the entire operation.  To remedy
this, inhibit undo-in-region when the last command was
mouse-drag-region.  (Method suggested by Stefan Monnier.)

* lisp/mouse.el (undo-drag-region): Set the undo-inhibit-region property.
* lisp/simple.el (undo): Inhibit undo-in-region if the last command
had the undo-inhibit-region property set.
---
 lisp/mouse.el  | 6 ++++++
 lisp/simple.el | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 76fec507e7..4a351f7be2 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -1104,6 +1104,12 @@ mouse-drag-region
     (run-hooks 'mouse-leave-buffer-hook)
     (mouse-drag-track start-event)))
 
+;; Inhibit the region-confinement when undoing mouse-drag-region
+;; immediately after the command.  Otherwise, the selection left
+;; active around the dragged text would prevent an undo of the whole
+;; operation.
+(put 'mouse-drag-region 'undo-inhibit-region t)
+
 (defun mouse-posn-property (pos property)
   "Look for a property at click position.
 POS may be either a buffer position or a click position like
diff --git a/lisp/simple.el b/lisp/simple.el
index 29e195bca6..10aecd651f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2508,6 +2508,10 @@ undo
 	 (base-buffer (or (buffer-base-buffer) (current-buffer)))
 	 (recent-save (with-current-buffer base-buffer
 			(recent-auto-save-p)))
+         ;; Allow certain commands to inhibit an immediately following
+         ;; undo-in-region.
+         (inhibit-region (and (symbolp last-command)
+                              (get last-command 'undo-inhibit-region)))
 	 message)
     ;; If we get an error in undo-start,
     ;; the next command should not be a "consecutive undo".
@@ -2525,7 +2529,8 @@ undo
 		       ;; it shows nothing else happened in between.
 		       (gethash list undo-equiv-table))))
       (setq undo-in-region
-	    (or (region-active-p) (and arg (not (numberp arg)))))
+	    (and (or (region-active-p) (and arg (not (numberp arg))))
+                 (not inhibit-region)))
       (if undo-in-region
 	  (undo-start (region-beginning) (region-end))
 	(undo-start))
-- 
2.21.0 (Apple Git-122)


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-31 11:00                         ` Mattias Engdegård
@ 2019-10-31 14:45                           ` Eli Zaretskii
  2019-10-31 16:04                             ` Mattias Engdegård
  0 siblings, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-31 14:45 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700, tkk, homeros.misasa, monnier

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 31 Oct 2019 12:00:08 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, 37700@debbugs.gnu.org,
>         tkk@misasa.okayama-u.ac.jp, homeros.misasa@gmail.com
> 
> > But now that I think about it, maybe a better option would be to check
> > 
> >    (when (symbolp last-command)
> >      (get last-command 'undo-inhibit-region))
> > 
> > and then put the `undo-inhibit-region` property on
> > `mouse-drag-and-drop-region`.
> 
> Thank you, this looks like the best idea so far. A very simple change, yet effective in practice. Not perfect --- last-command is not buffer-local, and even switching to a different frame and back will change it --- but good enough.
> 
> Patch attached.

Fine with me (I proposed something like this during the original
discussion), but please document this property in the ELisp manual.

Thanks.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-31 14:45                           ` Eli Zaretskii
@ 2019-10-31 16:04                             ` Mattias Engdegård
  2019-10-31 16:10                               ` Eli Zaretskii
  0 siblings, 1 reply; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-31 16:04 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 37700, tkk, homeros.misasa, monnier

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

31 okt. 2019 kl. 15.45 skrev Eli Zaretskii <eliz@gnu.org>:

> Fine with me (I proposed something like this during the original
> discussion), but please document this property in the ELisp manual.

Then I apologise for misunderstanding you!
Attached patch amended with changes to the manual and NEWS.


[-- Attachment #2: 0001-Inhibit-undo-in-region-for-mouse-drag-region-bug-377.patch --]
[-- Type: application/octet-stream, Size: 4736 bytes --]

From 808f466b5f7c779c41a6e03b177906d79a16e66a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 31 Oct 2019 10:31:27 +0100
Subject: [PATCH] Inhibit undo-in-region for mouse-drag-region (bug#37700)

'mouse-drag-region' leaves the region active around the dragged text,
so a straight undo did not revert the entire operation.  To remedy
this, inhibit undo-in-region when the last command was
mouse-drag-region.  (Method suggested by Stefan Monnier.)

* lisp/mouse.el (undo-drag-region): Set the undo-inhibit-region property.
* lisp/simple.el (undo): Inhibit undo-in-region if the last command
had the undo-inhibit-region property set.
* doc/lispref/symbols.texi (Standard Properties):
* doc/lispref/text.texi (Undo): Document undo-inhibit-region.
* etc/NEWS: Announce the property.
---
 doc/lispref/symbols.texi | 5 +++++
 doc/lispref/text.texi    | 6 ++++++
 etc/NEWS                 | 6 ++++++
 lisp/mouse.el            | 6 ++++++
 lisp/simple.el           | 7 ++++++-
 5 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 5d71fb39a2..6d3a89a54b 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -594,4 +594,9 @@ Standard Properties
 If non-@code{nil}, this specifies the named variable's documentation
 string.  This is set automatically by @code{defvar} and related
 functions.  @xref{Defining Faces}.
+
+@item undo-inhibit-region
+If non-@code{nil}, the named function prevents the @code{undo} operation
+from being restricted to the active region, if @code{undo} is invoked
+immediately after the function.  @xref{Undo}.
 @end table
diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index ac444f6afe..9f3238e8aa 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -1451,6 +1451,12 @@ Undo
 This function does not bind @code{undo-in-progress}.
 @end defun
 
+Some commands leave the region active after execution in such a way that
+it interferes with selective undo of that command.  To make @code{undo}
+ignore the active region when invoked immediately after such a command,
+set the property @code{undo-inhibit-region} of the command's function
+symbol to a non-nil value.
+
 @node Maintaining Undo
 @section Maintaining Undo Lists
 
diff --git a/etc/NEWS b/etc/NEWS
index ee73d2414b..030246db40 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -3102,6 +3102,12 @@ in other packages are now obsolete aliases of 'xor'.
 Setting this on the first character of a help string disables
 conversions via 'substitute-command-keys'.
 
++++
+** 'undo' can be made to ignore the active region for a command
+by setting its 'undo-inhibit-region' symbol property to non-nil.
+This is used by 'mouse-drag-region' to make that command's effect
+easier to undo immediately afterwards.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/lisp/mouse.el b/lisp/mouse.el
index 76fec507e7..4a351f7be2 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -1104,6 +1104,12 @@ mouse-drag-region
     (run-hooks 'mouse-leave-buffer-hook)
     (mouse-drag-track start-event)))
 
+;; Inhibit the region-confinement when undoing mouse-drag-region
+;; immediately after the command.  Otherwise, the selection left
+;; active around the dragged text would prevent an undo of the whole
+;; operation.
+(put 'mouse-drag-region 'undo-inhibit-region t)
+
 (defun mouse-posn-property (pos property)
   "Look for a property at click position.
 POS may be either a buffer position or a click position like
diff --git a/lisp/simple.el b/lisp/simple.el
index 29e195bca6..10aecd651f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2508,6 +2508,10 @@ undo
 	 (base-buffer (or (buffer-base-buffer) (current-buffer)))
 	 (recent-save (with-current-buffer base-buffer
 			(recent-auto-save-p)))
+         ;; Allow certain commands to inhibit an immediately following
+         ;; undo-in-region.
+         (inhibit-region (and (symbolp last-command)
+                              (get last-command 'undo-inhibit-region)))
 	 message)
     ;; If we get an error in undo-start,
     ;; the next command should not be a "consecutive undo".
@@ -2525,7 +2529,8 @@ undo
 		       ;; it shows nothing else happened in between.
 		       (gethash list undo-equiv-table))))
       (setq undo-in-region
-	    (or (region-active-p) (and arg (not (numberp arg)))))
+	    (and (or (region-active-p) (and arg (not (numberp arg))))
+                 (not inhibit-region)))
       (if undo-in-region
 	  (undo-start (region-beginning) (region-end))
 	(undo-start))
-- 
2.21.0 (Apple Git-122)


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-31 16:04                             ` Mattias Engdegård
@ 2019-10-31 16:10                               ` Eli Zaretskii
  2019-10-31 16:47                                 ` Mattias Engdegård
  0 siblings, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2019-10-31 16:10 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 37700, tkk, homeros.misasa, monnier

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 31 Oct 2019 17:04:41 +0100
> Cc: monnier@iro.umontreal.ca, 37700@debbugs.gnu.org,
>         tkk@misasa.okayama-u.ac.jp, homeros.misasa@gmail.com
> 
> > Fine with me (I proposed something like this during the original
> > discussion), but please document this property in the ELisp manual.
> 
> Then I apologise for misunderstanding you!

No need to apologize; I was just explaining why I like this patch ;-)

> Attached patch amended with changes to the manual and NEWS.

Thanks.

> --- a/doc/lispref/text.texi
> +++ b/doc/lispref/text.texi
> @@ -1451,6 +1451,12 @@ Undo
>  This function does not bind @code{undo-in-progress}.
>  @end defun
>  
> +Some commands leave the region active after execution in such a way that
> +it interferes with selective undo of that command.  To make @code{undo}
> +ignore the active region when invoked immediately after such a command,
> +set the property @code{undo-inhibit-region} of the command's function
> +symbol to a non-nil value.

Please add here a cross-reference to the other place, where this
property is described.

> +** 'undo' can be made to ignore the active region for a command
> +by setting its 'undo-inhibit-region' symbol property to non-nil.

Instead of "its" I'd use "that command's", because "its" is a bit
ambiguous in this context.





^ permalink raw reply	[flat|nested] 28+ messages in thread

* bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective
  2019-10-31 16:10                               ` Eli Zaretskii
@ 2019-10-31 16:47                                 ` Mattias Engdegård
  0 siblings, 0 replies; 28+ messages in thread
From: Mattias Engdegård @ 2019-10-31 16:47 UTC (permalink / raw)
  To: 37700-done; +Cc: Tak Kunihiro, tkk, Stefan Monnier

31 okt. 2019 kl. 17.10 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Please add here a cross-reference to the other place, where this
> property is described.

Done.

>> +** 'undo' can be made to ignore the active region for a command
>> +by setting its 'undo-inhibit-region' symbol property to non-nil.
> 
> Instead of "its" I'd use "that command's", because "its" is a bit
> ambiguous in this context.

Done.

Pushed with those patches (and moved the property description so that the list follows alphabetical order).






^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2019-10-31 16:47 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11 11:51 bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective Mattias Engdegård
2019-10-11 12:19 ` martin rudalics
2019-10-11 12:51   ` Mattias Engdegård
2019-10-11 14:43     ` Eli Zaretskii
2019-10-11 17:18       ` Mattias Engdegård
2019-10-11 18:36         ` Eli Zaretskii
2019-10-11 18:51           ` Eli Zaretskii
2019-10-12  8:24           ` martin rudalics
2019-10-11 18:57         ` Eli Zaretskii
2019-10-11 18:26     ` martin rudalics
2019-10-11 19:12       ` Eli Zaretskii
2019-10-12  1:55       ` Tak Kunihiro
2019-10-12  8:24         ` martin rudalics
2019-10-12 16:42           ` Mattias Engdegård
2019-10-12 17:17             ` Eli Zaretskii
2019-10-12 17:53               ` Eli Zaretskii
2019-10-16 15:27                 ` Mattias Engdegård
2019-10-26 10:15                   ` Eli Zaretskii
2019-10-28 20:01                   ` Stefan Monnier
2019-10-30 19:09                     ` Mattias Engdegård
2019-10-30 19:56                       ` Stefan Monnier
2019-10-31 11:00                         ` Mattias Engdegård
2019-10-31 14:45                           ` Eli Zaretskii
2019-10-31 16:04                             ` Mattias Engdegård
2019-10-31 16:10                               ` Eli Zaretskii
2019-10-31 16:47                                 ` Mattias Engdegård
2019-10-11 12:26 ` Eli Zaretskii
2019-10-11 12:53   ` Mattias Engdegård

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).