GNU bug report logs - #13648
24.3.50; remove-overlays bugs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13648 in the body.
You can then email your comments to 13648 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3.50; remove-overlays bugs
Date: Thu, 07 Feb 2013 16:09:21 +0100

[Message part 1 (text/plain, inline)]

0. emacs -Q
1. Evaluate the following sexp:

   (progn
     (switch-to-buffer (get-buffer-create "*test*"))
     (insert "one two three")
     (setq ov1 (make-overlay 1 4))
     (setq ov2 (make-overlay 5 9))
     (setq ov3 (make-overlay 9 14))
     (sit-for 1)
     (overlay-put ov1 'display "two")
     (overlay-put ov2 'display "")
     (overlay-put ov3 'after-string " four")
     (sit-for 1)
     (remove-overlays nil nil 'display "two")
     (remove-overlays nil nil 'display "")
     (sit-for 1)
     (remove-overlays nil nil 'after-string))

In buffer *test* you see the following:

1. First:

one two three

2. Then after one second:

two three four

3. Then after another second:

two two three four

4. Finally, after one more second:

one two three four


I think the last two displays demonstrate buggy behavior.  In the third,
remove-overlays has failed to clear the display string "two" but has
cleared the empty string.  In the fourth, the remove-overlays call
specified the after-string property, yet what has been cleared is the
display overlay "two" but not the after-string overlay "four".  

The reason for the first problem is that remove-overlays tests the
overlay value with eq, which fails for all strings except the empty
string.  Hence, both the display overlay "two" and the after-string
overlay "four" are not cleared by the second and third calls of
remove-overlays, respectively.

But the third call does remove "two" because overlay-get tries to get
the value of the overlay's after-string property, but it only has a
display property, so overlay-get returns nil, and since the fourth
argument of remove-overlays is also nil here, they are eq, so the
overlay is cleared.  The general problem here, I believe, is that,
although all the arguments of remove-overlays are optional (so the last
invocation of remove-overlays is legitimate), the logic of the code is
that the NAME and VAL arguments are either both nil or both non-nil,
which conflicts with the semantics of the &optional keyword.

I think the easiest and best fix is to make the NAME and VAL arguments
conform to the &optional semantics by allowing independent values.  This
means that the last call of remove-overlays in the above sexp would
clear any after-string overlays, regardless of their value.  I think
this would be useful, and it is backward compatible, because all uses of
remove-overlays in Emacs have either both or neither of the NAME and VAL
arguments (and any third-party code that only has the NAME argument is
either buggy, like the above sexp, or works by chance).  The patch below
implements this, and also fixes the first problem by testing with equal
if the eq test fails.


2013-02-07  Stephen Berman  <stephen.berman <at> gmx.net>

	* subr.el (remove-overlays): Handle string and list values of
	overlay properties.  If the property argument is non-nil and the
	value argument is nil, clear all overlays with that property
	regardless of their values.  (Bug#XXXXX)

[Message part 2 (text/x-patch, inline)]

=== modified file 'lisp/subr.el'
*** lisp/subr.el	2013-02-03 16:13:36 +0000
--- lisp/subr.el	2013-02-07 14:45:09 +0000
***************
*** 2579,2585 ****
  (defun remove-overlays (&optional beg end name val)
    "Clear BEG and END of overlays whose property NAME has value VAL.
  Overlays might be moved and/or split.
! BEG and END default respectively to the beginning and end of buffer."
    ;; This speeds up the loops over overlays.
    (unless beg (setq beg (point-min)))
    (unless end (setq end (point-max)))
--- 2579,2589 ----
  (defun remove-overlays (&optional beg end name val)
    "Clear BEG and END of overlays whose property NAME has value VAL.
  Overlays might be moved and/or split.
! 
! BEG and END default respectively to the beginning and end of
! buffer.  If VAL is nil and NAME is non-nil, clear all NAME
! overlays regardless of their values.  If both NAME and VAL are
! nil, clear all overlays from BEG to END."
    ;; This speeds up the loops over overlays.
    (unless beg (setq beg (point-min)))
    (unless end (setq end (point-max)))
***************
*** 2588,2607 ****
        (setq beg (prog1 end (setq end beg))))
    (save-excursion
      (dolist (o (overlays-in beg end))
!       (when (eq (overlay-get o name) val)
! 	;; Either push this overlay outside beg...end
! 	;; or split it to exclude beg...end
! 	;; or delete it entirely (if it is contained in beg...end).
! 	(if (< (overlay-start o) beg)
  	    (if (> (overlay-end o) end)
! 		(progn
! 		  (move-overlay (copy-overlay o)
! 				(overlay-start o) beg)
! 		  (move-overlay o end (overlay-end o)))
! 	      (move-overlay o (overlay-start o) beg))
! 	  (if (> (overlay-end o) end)
! 	      (move-overlay o end (overlay-end o))
! 	    (delete-overlay o)))))))
  
  ;;;; Miscellanea.
  
--- 2592,2614 ----
        (setq beg (prog1 end (setq end beg))))
    (save-excursion
      (dolist (o (overlays-in beg end))
!       (let ((v (overlay-get o name)))
! 	;; An overlay property value can be not just a symbol,
! 	;; but also a string or a list.
! 	(when (if val (or (eq v val) (equal v val)) name)
! 	  ;; Either push this overlay outside beg...end
! 	  ;; or split it to exclude beg...end
! 	  ;; or delete it entirely (if it is contained in beg...end).
! 	  (if (< (overlay-start o) beg)
! 	      (if (> (overlay-end o) end)
! 		  (progn
! 		    (move-overlay (copy-overlay o)
! 				  (overlay-start o) beg)
! 		    (move-overlay o end (overlay-end o)))
! 		(move-overlay o (overlay-start o) beg))
  	    (if (> (overlay-end o) end)
! 		(move-overlay o end (overlay-end o))
! 	      (delete-overlay o))))))))
  
  ;;;; Miscellanea.
  

[Message part 3 (text/plain, inline)]

In GNU Emacs 24.3.50.6 (x86_64-suse-linux-gnu, GTK+ Version 3.4.4)
 of 2013-02-07 on rosalinde
Bzr revision: 111689 michael.albinus <at> gmx.de-20130207085004-ztc6mdtkh756peam
Windowing system distributor `The X.Org Foundation', version 11.0.11203000
System Description:	openSUSE 12.2 (x86_64)

Configured using:
 `configure --without-toolkit-scroll-bars CFLAGS=-g3 -O0 --no-create
 --no-recursion'

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Message #8 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Thu, 07 Feb 2013 11:42:46 -0500

> The reason for the first problem is that remove-overlays tests the
> overlay value with eq, which fails for all strings [...]

No, it won't fail on all strings.  You just need to pass it the same
string you added to the overlay, rather than a copy of it.
I.e. this is not a bug.

> invocation of remove-overlays is legitimate), the logic of the code is
> that the NAME and VAL arguments are either both nil or both non-nil,

Indeed.

> which conflicts with the semantics of the &optional keyword.

Right.  We should document it in the docstring.

> This means that the last call of remove-overlays in the above sexp
> would clear any after-string overlays, regardless of their value.

Normally we don't distinguish "an property FOO of value nil" and "no
property FOO".  So I think what would make sense is to say that if VAL
is nil, then we remove any overlay whose NAME property is non-nil
(i.e. the exact inverse from what we currently do).

This said, the reason why I have not implemented this case of NAME being
specified while VAL is left unspecified is because I haven't come up
with a need for it.  So I'd be interested to hear the backstory of
why/where you need it.


        Stefan

Message #11 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Thu, 07 Feb 2013 20:16:07 +0100

On Thu, 07 Feb 2013 11:42:46 -0500 Stefan Monnier <monnier <at> IRO.UMontreal.CA> wrote:

>> The reason for the first problem is that remove-overlays tests the
>> overlay value with eq, which fails for all strings [...]
>
> No, it won't fail on all strings.  You just need to pass it the same
> string you added to the overlay, rather than a copy of it.
> I.e. this is not a bug.

Damn, I get tripped up by that a lot.  And the fact that the empty
string is an exception doesn't help to keep the difference in mind.
Still, it is rather cumbersome to include appropriate let-bindings or
calls to overlay-get for each string value in each use of
remove-overlays, as opposed to adding a single equal-check to that.

>> invocation of remove-overlays is legitimate), the logic of the code is
>> that the NAME and VAL arguments are either both nil or both non-nil,
>
> Indeed.
>
>> which conflicts with the semantics of the &optional keyword.
>
> Right.  We should document it in the docstring.
>
>> This means that the last call of remove-overlays in the above sexp
>> would clear any after-string overlays, regardless of their value.
>
> Normally we don't distinguish "an property FOO of value nil" and "no
> property FOO".  So I think what would make sense is to say that if VAL
> is nil, then we remove any overlay whose NAME property is non-nil
> (i.e. the exact inverse from what we currently do).
>
> This said, the reason why I have not implemented this case of NAME being
> specified while VAL is left unspecified is because I haven't come up
> with a need for it.  So I'd be interested to hear the backstory of
> why/where you need it.

To be honest, I'm not sure I do need it.  I have code that inserts
before-string overlays with different values, some fixed and some
dynamically generated, and when these overlays need to be cleared, it
would be easier to just refer to the before-string property.  But the
way I use the overlays, it actually suffices to leave out both the
property and the value, i.e. just remove all overlays in the region.  At
first I thought that's not safe enough, because there could be other
other overlays at the same locations but with different properties, but
in fact I haven't needed that yet.  But I guess the main thing that
confused me was the conflict with the semantics of &optional, and since
it seems easy enough to avoid, I think it would be better than just
documenting the conflict.

Steve Berman

Message #14 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Thu, 07 Feb 2013 20:45:32 -0500

> Damn, I get tripped up by that a lot.  And the fact that the empty
> string is an exception doesn't help to keep the difference in mind.
> Still, it is rather cumbersome to include appropriate let-bindings or
> calls to overlay-get for each string value in each use of
> remove-overlays, as opposed to adding a single equal-check to that.

Generally the best solution is very different: add another property to
every overlay.  E.g. smerge adds the property `smerge' with values like
`conflict' or `refine', so you can then (remove-overlays beg end
'smerge 'refine) without caring about particular values of
`after-string' or any other property.


        Stefan

Message #17 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Fri, 08 Feb 2013 15:50:45 +0100

On Thu, 07 Feb 2013 20:45:32 -0500 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> Damn, I get tripped up by that a lot.  And the fact that the empty
>> string is an exception doesn't help to keep the difference in mind.
>> Still, it is rather cumbersome to include appropriate let-bindings or
>> calls to overlay-get for each string value in each use of
>> remove-overlays, as opposed to adding a single equal-check to that.
>
> Generally the best solution is very different: add another property to
> every overlay.  E.g. smerge adds the property `smerge' with values like
> `conflict' or `refine', so you can then (remove-overlays beg end
> 'smerge 'refine) without caring about particular values of
> `after-string' or any other property.

Thanks, that sounds like a good strategy; I'll try it.

Steve Berman

Message #20 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: Stephen Berman <stephen.berman <at> gmx.net>, 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Tue, 25 Aug 2020 13:44:55 +0200

Stefan Monnier <monnier <at> IRO.UMontreal.CA> writes:

>> invocation of remove-overlays is legitimate), the logic of the code is
>> that the NAME and VAL arguments are either both nil or both non-nil,
>
> Indeed.
>
>> which conflicts with the semantics of the &optional keyword.
>
> Right.  We should document it in the docstring.

Does the patch below look OK?

>> This means that the last call of remove-overlays in the above sexp
>> would clear any after-string overlays, regardless of their value.
>
> Normally we don't distinguish "an property FOO of value nil" and "no
> property FOO".  So I think what would make sense is to say that if VAL
> is nil, then we remove any overlay whose NAME property is non-nil
> (i.e. the exact inverse from what we currently do).
>
> This said, the reason why I have not implemented this case of NAME being
> specified while VAL is left unspecified is because I haven't come up
> with a need for it.  So I'd be interested to hear the backstory of
> why/where you need it.

The case here is (remove-overlay beg end 'foo), which will remove all
overlays that don't have foo (as well as the ones that have foo, but
it's set to nil)?  

diff --git a/lisp/subr.el b/lisp/subr.el
index a58a873a33..bd50c52552 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3073,7 +3073,10 @@ copy-overlay
 (defun remove-overlays (&optional beg end name val)
   "Clear BEG and END of overlays whose property NAME has value VAL.
 Overlays might be moved and/or split.
-BEG and END default respectively to the beginning and end of buffer."
+
+BEG and END default respectively to the beginning and end of buffer.
+Values are compared with `eq'.
+If either NAME or VAL are specified, both should be specified."
   ;; This speeds up the loops over overlays.
   (unless beg (setq beg (point-min)))
   (unless end (setq end (point-max)))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #23 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: stephen.berman <at> gmx.net, monnier <at> IRO.UMontreal.CA, 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Tue, 25 Aug 2020 16:02:01 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Tue, 25 Aug 2020 13:44:55 +0200
> Cc: Stephen Berman <stephen.berman <at> gmx.net>, 13648 <at> debbugs.gnu.org
> 
>  (defun remove-overlays (&optional beg end name val)
>    "Clear BEG and END of overlays whose property NAME has value VAL.
            ^^^^^^^^^^^^^^^^^^^^^^^
Please also fix this confusing wording, while at that.

Thanks.

Message #26 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Stephen Berman <stephen.berman <at> gmx.net>, 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Tue, 25 Aug 2020 09:55:43 -0400

>> This said, the reason why I have not implemented this case of NAME being
>> specified while VAL is left unspecified is because I haven't come up
>> with a need for it.  So I'd be interested to hear the backstory of
>> why/where you need it.
> The case here is (remove-overlay beg end 'foo), which will remove all
> overlays that don't have foo (as well as the ones that have foo, but
> it's set to nil)?  

I think the case you describe is indeed never useful, but it *is*
implemented, AFAIK.

The more useful interpretation I think would be for
(remove-overlay beg end 'foo) to remove all overlays that have a non-nil
value of `foo`.  But I haven't implemented it because (despite my
impression that it would be more useful) I haven't actually found any
need for it.

> diff --git a/lisp/subr.el b/lisp/subr.el
> index a58a873a33..bd50c52552 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -3073,7 +3073,10 @@ copy-overlay
>  (defun remove-overlays (&optional beg end name val)
>    "Clear BEG and END of overlays whose property NAME has value VAL.

As Eli points out, I wasn't very inspired when I wrote this first line.
We should at very least replace "BEG and END" with "BEG...END" or
something like that.

> +BEG and END default respectively to the beginning and end of buffer.
> +Values are compared with `eq'.
> +If either NAME or VAL are specified, both should be specified."

LGTM,


        Stefan

Message #33 received at 13648 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Stephen Berman <stephen.berman <at> gmx.net>, 13648 <at> debbugs.gnu.org
Subject: Re: bug#13648: 24.3.50; remove-overlays bugs
Date: Tue, 25 Aug 2020 16:23:26 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> The case here is (remove-overlay beg end 'foo), which will remove all
>> overlays that don't have foo (as well as the ones that have foo, but
>> it's set to nil)?  
>
> I think the case you describe is indeed never useful, but it *is*
> implemented, AFAIK.
>
> The more useful interpretation I think would be for
> (remove-overlay beg end 'foo) to remove all overlays that have a non-nil
> value of `foo`.  But I haven't implemented it because (despite my
> impression that it would be more useful) I haven't actually found any
> need for it.

Yeah, and perhaps that'll break something...   In any case, it's
probably not worth mentioning in the doc string.  :/

>>  (defun remove-overlays (&optional beg end name val)
>>    "Clear BEG and END of overlays whose property NAME has value VAL.
>
> As Eli points out, I wasn't very inspired when I wrote this first line.
> We should at very least replace "BEG and END" with "BEG...END" or
> something like that.

Yup.  I've added more text there.

>> +BEG and END default respectively to the beginning and end of buffer.
>> +Values are compared with `eq'.
>> +If either NAME or VAL are specified, both should be specified."
>
> LGTM,

Pushed to the trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

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