GNU bug report logs - #77042
31.0.50; `string-pixel-width' could return wrong results with alternative properties

Previous Next

To reply to this bug, email your comments to 77042 AT debbugs.gnu.org.
There is no need to reopen the bug first.

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

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

From: David Ponce <da_vid <at> orange.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; `string-pixel-width' could return wrong results with
 alternative properties
Date: Sat, 15 Mar 2025 23:43:33 +0100

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

Hello,

During some experiments to use `char-property-alias-alist', I
discovered that `string-pixel-width' could return wrong results with
alternative properties that affect how the string is displayed.

Here is a simple recipe to eval in the *scratch* buffer, that
illustrates the issue:

(let ((text0 (propertize "This text" 'face 'variable-pitch))
      (text1 (propertize "This text" 'my-face 'variable-pitch)))
  (with-temp-buffer
    (setq-local char-property-alias-alist '((face my-face)))
    (list
     (string-pixel-width text0 (current-buffer))
     (string-pixel-width text1 (current-buffer)))))

Normally the result should be a list of two identical numbers.  But,
on my config, the result is (89 117) instead of expected (89 89).

This problem is similar to the already solved case with
`face-remapping-alist', as is the solution.
                        
I propose the attached patch to fix this and slightly simplify the
code.  On my configuration, the new code has no significant impact on
performance.

The patch also includes several tests of `string-pixel-width' that
could be added to test/lisp/misc-test.el.  As expected with the
current implementation, all tests pass but the test
`misc-test-string-pixel-width-char-property-alias-alist'.  With the
patch applied, all tests pass:

-----------------------
emacs -batch -l ./test/lisp/misc-tests.el -f ert-run-tests-batch-and-exit

   [...]
   passed   6/11  misc-test-string-pixel-width-char-property-alias-alist (0.001030 sec)
   passed   7/11  misc-test-string-pixel-width-display-line-numbers (0.000057 sec)
   passed   8/11  misc-test-string-pixel-width-face-remapping-alist (0.000065 sec)
   passed   9/11  misc-test-string-pixel-width-line-and-wrap-prefix (0.000240 sec)
   [...]
-----------------------

Here is a possible Change log:

2025-03-15  David Ponce  <da_vid <at> orange.fr>

	Fix possible wrong result of `string-pixel-width' with alternate
	properties.  Create regression tests.

	* lisp/emacs-lisp/subr-x.el (string-pixel-width): Like for
	`face-remapping-alist', use in work buffer the value of
	`char-property-alias-alist' local to the passed buffer, to
	correctly compute pixel width.

	* test/lisp/misc-tests.el: Add tests for `string-pixel-width'.

Thanks

[string-pixel-width-fix-alt-props-plus-tests-V0.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 77042 <at> debbugs.gnu.org
Subject: Re: bug#77042: 31.0.50;
 `string-pixel-width' could return wrong results with alternative
 properties
Date: Sun, 16 Mar 2025 09:31:17 +0200

> Date: Sat, 15 Mar 2025 23:43:33 +0100
> From:  David Ponce via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> (let ((text0 (propertize "This text" 'face 'variable-pitch))
>        (text1 (propertize "This text" 'my-face 'variable-pitch)))
>    (with-temp-buffer
>      (setq-local char-property-alias-alist '((face my-face)))
>      (list
>       (string-pixel-width text0 (current-buffer))
>       (string-pixel-width text1 (current-buffer)))))
> 
> Normally the result should be a list of two identical numbers.  But,
> on my config, the result is (89 117) instead of expected (89 89).
> 
> This problem is similar to the already solved case with
> `face-remapping-alist', as is the solution.
>                          
> I propose the attached patch to fix this and slightly simplify the
> code.  On my configuration, the new code has no significant impact on
> performance.

Thanks, please see some comments below.

> +      ;; Setup current buffer to correctly compute pixel width.
> +      (when buffer
> +        (if (local-variable-p 'face-remapping-alist buffer)
> +            (setq-local face-remapping-alist
> +                        (buffer-local-value 'face-remapping-alist
> +                                            buffer)))
> +        (if (local-variable-p 'char-property-alias-alist buffer)
> +            (setq-local char-property-alias-alist
> +                        (buffer-local-value 'char-property-alias-alist
> +                                            buffer))))

What about default-text-properties? shouldn't it get the same
treatment?

> -      (add-text-properties
> -       (point-min) (point-max) '(display-line-numbers-disable t))
> -      ;; Prefer `remove-text-properties' to `propertize' to avoid
> -      ;; creating a new string on each call.
> -      (remove-text-properties
> -       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
> -      (setq line-prefix nil wrap-prefix nil)
> +      (add-text-properties (point-min) (point-max)
> +                           '( display-line-numbers-disable t
> +                              line-prefix "" wrap-prefix ""))

What is the rationale for this hunk?  Was there any problem with the
original code?

In any case, please don't leave a space after an opening parenthesis.

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77042 <at> debbugs.gnu.org
Subject: Re: bug#77042: 31.0.50; `string-pixel-width' could return wrong
 results with alternative properties
Date: Sun, 16 Mar 2025 11:31:21 +0100

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

On 2025-03-16 08:31, Eli Zaretskii wrote:
>> Date: Sat, 15 Mar 2025 23:43:33 +0100
>> From:  David Ponce via "Bug reports for GNU Emacs,
>>   the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> (let ((text0 (propertize "This text" 'face 'variable-pitch))
>>         (text1 (propertize "This text" 'my-face 'variable-pitch)))
>>     (with-temp-buffer
>>       (setq-local char-property-alias-alist '((face my-face)))
>>       (list
>>        (string-pixel-width text0 (current-buffer))
>>        (string-pixel-width text1 (current-buffer)))))
>>
>> Normally the result should be a list of two identical numbers.  But,
>> on my config, the result is (89 117) instead of expected (89 89).
>>
>> This problem is similar to the already solved case with
>> `face-remapping-alist', as is the solution.
>>                           
>> I propose the attached patch to fix this and slightly simplify the
>> code.  On my configuration, the new code has no significant impact on
>> performance.
> 
> Thanks, please see some comments below.
> 
>> +      ;; Setup current buffer to correctly compute pixel width.
>> +      (when buffer
>> +        (if (local-variable-p 'face-remapping-alist buffer)
>> +            (setq-local face-remapping-alist
>> +                        (buffer-local-value 'face-remapping-alist
>> +                                            buffer)))
>> +        (if (local-variable-p 'char-property-alias-alist buffer)
>> +            (setq-local char-property-alias-alist
>> +                        (buffer-local-value 'char-property-alias-alist
>> +                                            buffer))))
> 
> What about default-text-properties? shouldn't it get the same
> treatment?

Yes, you are right.  I updated the patch to include this case and a
new test.

2025-03-16  David Ponce  <da_vid <at> orange.fr>

	Fix possible wrong result of `string-pixel-width' with alternate
	and default properties.  Create regression tests.

	* lisp/emacs-lisp/subr-x.el (string-pixel-width): Like for
	`face-remapping-alist', use in work buffer the value of
	`char-property-alias-alist' and `default-text-properties'
	local to the passed buffer, to correctly compute pixel width.

	* test/lisp/misc-tests.el: Add tests for `string-pixel-width'.


> 
>> -      (add-text-properties
>> -       (point-min) (point-max) '(display-line-numbers-disable t))
>> -      ;; Prefer `remove-text-properties' to `propertize' to avoid
>> -      ;; creating a new string on each call.
>> -      (remove-text-properties
>> -       (point-min) (point-max) '(line-prefix nil wrap-prefix nil))
>> -      (setq line-prefix nil wrap-prefix nil)
>> +      (add-text-properties (point-min) (point-max)
>> +                           '( display-line-numbers-disable t
>> +                              line-prefix "" wrap-prefix ""))
> 
> What is the rationale for this hunk?  Was there any problem with the
> original code?

No problem.  But the new code is simpler and should be more efficient.
To disable `display-line-numbers', `line-prefix' and `wrap-prefix', an
unique call to `add-text-properties' can achieve the same result as
the previous implementation which called `add-text properties', then
`remove-text-properties', then `setq' twice.

> 
> In any case, please don't leave a space after an opening parenthesis.

Sorry, I thought it was correct to add a space after the opening
parenthesis to correctly indent a lisp data expression.  I rewrote the
expression to avoid this.

Regarding the proposed tests, I noticed that those involving faces
always pass when run in batch, but work as expected interactively.
Probably because face attributes don't make sense in batch mode?

I mentioned this point in a comment before such tests, because I don't
know if it is possible to limit them to be run interactively.

Thank you for your review and help!

[string-pixel-width-fix-alt-def-props-plus-tests-V1.patch (text/x-patch, attachment)]

Message #16 received at 77042-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 77042-done <at> debbugs.gnu.org
Subject: Re: bug#77042: 31.0.50; `string-pixel-width' could return wrong
 results with alternative properties
Date: Thu, 20 Mar 2025 14:57:51 +0200

> Date: Sun, 16 Mar 2025 11:31:21 +0100
> Cc: 77042 <at> debbugs.gnu.org
> From: David Ponce <da_vid <at> orange.fr>
> 
> Thank you for your review and help!

Thanks, I installed the patch on the master branch, and I'm therefore
closing this bug.

Previous Next

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