From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Basil L. Contovounesios" Subject: Re: 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/)] Date: Mon, 03 Sep 2018 17:46:08 +0100 Message-ID: <87zhwyttpr.fsf@tcd.ie> References: <874lf8hhel.fsf@tcd.ie> <87d0tutxch.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54018) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fwrzb-0002jr-0E for emacs-orgmode@gnu.org; Mon, 03 Sep 2018 12:46:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fwrzW-0004Lr-N0 for emacs-orgmode@gnu.org; Mon, 03 Sep 2018 12:46:22 -0400 Received: from mail-ed1-x543.google.com ([2a00:1450:4864:20::543]:37768) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fwrzW-0004Ks-DM for emacs-orgmode@gnu.org; Mon, 03 Sep 2018 12:46:18 -0400 Received: by mail-ed1-x543.google.com with SMTP id a20-v6so1227042edd.4 for ; Mon, 03 Sep 2018 09:46:16 -0700 (PDT) In-Reply-To: <87d0tutxch.fsf@nicolasgoaziou.fr> (Nicolas Goaziou's message of "Mon, 3 Sep 2018 17:27:42 +0200") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: "Emacs-orgmode" To: Nicolas Goaziou Cc: emacs-orgmode@gnu.org Nicolas Goaziou writes: > "Basil L. Contovounesios" 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