* bug#13648: 24.3.50; remove-overlays bugs
@ 2013-02-07 15:09 Stephen Berman
2013-02-07 16:42 ` Stefan Monnier
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Berman @ 2013-02-07 15:09 UTC (permalink / raw)
To: 13648
[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]
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@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)
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: remove-overlays patch --]
[-- Type: text/x-patch, Size: 2629 bytes --]
=== 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)))))))
\f
;;;; 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))))))))
\f
;;;; Miscellanea.
[-- Attachment #3: Type: text/plain, Size: 557 bytes --]
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@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2013-02-07 15:09 bug#13648: 24.3.50; remove-overlays bugs Stephen Berman
@ 2013-02-07 16:42 ` Stefan Monnier
2013-02-07 19:16 ` Stephen Berman
2020-08-25 11:44 ` Lars Ingebrigtsen
0 siblings, 2 replies; 9+ messages in thread
From: Stefan Monnier @ 2013-02-07 16:42 UTC (permalink / raw)
To: Stephen Berman; +Cc: 13648
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2013-02-07 16:42 ` Stefan Monnier
@ 2013-02-07 19:16 ` Stephen Berman
2013-02-08 1:45 ` Stefan Monnier
2020-08-25 11:44 ` Lars Ingebrigtsen
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Berman @ 2013-02-07 19:16 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 13648
On Thu, 07 Feb 2013 11:42:46 -0500 Stefan Monnier <monnier@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2013-02-07 19:16 ` Stephen Berman
@ 2013-02-08 1:45 ` Stefan Monnier
2013-02-08 14:50 ` Stephen Berman
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2013-02-08 1:45 UTC (permalink / raw)
To: Stephen Berman; +Cc: 13648
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2013-02-08 1:45 ` Stefan Monnier
@ 2013-02-08 14:50 ` Stephen Berman
0 siblings, 0 replies; 9+ messages in thread
From: Stephen Berman @ 2013-02-08 14:50 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 13648
On Thu, 07 Feb 2013 20:45:32 -0500 Stefan Monnier <monnier@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2013-02-07 16:42 ` Stefan Monnier
2013-02-07 19:16 ` Stephen Berman
@ 2020-08-25 11:44 ` Lars Ingebrigtsen
2020-08-25 13:02 ` Eli Zaretskii
2020-08-25 13:55 ` Stefan Monnier
1 sibling, 2 replies; 9+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-25 11:44 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Stephen Berman, 13648
Stefan Monnier <monnier@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2020-08-25 11:44 ` Lars Ingebrigtsen
@ 2020-08-25 13:02 ` Eli Zaretskii
2020-08-25 13:55 ` Stefan Monnier
1 sibling, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2020-08-25 13:02 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: stephen.berman, monnier, 13648
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 25 Aug 2020 13:44:55 +0200
> Cc: Stephen Berman <stephen.berman@gmx.net>, 13648@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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2020-08-25 11:44 ` Lars Ingebrigtsen
2020-08-25 13:02 ` Eli Zaretskii
@ 2020-08-25 13:55 ` Stefan Monnier
2020-08-25 14:23 ` Lars Ingebrigtsen
1 sibling, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2020-08-25 13:55 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: Stephen Berman, 13648
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#13648: 24.3.50; remove-overlays bugs
2020-08-25 13:55 ` Stefan Monnier
@ 2020-08-25 14:23 ` Lars Ingebrigtsen
0 siblings, 0 replies; 9+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-25 14:23 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Stephen Berman, 13648
Stefan Monnier <monnier@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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-25 14:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-07 15:09 bug#13648: 24.3.50; remove-overlays bugs Stephen Berman
2013-02-07 16:42 ` Stefan Monnier
2013-02-07 19:16 ` Stephen Berman
2013-02-08 1:45 ` Stefan Monnier
2013-02-08 14:50 ` Stephen Berman
2020-08-25 11:44 ` Lars Ingebrigtsen
2020-08-25 13:02 ` Eli Zaretskii
2020-08-25 13:55 ` Stefan Monnier
2020-08-25 14:23 ` Lars Ingebrigtsen
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).