GNU bug report logs - #53294
29.0.50; Indirect font changes incorrectly affecting original buffer

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 53294 in the body.
You can then email your comments to 53294 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: Andrew Hyatt <ahyatt <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Indirect font changes incorrectly affecting original buffer
Date: Sun, 16 Jan 2022 00:13:26 -0500

[text-scale-patch (application/octet-stream, attachment)]

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

Hi all,

I noticed a bug recently where if I scale up fonts in an org 
capture buffer, it affects the original buffer, which keeps 
getting bigger and bigger every time org-capture is run.

However, you don't need org to reproduce this.  Here's a quick way 
to reproduce, which works with emacs -Q:

(require 'face-remap) (defun ash/big-font () 
 "Creates a font that is big enough for about 20 lines of text." 
 (interactive) (let ((text-scale-mode-amount (/ (frame-height) 
 20))) 
   (text-scale-mode 1))) 

(defun ash/reproduce-with-indirect-buffer () 
 (interactive) (let ((buf (get-buffer-create "*Orig buffer*"))) 
   (set-buffer buf) (variable-pitch-mode 1) ;; same way org mode 
   creates indirect buffer (set-buffer (make-indirect-buffer buf 
   "*Indirect buffer*" 'clone)) (ash/big-font-new) (kill-buffer 
   (current-buffer)))) 

Running ash/reproduce-with-indirect-buffer will increase the 
indirect buffer in size each time.  If you look at 
face-remapping-alist, it's clear that the original buffer's value 
is being altered by the indirect buffer.

I traced this down to the setcdr in face-remove-add-relative, 
which is incorrectly alterting a buffer local variable, by setting 
parts of a variable in a way that will affect the original 
variable.  Evidently making something buffer local doesn't clone 
it, it just copies the value, and so you can alter the original by 
operations such as setcdr. 

Assuming that the fixing how variables becomes buffer local is too 
much, I've fixed this in face-remap.el.  The patch is attached.


In GNU Emacs 29.0.50 (build 1, aarch64-apple-darwin21.2.0, NS 
appkit-2113.20 Version 12.1 (Build 21C52)) 
of 2021-12-26 built on andrews-mbp.lan 
Repository revision: 978987f7ad58cd66fe51cefde53ba4771b189aeb 
Repository branch: master Windowing system distributor 'Apple', 
version 10.3.2113 System Description:  macOS 12.1

Configured using:
'configure --with-native-
Configured features:
ACL GLIB GNUTLS JSON LCMS2 LIBXML2 MODULES NATIVE_COMP NOTIFY 
KQUEUE NS
PDUMPER PNG RSVG SQLITE3 THREADS TOOLKIT_SCROLL_BARS XIM ZLIB
Important settings:
 value of $LANG: en_US.UTF-8
  
locale-coding-system: utf-8-unix

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50;
 Indirect font changes incorrectly affecting original buffer
Date: Sun, 16 Jan 2022 11:22:27 +0200

> From: Andrew Hyatt <ahyatt <at> gmail.com>
> Date: Sun, 16 Jan 2022 00:13:26 -0500
> 
> I noticed a bug recently where if I scale up fonts in an org 
> capture buffer, it affects the original buffer, which keeps 
> getting bigger and bigger every time org-capture is run.
> 
> However, you don't need org to reproduce this.  Here's a quick way 
> to reproduce, which works with emacs -Q:
> 
> (require 'face-remap) (defun ash/big-font () 
>   "Creates a font that is big enough for about 20 lines of text." 
>   (interactive) (let ((text-scale-mode-amount (/ (frame-height) 
>   20))) 
>     (text-scale-mode 1))) 
>  
> (defun ash/reproduce-with-indirect-buffer () 
>   (interactive) (let ((buf (get-buffer-create "*Orig buffer*"))) 
>     (set-buffer buf) (variable-pitch-mode 1) ;; same way org mode 
>     creates indirect buffer (set-buffer (make-indirect-buffer buf 
>     "*Indirect buffer*" 'clone)) (ash/big-font-new) (kill-buffer 
>     (current-buffer)))) 
> 
> Running ash/reproduce-with-indirect-buffer will increase the 
> indirect buffer in size each time.  If you look at 
> face-remapping-alist, it's clear that the original buffer's value 
> is being altered by the indirect buffer.

I don't think I understand what I should see and pay attention to with
this recipe (and it includes several errors that took me some time to
fix, before I could run it), but isn't this because you used non-nil
CLONE argument to make-indirect-buffer?

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 09:43:16 -0500

On Sun, Jan 16, 2022 at 11:22 AM Eli Zaretskii <eliz <at> gnu.org> 
wrote: 

>> From: Andrew Hyatt <ahyatt <at> gmail.com> Date: Sun, 16 Jan 2022 
>> 00:13:26 -0500  I noticed a bug recently where if I scale up 
>> fonts in an org  capture buffer, it affects the original 
>> buffer, which keeps  getting bigger and bigger every time 
>> org-capture is run.   However, you don't need org to reproduce 
>> this.  Here's a quick way  to reproduce, which works with emacs 
>> -Q:  (require 'face-remap) (defun ash/big-font ()  
>>   "Creates a font that is big enough for about 20 lines of 
>>   text."  (interactive) (let ((text-scale-mode-amount (/ 
>>   (frame-height)  20)))  
>>     (text-scale-mode 1)))  
>>   
>> (defun ash/reproduce-with-indirect-buffer ()  
>>   (interactive) (let ((buf (get-buffer-create "*Orig 
>>   buffer*")))  
>>     (set-buffer buf) (variable-pitch-mode 1) ;; same way org 
>>     mode  creates indirect buffer (set-buffer 
>>     (make-indirect-buffer buf  "*Indirect buffer*" 'clone)) 
>>     (ash/big-font-new) (kill-buffer  (current-buffer))))  
>>  Running ash/reproduce-with-indirect-buffer will increase the 
>> indirect buffer in size each time.  If you look at 
>> face-remapping-alist, it's clear that the original buffer's 
>> value  is being altered by the indirect buffer. 
> 
> I don't think I understand what I should see and pay attention 
> to with this recipe (and it includes several errors that took me 
> some time to fix, before I could run it), but isn't this because 
> you used non-nil CLONE argument to make-indirect-buffer? 


Sorry for any errors, I simplified it a bit before I sent it out 
and perhaps made a few mistakes. What you should see, if you go 
the *Orig Buffer*, is that the font scale keeps increasing. If you 
look at the face-remapping-alist of that buffer, it will be 
growing with duplicate height specs, one for every time you called 
the ash/reproduce-with-indirect-buffer.

Yes, the non-nil CLONE argument is essential to reproduce. But 
that just means the indirect buffer will inherit the state from 
the direct buffer.  It doesn't imply that changes made to the 
indirect buffer's state will affect the original buffer, which is 
what is happening here.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 17:00:38 +0200

> From: Andrew Hyatt <ahyatt <at> gmail.com>
> Cc: 53294 <at> debbugs.gnu.org
> Date: Sun, 16 Jan 2022 09:43:16 -0500
> 
> Yes, the non-nil CLONE argument is essential to reproduce. But 
> that just means the indirect buffer will inherit the state from 
> the direct buffer.  It doesn't imply that changes made to the 
> indirect buffer's state will affect the original buffer, which is 
> what is happening here.

But "inheriting" the state means the indirect buffer gets the copy of
the variables of the original buffer, and that is not a deep copy,
AFAIU.

Stefan, any comments on this issue?

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 10:28:40 -0500

On Sun, Jan 16, 2022 at 05:00 PM Eli Zaretskii <eliz <at> gnu.org> 
wrote: 

>> From: Andrew Hyatt <ahyatt <at> gmail.com> Cc: 53294 <at> debbugs.gnu.org 
>> Date: Sun, 16 Jan 2022 09:43:16 -0500  Yes, the non-nil CLONE 
>> argument is essential to reproduce. But  that just means the 
>> indirect buffer will inherit the state from  the direct buffer. 
>> It doesn't imply that changes made to the  indirect buffer's 
>> state will affect the original buffer, which is  what is 
>> happening here. 
> 
> But "inheriting" the state means the indirect buffer gets the 
> copy of the variables of the original buffer, and that is not a 
> deep copy, AFAIU. 
> 
> Stefan, any comments on this issue? 

Sorry, I may have been unclear. I'm not disagreeing - what you 
just said is correct. But, because of that, it's a bug for code to 
make an indirect buffer, then perform operations on it via setcdr 
or setf like things, which then will affect the original buffer's 
variables.

Alternatively, it'd perhaps would be a better fix to make the 
indirect buffer's copy a deep copy instead of the shallow copy it 
is now, so that these situations don't arise. I don't have a patch 
for that, though, but could create one if you thought it was a 
good idea.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 53294 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 17:41:53 +0200

> From: Andrew Hyatt <ahyatt <at> gmail.com>
> Cc: 53294 <at> debbugs.gnu.org
> Date: Sun, 16 Jan 2022 10:28:40 -0500
> 
> > But "inheriting" the state means the indirect buffer gets the 
> > copy of the variables of the original buffer, and that is not a 
> > deep copy, AFAIU. 
> > 
> > Stefan, any comments on this issue? 
> 
> Sorry, I may have been unclear. I'm not disagreeing - what you 
> just said is correct. But, because of that, it's a bug for code to 
> make an indirect buffer, then perform operations on it via setcdr 
> or setf like things, which then will affect the original buffer's 
> variables.

I'm not sure it's a bug.  It could be a "feature", not limited to
face-remapping-alist, but instead affecting every buffer-local
variable that is not a simple scalar.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Andrew Hyatt <ahyatt <at> gmail.com>, 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 12:11:39 -0500

>> > But "inheriting" the state means the indirect buffer gets the 
>> > copy of the variables of the original buffer, and that is not a 
>> > deep copy, AFAIU. 
>> > Stefan, any comments on this issue? 
>> Sorry, I may have been unclear. I'm not disagreeing - what you 
>> just said is correct. But, because of that, it's a bug for code to 
>> make an indirect buffer, then perform operations on it via setcdr 
>> or setf like things, which then will affect the original buffer's 
>> variables.
> I'm not sure it's a bug.  It could be a "feature", not limited to
> face-remapping-alist, but instead affecting every buffer-local
> variable that is not a simple scalar.

`make-indirect-buffer` can't do the right thing, indeed, because
different variables need different handling.  `clone-buffer` tries to
handle these issues by relying on `clone-buffer-hook` to handle the
various variables on a case-by-case basis, but `make-indirect-buffer`
doesn't come with such a hook.

Some ways we can fix this:

- In `face-remap.el`, refrain from modifying the `face-remapping-alist`
  by side-effects (i.e. avoid `delq`, `setcdr`, and friends).

- Add a `make-indirect-buffer-hook` and arrange for `face-remap.el` to
  add a function there that does a deep enough copy of
  `face-remapping-alist`.

- Remember that indirect buffers are an attractive nuisance and should
  be deprecated (but note that I suspect the same bug affects
  `clone-buffer` because it doesn't make a deep enough copy of
  `face-remapping-alist` either).


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: ahyatt <at> gmail.com, 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 20:16:06 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Andrew Hyatt <ahyatt <at> gmail.com>,  53294 <at> debbugs.gnu.org
> Date: Sun, 16 Jan 2022 12:11:39 -0500
> 
> Some ways we can fix this:
> 
> - In `face-remap.el`, refrain from modifying the `face-remapping-alist`
>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends).
> 
> - Add a `make-indirect-buffer-hook` and arrange for `face-remap.el` to
>   add a function there that does a deep enough copy of
>   `face-remapping-alist`.
> 
> - Remember that indirect buffers are an attractive nuisance and should
>   be deprecated (but note that I suspect the same bug affects
>   `clone-buffer` because it doesn't make a deep enough copy of
>   `face-remapping-alist` either).

The last one tells me we are better with leaving this sleeping dog
lie.

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Sun, 16 Jan 2022 16:41:44 -0500

On Sun, Jan 16, 2022 at 08:16 PM Eli Zaretskii <eliz <at> gnu.org> 
wrote: 

>> From: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Andrew 
>> Hyatt <ahyatt <at> gmail.com>,  53294 <at> debbugs.gnu.org Date: Sun, 16 
>> Jan 2022 12:11:39 -0500  Some ways we can fix this:  - In 
>> `face-remap.el`, refrain from modifying the 
>> `face-remapping-alist` 
>>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends). 
>>  - Add a `make-indirect-buffer-hook` and arrange for 
>> `face-remap.el` to 
>>   add a function there that does a deep enough copy of 
>>   `face-remapping-alist`. 
>>  - Remember that indirect buffers are an attractive nuisance 
>> and should 
>>   be deprecated (but note that I suspect the same bug affects 
>>   `clone-buffer` because it doesn't make a deep enough copy of 
>>   `face-remapping-alist` either). 
> 
> The last one tells me we are better with leaving this sleeping 
> dog lie. 

I agree the use of indirect buffers is problematic, but this 
problem actually results in user-visible bugs. Anyone who likes 
their org-mode buffer to be variable-pitch, and likes their 
capture buffer to be in a larger font (both pretty reasonable 
things), will run into this problem.

My patch fixes this in a way whose only downside is that it would 
be less efficient notably when you have a lot of face-remappings. 
But it's not clear to me that face-remapping-alist ever gets so 
big or is changed so often that this would be a problem.

The only other option is to fix this in org-mode, but they are 
cloning their indirect buffer presumably so that the capture 
buffer looks and behaves like the parent buffer, which is 
reasonable. I'd have to break that, or maybe just add a hack to 
deep copy face-remapping-alist.  Both options seem a bit wrong.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Mon, 17 Jan 2022 16:47:08 -0500

>> - In `face-remap.el`, refrain from modifying the `face-remapping-alist`
>>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends).
>> 
>> - Add a `make-indirect-buffer-hook` and arrange for `face-remap.el` to
>>   add a function there that does a deep enough copy of
>>   `face-remapping-alist`.
>> 
>> - Remember that indirect buffers are an attractive nuisance and should
>>   be deprecated (but note that I suspect the same bug affects
>>   `clone-buffer` because it doesn't make a deep enough copy of
>>   `face-remapping-alist` either).
>
> The last one tells me we are better with leaving this sleeping dog
> lie.

Actually, looking back at the code, I see we already have the needed
hook (tho called from `clone-indirect-buffer`), so I think the better
option is to change `make-indirect-buffer` so it runs
`clone-indirect-buffer-hook` (when called with a non-nil `clone` arg)
instead of having `clone-indirect-buffer` run it.  And then in
`face-remap.el` add a function to that hook which copies
`face-remapping-alist`.

An intermediate option is to change the Org code to use
`clone-indirect-buffer` instead of `make-indirect-buffer` (but that
still requires the `face-remap.el` addition to the hook).

[ Sorry Andrew for not noticing your earlier patch which does something
  similar to my first suggestion above.  ]

Andrew, would you be willing to write that patch?


        Stefan

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Mon, 17 Jan 2022 21:24:24 -0500

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

On Mon, Jan 17, 2022 at 04:47 PM Stefan Monnier 
<monnier <at> iro.umontreal.ca> wrote: 

>>> - In `face-remap.el`, refrain from modifying the 
>>> `face-remapping-alist` 
>>>   by side-effects (i.e. avoid `delq`, `setcdr`, and friends). 
>>>  - Add a `make-indirect-buffer-hook` and arrange for 
>>> `face-remap.el` to 
>>>   add a function there that does a deep enough copy of 
>>>   `face-remapping-alist`. 
>>>  - Remember that indirect buffers are an attractive nuisance 
>>> and should 
>>>   be deprecated (but note that I suspect the same bug affects 
>>>   `clone-buffer` because it doesn't make a deep enough copy of 
>>>   `face-remapping-alist` either). 
>> 
>> The last one tells me we are better with leaving this sleeping 
>> dog lie. 
> 
> Actually, looking back at the code, I see we already have the 
> needed hook (tho called from `clone-indirect-buffer`), so I 
> think the better option is to change `make-indirect-buffer` so 
> it runs `clone-indirect-buffer-hook` (when called with a non-nil 
> `clone` arg) instead of having `clone-indirect-buffer` run it. 
> And then in `face-remap.el` add a function to that hook which 
> copies `face-remapping-alist`. 
> 
> An intermediate option is to change the Org code to use 
> `clone-indirect-buffer` instead of `make-indirect-buffer` (but 
> that still requires the `face-remap.el` addition to the hook). 
> 
> [ Sorry Andrew for not noticing your earlier patch which does 
> something 
>   similar to my first suggestion above.  ] 
> 
> Andrew, would you be willing to write that patch?
>  
>         Stefan 

That sounds reasonable, I can do that. In fact, I just did this 
based on your suggestions, and it does work well. The only weird 
thing is that I had to pull `clone-indirect-buffer-hook' into the 
c code, because that's where `make-indirect-buffer' is. Let me 
know if that seems wrong.

I've attached the patch, please let me know if there is an issue. 
I have commit access, so I can just commit it myself after your OK 
(if so, I'll wait for a week or so to see if Eli has a comment as 
well before checkin).

[text-scale-patch-v2 (application/octet-stream, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Mon, 17 Jan 2022 21:47:30 -0500

> That sounds reasonable, I can do that. In fact, I just did this based on
> your suggestions, and it does work well.  The only weird thing is that I had
> to pull `clone-indirect-buffer-hook' into the c code, because that's where
> `make-indirect-buffer' is.

You could leave the `defvar` in Lisp and only pull the DEFSYM, but
yes, you need to pull at least part of it into C.

> Let me know if that seems wrong.

Nope.

> I've attached the patch, please let me know if there is an issue. I have
> commit access, so I can just commit it myself after your OK (if so, I'll
> wait for a week or so to see if Eli has a comment as well before checkin).

Comments below, thanks,


        Stefan


> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
> index 00560f9d2e..fc0edb7119 100644
> --- a/lisp/face-remap.el
> +++ b/lisp/face-remap.el
> @@ -70,6 +70,14 @@ internal-lisp-face-attributes
>     :foreground :background :stipple :overline :strike-through :box
>     :font :inherit :fontset :distant-foreground :extend :vector])
>  
> +(defun face-attrs--make-indirect-safe ()
> +  "Do a deep copy of `face-remapping-alist' to keep the original buffer safe."
> +  (setq-local face-remapping-alist
> +              (copy-tree
> +               (buffer-local-value 'face-remapping-alist (buffer-base-buffer)))))

I think `mapcar #'copy-sequence` is slightly more correct than
`copy-tree`, and you shouldn't need
(buffer-local-value 'face-remapping-alist (buffer-base-buffer)) because
that buffer's value should already have been copied to the current buffer.

> @@ -912,6 +912,10 @@ DEFUN ("make-indirect-buffer", Fmake_indirect_buffer, Smake_indirect_buffer,
>        Fset (intern ("buffer-save-without-query"), Qnil);
>        Fset (intern ("buffer-file-number"), Qnil);
>        Fset (intern ("buffer-stale-function"), Qnil);
> +      /* Cloned buffers need extra setup, to do things such as deep
> +	 variable copies for list variables that might be mangled due
> +	 to destructive operations in the indirect buffer. */
> +      safe_run_hooks (Qclone_indirect_buffer_hook);

I think you want to use just `run_hook` here because there's no need to
catch errors.

More importantly, you need to remove the corresponding `run-hook` from
`clone-indirect-buffer`.


        Stefan

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Tue, 18 Jan 2022 21:37:59 -0500

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

On Mon, Jan 17, 2022 at 09:47 PM Stefan Monnier 
<monnier <at> iro.umontreal.ca> wrote: 

>> That sounds reasonable, I can do that. In fact, I just did this 
>> based on your suggestions, and it does work well.  The only 
>> weird thing is that I had to pull `clone-indirect-buffer-hook' 
>> into the c code, because that's where `make-indirect-buffer' 
>> is. 
> 
> You could leave the `defvar` in Lisp and only pull the DEFSYM, 
> but yes, you need to pull at least part of it into C. 
> 
>> Let me know if that seems wrong. 
> 
> Nope. 
> 
>> I've attached the patch, please let me know if there is an 
>> issue. I have commit access, so I can just commit it myself 
>> after your OK (if so, I'll wait for a week or so to see if Eli 
>> has a comment as well before checkin). 
> 
> Comments below, thanks, 
>  
>         Stefan 
>  
>> diff --git a/lisp/face-remap.el b/lisp/face-remap.el index 
>> 00560f9d2e..fc0edb7119 100644 --- a/lisp/face-remap.el +++ 
>> b/lisp/face-remap.el @@ -70,6 +70,14 @@ 
>> internal-lisp-face-attributes 
>>     :foreground :background :stipple :overline :strike-through 
>>     :box :font :inherit :fontset :distant-foreground :extend 
>>     :vector]) 
>>   
>> +(defun face-attrs--make-indirect-safe () +  "Do a deep copy of 
>> `face-remapping-alist' to keep the original buffer safe."  + 
>> (setq-local face-remapping-alist +              (copy-tree + 
>> (buffer-local-value 'face-remapping-alist 
>> (buffer-base-buffer))))) 
> 
> I think `mapcar #'copy-sequence` is slightly more correct than 
> `copy-tree`, and you shouldn't need (buffer-local-value 
> 'face-remapping-alist (buffer-base-buffer)) because that 
> buffer's value should already have been copied to the current 
> buffer. 
> 
>> @@ -912,6 +912,10 @@ DEFUN ("make-indirect-buffer", 
>> Fmake_indirect_buffer, Smake_indirect_buffer, 
>>        Fset (intern ("buffer-save-without-query"), Qnil); Fset 
>>        (intern ("buffer-file-number"), Qnil); Fset (intern 
>>        ("buffer-stale-function"), Qnil); 
>> +      /* Cloned buffers need extra setup, to do things such as 
>> deep +	 variable copies for list variables that might be 
>> mangled due +	 to destructive operations in the indirect 
>> buffer. */ +      safe_run_hooks (Qclone_indirect_buffer_hook); 
> 
> I think you want to use just `run_hook` here because there's no 
> need to catch errors. 
> 
> More importantly, you need to remove the corresponding 
> `run-hook` from `clone-indirect-buffer`. 
>  
>         Stefan

Makes sense, thanks for the suggestions.  I've included the 
revised patch.

[text-scale-patch-v3 (application/octet-stream, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 53294 <at> debbugs.gnu.org
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Tue, 18 Jan 2022 22:42:08 -0500

> Makes sense, thanks for the suggestions.  I've included the revised patch.

Looks good to me, thanks,


        Stefan

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Andrew Hyatt <ahyatt <at> gmail.com>, 53294 <at> debbugs.gnu.org,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Thu, 20 Jan 2022 14:42:45 +0100

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

>> Makes sense, thanks for the suggestions.  I've included the revised patch.
>
> Looks good to me, thanks,

So I've pushed the patch to Emacs 29.

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

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

From: Anders Johansson <mejlaandersj <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 
 Andrew Hyatt <ahyatt <at> gmail.com>, 53294 <at> debbugs.gnu.org,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Wed, 16 Feb 2022 18:41:15 +0100

Hi,

Stefan wrote:
> I think `mapcar #'copy-sequence` is slightly more correct than
> `copy-tree`,

This gives me  "Wrong type argument: listp, mini-modeline-mode-line-active"
For a face-remapping-alist that looks like:
((mode-line-active . mini-modeline-mode-line-active)
 (mode-line-inactive . mini-modeline-mode-line-inactive))

According to my reading of the docstring of face-remapping-alist, this
should be allowed, and does work (except for indirect buffers perhaps)

face-remap-add-relative would generate:
((mode-line-active mini-modeline-mode-line-active)
 (mode-line-inactive mini-modeline-mode-line-inactive))

mini-modeline (https://github.com/kiennq/emacs-mini-modeline/) sets
face-remapping-alist directly for various reasons. I guess this is not
really recommended. But this fix seems to cause problems in an
otherwise "correct" use of face-remapping-alist.

Best,
Anders Johansson

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Anders Johansson <mejlaandersj <at> gmail.com>
Cc: Andrew Hyatt <ahyatt <at> gmail.com>, 53294 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Thu, 17 Feb 2022 12:41:03 +0100

Anders Johansson <mejlaandersj <at> gmail.com> writes:

> This gives me  "Wrong type argument: listp, mini-modeline-mode-line-active"
> For a face-remapping-alist that looks like:
> ((mode-line-active . mini-modeline-mode-line-active)
>  (mode-line-inactive . mini-modeline-mode-line-inactive))

Do you have a recipe to reproduce the problem, starting from "emacs -Q"?

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Anders Johansson <mejlaandersj <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 53294 <at> debbugs.gnu.org,
 Andrew Hyatt <ahyatt <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#53294: 29.0.50; Indirect font changes incorrectly affecting
 original buffer
Date: Thu, 17 Feb 2022 08:38:08 -0500

>> I think `mapcar #'copy-sequence` is slightly more correct than
>> `copy-tree`,
>
> This gives me  "Wrong type argument: listp, mini-modeline-mode-line-active"
> For a face-remapping-alist that looks like:
> ((mode-line-active . mini-modeline-mode-line-active)
>  (mode-line-inactive . mini-modeline-mode-line-inactive))

I pushed a patch to `master` which should fix it.  Can you confirm that
it fixes it for you?


        Stefan

Previous Next

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