GNU bug report logs - #16818
Undo in region after markers in undo history relocated

Previous Next

Full log

View this message in rfc822 format

From: Stefan <monnier <at> IRO.UMontreal.CA>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: bug#16818: Acknowledgement (Undo in region after markers in undo history relocated)
Date: Mon, 24 Mar 2014 09:03:50 -0400

> Attached is the new patch to solve this, implementing option 2 and
> incorporating your feedback.

Looks good, thank you very much.  Feel free to install into emacs-24,
but please see my comments below.


        Stefan


> +2014-03-13  Barry O'Reilly  <gundaetiapo <at> gmail.com>
> +	* markers.texi (Moving Marker Positions): The 2014-03-02 doc

Missing empty line between the two.

> +             (while (and (consp (car list))
> +                         (markerp (caar list))

(markerp (car-safe (car list)))

> +               (and (eq (marker-buffer (caar list))
> +                        (current-buffer))
> +                    (integerp (marker-position (caar list)))

I think this `integerp' test is redundant (marker-position can only
return nil or an integer and if it returns nil, then marker-buffer also
returns nil).

>            ;; (MARKER . OFFSET) means a marker MARKER was adjusted by OFFSET.
>            (`(,(and marker (pred markerp)) . ,(and offset (pred integerp)))
> -           (when (marker-buffer marker)
> -             (set-marker marker
> -                         (- marker offset)
> -                         (marker-buffer marker))))
> +           (warn "Encountered %S entry in undo list with no matching (TEXT . POS) entry"
> +                 next))

I think I'd add the warning without removing the `set-marker', just to
be conservative.

> +                (when (and (consp undo-elt)
> +                           (stringp (car undo-elt))

(stringp (car-safe undo-elt))

> +                           (integerp (cdr undo-elt)))
> +                  (let ((list-i (cdr undo-list-copy)))
> +                    (while (markerp (caar list-i))

I think you need (markerp (car-safe (car list-i))) because (car list-i)
may be an integer.

Also, I'd try to avoid repeatedly calling (car list-i) in the body of
the `while' loop(s) by let-binding it.  I'd probably do it via `pop', as
in

  (while (...)
    (let ((elem (pop undo-list)))
      ...))

>  	((and (consp undo-elt) (markerp (car undo-elt)))
> -	 ;; This is a marker-adjustment element (MARKER . ADJUSTMENT).
> -	 ;; See if MARKER is inside the region.
> -	 (let ((alist-elt (assq (car undo-elt) undo-adjusted-markers)))
> -	   (unless alist-elt
> -	     (setq alist-elt (cons (car undo-elt)
> -				   (marker-position (car undo-elt))))
> -	     (setq undo-adjusted-markers
> -		   (cons alist-elt undo-adjusted-markers)))
> -	   (and (cdr alist-elt)
> -		(>= (cdr alist-elt) start)
> -		(<= (cdr alist-elt) end))))
> +	 ;; (MARKER . ADJUSTMENT)
> +         nil)

It would also be more conservative to keep this unchanged, but I think
I agree with you here that we don't need to be *that* conservative.

> -  record_delete (beg, make_buffer_string (beg, beg + length, 1));
> +  record_delete (beg, make_buffer_string (beg, beg + length, 1), false);

Begs the question: why didn't (don't) we record marker adjustments here
(and other similar places)?


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.