bug#810: 23.0.60; atomic-change-group narrowing problem

bug#810: 23.0.60; atomic-change-group narrowing problem

[Lennart Borgman (gmail)]

When using atomic-change-group I got the message

  undo-more: Changes to be undone are outside visible portion of buffer

The changes are also not undone. This seems wrong to me.

If I put (widen) before the end of my code in the atomic-change-group
the message above disappears and the change is undone.


In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-08-10
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags
-Ic:/g/include -fno-crossjumping'

bug#810: 23.0.60; atomic-change-group narrowing problem

[martin rudalics]

 > When using atomic-change-group I got the message
 >
 >   undo-more: Changes to be undone are outside visible portion of buffer

This message is usually a consequence of narrowing the buffer within the
body of `atomic-change-group' or afterwards.

 > The changes are also not undone. This seems wrong to me.

If you did narrow the buffer this is normal and not specific to
`atomic-change-group'.

martin

bug#810: 23.0.60; atomic-change-group narrowing problem

[Lennart Borgman (gmail)]

martin rudalics wrote:
>> When using atomic-change-group I got the message
>>
>>   undo-more: Changes to be undone are outside visible portion of buffer
> 
> This message is usually a consequence of narrowing the buffer within the
> body of `atomic-change-group' or afterwards.
> 
>> The changes are also not undone. This seems wrong to me.
> 
> If you did narrow the buffer this is normal and not specific to
> `atomic-change-group'.

Maybe, but what is specific to atomic-change-group is that the doc
string says

  Perform body as an atomic change group.
  This means that if body exits abnormally,
  all of its changes to the current buffer are undone.

AFAICS there is something wrong either in the doc string or in the function.

I would suggest that this is a bug in the function. Think for example of
the case that you want to test something during narrowing and be sure
that the buffer is not changed.

bug#810: 23.0.60; atomic-change-group narrowing problem

[martin rudalics]

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

 > I would suggest that this is a bug in the function. Think for example of
 > the case that you want to test something during narrowing and be sure
 > that the buffer is not changed.

Does the attached patch do what you want?

martin

[-- Attachment #2: 810.diff --]
[-- Type: text/plain, Size: 2200 bytes --]

*** subr.el.~1.602.~	2008-07-31 07:33:45.000000000 +0200
--- subr.el	2008-08-29 11:15:34.765625000 +0200
***************
*** 1991,2016 ****
    (dolist (elt handle)
      (with-current-buffer (car elt)
        (setq elt (cdr elt))
!       (let ((old-car
!              (if (consp elt) (car elt)))
!             (old-cdr
!              (if (consp elt) (cdr elt))))
!         ;; Temporarily truncate the undo log at ELT.
!         (when (consp elt)
!           (setcar elt nil) (setcdr elt nil))
!         (unless (eq last-command 'undo) (undo-start))
!         ;; Make sure there's no confusion.
!         (when (and (consp elt) (not (eq elt (last pending-undo-list))))
!           (error "Undoing to some unrelated state"))
!         ;; Undo it all.
!         (save-excursion
!           (while (listp pending-undo-list) (undo-more 1)))
!         ;; Reset the modified cons cell ELT to its original content.
!         (when (consp elt)
!           (setcar elt old-car)
!           (setcdr elt old-cdr))
!         ;; Revert the undo info to what it was when we grabbed the state.
!         (setq buffer-undo-list elt)))))
  \f
  ;;;; Display-related functions.
  
--- 1991,2018 ----
    (dolist (elt handle)
      (with-current-buffer (car elt)
        (setq elt (cdr elt))
!       (save-restriction
! 	(widen)
! 	(let ((old-car
! 	       (if (consp elt) (car elt)))
! 	      (old-cdr
! 	       (if (consp elt) (cdr elt))))
! 	  ;; Temporarily truncate the undo log at ELT.
! 	  (when (consp elt)
! 	    (setcar elt nil) (setcdr elt nil))
! 	  (unless (eq last-command 'undo) (undo-start))
! 	  ;; Make sure there's no confusion.
! 	  (when (and (consp elt) (not (eq elt (last pending-undo-list))))
! 	    (error "Undoing to some unrelated state"))
! 	  ;; Undo it all.
! 	  (save-excursion
! 	    (while (listp pending-undo-list) (undo-more 1)))
! 	  ;; Reset the modified cons cell ELT to its original content.
! 	  (when (consp elt)
! 	    (setcar elt old-car)
! 	    (setcdr elt old-cdr))
! 	  ;; Revert the undo info to what it was when we grabbed the state.
! 	  (setq buffer-undo-list elt))))))
  \f
  ;;;; Display-related functions.
  

bug#810: 23.0.60; atomic-change-group narrowing problem

[martin rudalics]

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

 > When using atomic-change-group I got the message
 >
 >   undo-more: Changes to be undone are outside visible portion of buffer
 >
 > The changes are also not undone. This seems wrong to me.

The attached patch now also tries to restore any narrowing in force
before the atomic change.

martin

[-- Attachment #2: 810.diff --]
[-- Type: text/plain, Size: 3329 bytes --]

*** subr.el.~1.604.~	2008-08-31 09:48:50.531250000 +0200
--- subr.el	2008-09-01 19:52:32.843750000 +0200
***************
*** 1907,1912 ****
--- 1907,1917 ----
  \f
  ;;; Atomic change groups.
  
+ (defvar atomic-change-point-min nil
+   "Value returned by `point-min' before an atomic change.")
+ (defvar atomic-change-point-max nil
+   "Value returned by `point-max' before an atomic change.")
+ 
  (defmacro atomic-change-group (&rest body)
    "Perform BODY as an atomic change group.
  This means that if BODY exits abnormally,
***************
*** 1920,1925 ****
--- 1925,1932 ----
    (let ((handle (make-symbol "--change-group-handle--"))
  	(success (make-symbol "--change-group-success--")))
      `(let ((,handle (prepare-change-group))
+ 	   (atomic-change-point-min (point-min))
+ 	   (atomic-change-point-max (point-max))
  	   ;; Don't truncate any undo data in the middle of this.
  	   (undo-outer-limit nil)
  	   (undo-limit most-positive-fixnum)
***************
*** 1992,2017 ****
    (dolist (elt handle)
      (with-current-buffer (car elt)
        (setq elt (cdr elt))
!       (let ((old-car
!              (if (consp elt) (car elt)))
!             (old-cdr
!              (if (consp elt) (cdr elt))))
!         ;; Temporarily truncate the undo log at ELT.
!         (when (consp elt)
!           (setcar elt nil) (setcdr elt nil))
!         (unless (eq last-command 'undo) (undo-start))
!         ;; Make sure there's no confusion.
!         (when (and (consp elt) (not (eq elt (last pending-undo-list))))
!           (error "Undoing to some unrelated state"))
!         ;; Undo it all.
!         (save-excursion
!           (while (listp pending-undo-list) (undo-more 1)))
!         ;; Reset the modified cons cell ELT to its original content.
!         (when (consp elt)
!           (setcar elt old-car)
!           (setcdr elt old-cdr))
!         ;; Revert the undo info to what it was when we grabbed the state.
!         (setq buffer-undo-list elt)))))
  \f
  ;;;; Display-related functions.
  
--- 1999,2030 ----
    (dolist (elt handle)
      (with-current-buffer (car elt)
        (setq elt (cdr elt))
!       (save-restriction
! 	(widen)
! 	(let ((old-car
! 	       (if (consp elt) (car elt)))
! 	      (old-cdr
! 	       (if (consp elt) (cdr elt))))
! 	  ;; Temporarily truncate the undo log at ELT.
! 	  (when (consp elt)
! 	    (setcar elt nil) (setcdr elt nil))
! 	  (unless (eq last-command 'undo) (undo-start))
! 	  ;; Make sure there's no confusion.
! 	  (when (and (consp elt) (not (eq elt (last pending-undo-list))))
! 	    (error "Undoing to some unrelated state"))
! 	  ;; Undo it all.
! 	  (save-excursion
! 	    (while (listp pending-undo-list) (undo-more 1)))
! 	  ;; Reset the modified cons cell ELT to its original content.
! 	  (when (consp elt)
! 	    (setcar elt old-car)
! 	    (setcdr elt old-cdr))
! 	  ;; Revert the undo info to what it was when we grabbed the state.
! 	  (setq buffer-undo-list elt)))
!       (unless (and (= (point-min) atomic-change-point-min)
! 		   (= (point-max) atomic-change-point-max))
! 	;; Try to restore narrowing in force before atomic change.
! 	(narrow-to-region atomic-change-point-min atomic-change-point-max)))))
  \f
  ;;;; Display-related functions.
  

bug#810: 23.0.60; atomic-change-group narrowing problem

[Stefan Monnier]

>> When using atomic-change-group I got the message
>> undo-more: Changes to be undone are outside visible portion of buffer
>> The changes are also not undone. This seems wrong to me.

> The attached patch now also tries to restore any narrowing in force
> before the atomic change.

I'm not sure we should do that.  Currently, narrowing operations are not
saved to the undo-log, so making atomic-change-group save&restore the
narrowing makes it behave differently from the undo.


        Stefan

bug#810: 23.0.60; atomic-change-group narrowing problem

[martin rudalics]

 >>> When using atomic-change-group I got the message
 >>> undo-more: Changes to be undone are outside visible portion of buffer
 >>> The changes are also not undone. This seems wrong to me.
 >
 >> The attached patch now also tries to restore any narrowing in force
 >> before the atomic change.
 >
 > I'm not sure we should do that.  Currently, narrowing operations are not
 > saved to the undo-log, so making atomic-change-group save&restore the
 > narrowing makes it behave differently from the undo.

I know what you mean but the present case is different.  When someone
uses narrowing within the atomic change group and we really want to undo
the changes (we still can decide that we won't fix the bug ;-)) we have
to widen the buffer.  If the buffer was narrowed _before_ entering the
atomic change group, it would be incorrect not to restore that.  Or am I
missing something?

martin

bug#810: 23.0.60; atomic-change-group narrowing problem

[Lennart Borgman (gmail)]

martin rudalics wrote:
>>>> When using atomic-change-group I got the message
>>>> undo-more: Changes to be undone are outside visible portion of buffer
>>>> The changes are also not undone. This seems wrong to me.
>>
>>> The attached patch now also tries to restore any narrowing in force
>>> before the atomic change.
>>
>> I'm not sure we should do that.  Currently, narrowing operations are not
>> saved to the undo-log, so making atomic-change-group save&restore the
>> narrowing makes it behave differently from the undo.
> 
> I know what you mean but the present case is different.  When someone
> uses narrowing within the atomic change group and we really want to undo
> the changes (we still can decide that we won't fix the bug ;-)) we have
> to widen the buffer.  If the buffer was narrowed _before_ entering the
> atomic change group, it would be incorrect not to restore that.  Or am I
> missing something?


I agree. It would be very surprising if narrowing was not restored.

bug#810: 23.0.60; atomic-change-group narrowing problem

[Stefan Monnier]

>>>>> When using atomic-change-group I got the message
>>>>> undo-more: Changes to be undone are outside visible portion of buffer
>>>>> The changes are also not undone. This seems wrong to me.
>>> 
>>>> The attached patch now also tries to restore any narrowing in force
>>>> before the atomic change.
>>> 
>>> I'm not sure we should do that.  Currently, narrowing operations are not
>>> saved to the undo-log, so making atomic-change-group save&restore the
>>> narrowing makes it behave differently from the undo.
>> 
>> I know what you mean but the present case is different.  When someone
>> uses narrowing within the atomic change group and we really want to undo
>> the changes (we still can decide that we won't fix the bug ;-)) we have
>> to widen the buffer.  If the buffer was narrowed _before_ entering the
>> atomic change group, it would be incorrect not to restore that.  Or am I
>> missing something?

The widening needs to be temporary around the call to undo, but that
doesn't seem to imply that atomic-change-group will preserve narrowing.


        Stefan

bug#810: 23.0.60; atomic-change-group narrowing problem

[martin rudalics]

 > The widening needs to be temporary around the call to undo, but that
 > doesn't seem to imply that atomic-change-group will preserve narrowing.

OK.  Installed without restoring the initial narrowing state.  Any
narrowing within the group will remain effective when the change is
canceled.

martin

bug#810: 23.0.60; atomic-change-group narrowing problem

[Richard M. Stallman]

    I know what you mean but the present case is different.  When someone
    uses narrowing within the atomic change group and we really want to undo
    the changes (we still can decide that we won't fix the bug ;-)) we have
    to widen the buffer.

We could say that if the body of the atomic-change-group does
narrowing, then it has the responsibility to widen again.
That seems logical.

bug#810: marked as done (23.0.60; atomic-change-group narrowing problem)

[Emacs bug Tracking System]

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


Your message dated Sun, 07 Sep 2008 12:02:38 +0200
with message-id <48C3A6BE.8030305@gmx.at>
and subject line Re: bug#810: 23.0.60; atomic-change-group narrowing problem
has caused the Emacs bug report #810,
regarding 23.0.60; atomic-change-group narrowing problem
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact don@donarmstrong.com
immediately.)


-- 
810: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=810
Emacs Bug Tracking System
Contact don@donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 2901 bytes --]

From: "Lennart Borgman (gmail)" <lennart.borgman@gmail.com>
To: emacs-pretest-bug@gnu.org
Subject: 23.0.60; atomic-change-group narrowing problem
Date: Thu, 28 Aug 2008 20:58:42 +0200
Message-ID: <48B6F562.9060006@gmail.com>

When using atomic-change-group I got the message

  undo-more: Changes to be undone are outside visible portion of buffer

The changes are also not undone. This seems wrong to me.

If I put (widen) before the end of my code in the atomic-change-group
the message above disappears and the change is undone.


In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-08-10
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags
-Ic:/g/include -fno-crossjumping'



[-- Attachment #3: Type: message/rfc822, Size: 1475 bytes --]

From: martin rudalics <rudalics@gmx.at>
To: 810-done@emacsbugs.donarmstrong.com
Cc: lennart.borgman@gmail.com
Subject: Re: bug#810: 23.0.60; atomic-change-group narrowing problem
Date: Sun, 07 Sep 2008 12:02:38 +0200
Message-ID: <48C3A6BE.8030305@gmx.at>

Fixed as

2008-09-07  Martin Rudalics  <rudalics@gmx.at>

	* subr.el (cancel-change-group): Widen buffer temporarily when
	undoing changes.  (Bug#810)

Thanks