Re: [O] Bug: org-string-display throws on image display property [9.1.9 (release_9.1.9-65-g5e4542 @ /home/blc/.local/share/emacs/27.0.50/lisp/org/)]

[Basil L. Contovounesios]

Nicolas Goaziou <address@hidden> writes:

> "Basil L. Contovounesios" <address@hidden> writes:
>
>> Severity: minor
>>
>> 0. emacs -Q
>> 1. M-x toggle-debug-on-error RET
>> 2. Evaluate the following:
>>
>>   (require 'org)
>>   (org-string-display
>>    (propertize " " 'display '(image :type svg
>>                                     :file "/path/to/image.svg")))
>>
>> This gives the following error backtrace:
>>
>> Debugger entered--Lisp error: (wrong-type-argument sequencep t)
>
> `org-string-display' doesn't exist anymore in master (Org 9.2). However,
> there is the same error with `org-string-width'. 
>
> What is `org-string-width' supposed to do on your example? Probably
> return an error.

I realise that the 'display' property is a pretty complicated beast, so
it may be impractical for 'org-string-width' to do this, but I think
more accurate/useful behaviour would be to calculate the displayed width
of the image via 'image-size' or 'image-display-size', rather than
signalling an error.

>> I realise that org-string-display is a relatively obscure function that
>> was probably not designed with image or other complex display properties
>> in mind, but AFAICT the following is a pretty obvious "fix", given that
>> the surrounding code expects 'value' to be a string or nil, but never t:
>>
>> diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
>> index 583633605f..5ab069e6a2 100644
>> --- a/lisp/org/org-macs.el
>> +++ b/lisp/org/org-macs.el
>> @@ -108,7 +108,7 @@ org-string-display
>>                     ;; string (face...).
>>                     (let* ((display (plist-get props 'display))
>>                            (value (if (stringp display) display
>> -                                   (cl-some #'stringp display))))
>> +                                       (cl-find-if #'stringp display))))
>>                       (when value
>>                         (apply #'propertize
>>                                ;; Displayed string could contain
>>
>>
>> With this change, the return value of org-string-display does not really
>> make sense for images, but at least the function doesn't barf on complex
>> but valid display properties.  WDYT?
>
> If `org-string-width' returns nil, it will ensue an error somewhere
> further in the code. So returning an error right here is better (or
> find a way to compute the width of an image in columns).

Why/when would 'org-string-width' return nil?  My suggestion was to
replace 'cl-some' with 'cl-find-if', because the value of
(cl-some #'stringp ...) is a boolean, not a sequence.

To clarify - I am not suggesting that 'org-string-width' should never
signal an error; I am only suggesting that the current
(wrong-type-argument sequencep t) error is due to inappropriate use of
'cl-some'.  My comments may already be outdated, though, since they
refer to the version of org-macs.el shipping with latest Emacs master.

> As another data point, `string-width' returns 1 on your example, which
> is probably wrong.

Neither 'string-width' nor 'current-column' take 'display' properties
into account; see e.g. https://emacs.stackexchange.com/q/44495/15748.
But that's an Emacs feature request of its own.

Thanks,

-- 
Basil