add-face-text-property destructively modified properties

add-face-text-property destructively modified properties

[Lars Ingebrigtsen]

Up until yesterday, using this function on copies of strings would
modify the original string:

(progn
  (setq str1 (propertize "foo" 'face '(underline highlight)))
  (setq str2 (concat str1))
  (add-face-text-property 0 3 'foobar t str2)
  (pp str1 (current-buffer)))
->

#("foo" 0 3
  (face
   (underline highlight foobar)))

This didn't seem right to me, so I modified it to not do that, but only
modify str2 when I told it to modify str2.

Eli wanted comments about whether anybody thinks this is a surprising
change.

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

Re: add-face-text-property destructively modified properties

[Stefan Monnier]

> Up until yesterday, using this function on copies of strings would
> modify the original string:

Indeed.  There was an open bug report about that (sorry, don't have the
bug-nb at hand).

> Eli wanted comments about whether anybody thinks this is a surprising
> change.

I think it was a plain bug.  The old behavior was
definitely surprising. 

What is surprising about the change is that it took so long to come ;-)


        Stefan

Re: add-face-text-property destructively modified properties

[Lars Ingebrigtsen]

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

>> Up until yesterday, using this function on copies of strings would
>> modify the original string:
>
> Indeed.  There was an open bug report about that (sorry, don't have the
> bug-nb at hand).

Yup; it was that bug report that made me change the behaviour.

>> Eli wanted comments about whether anybody thinks this is a surprising
>> change.
>
> I think it was a plain bug.  The old behavior was
> definitely surprising. 
>
> What is surprising about the change is that it took so long to come ;-)

When I added add-face-text-property, it was for shr, which only uses it
for buffers where this isn't a problem (I think...?), so I probably
didn't even consider how this would work in strings.

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

Re: add-face-text-property destructively modified properties

[Stefan Monnier]

> When I added add-face-text-property, it was for shr, which only uses it
> for buffers where this isn't a problem (I think...?),

I'm pretty sure the problem can occur in buffers as well.
E.g. if you (put-text-property X Y 'face '(a b c d)) at variable places
(hence using the exact same list object as `face` property at various
places) and then use add-face-text-property on some of those places.
[ both within a given buffer and between different buffers.  ]


        Stefan

Re: add-face-text-property destructively modified properties

[Lars Ingebrigtsen]

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

>> When I added add-face-text-property, it was for shr, which only uses it
>> for buffers where this isn't a problem (I think...?),
>
> I'm pretty sure the problem can occur in buffers as well.
> E.g. if you (put-text-property X Y 'face '(a b c d)) at variable places
> (hence using the exact same list object as `face` property at various
> places) and then use add-face-text-property on some of those places.
> [ both within a given buffer and between different buffers.  ]

Ah, yes, that's true.  Mixing and matching put-text-property with
add-face-text-property could also lead to similar problems as in
strings.

But can `buffer-substring' give us the same problems, or are the text
properties deep-copied in that case?

(I've now changed add-face-text-property to be non-destructive when
changing buffer face properties, too.)

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

Re: add-face-text-property destructively modified properties

[Stefan Monnier]

> But can `buffer-substring' give us the same problems, or are the text
> properties deep-copied in that case?

I don't think they *can* be deep-copied (at least not without breaking
code).


        Stefan

Re: add-face-text-property destructively modified properties

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 09 Oct 2019 20:14:26 +0200
> 
> (progn
>   (setq str1 (propertize "foo" 'face '(underline highlight)))
>   (setq str2 (concat str1))
>   (add-face-text-property 0 3 'foobar t str2)
>   (pp str1 (current-buffer)))
> ->
> 
> #("foo" 0 3
>   (face
>    (underline highlight foobar)))
> 
> This didn't seem right to me, so I modified it to not do that, but only
> modify str2 when I told it to modify str2.
> 
> Eli wanted comments about whether anybody thinks this is a surprising
> change.

Mostly whether people think this could break something, as we had this
behavior for quite some time, I think.

Re: add-face-text-property destructively modified properties

[Stefan Monnier]

> Mostly whether people think this could break something, as we had this
> behavior for quite some time, I think.

It's always possible, of course, but I'd be *very* surprised.


        Stefan