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

Previous Next

Full log

View this message in rfc822 format

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: 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)]

Previous Next

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