bug#40978: 28.0.50; create-image and find-image consistency

[David Ponce <da_vid@orange.fr>]

Hello dear Emacs developers,

I am using Emacs latest master, and noticed some inconsistency between the
results of function `create-image' and `find-image'.

Here is an example:

(create-image "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")
==> (image :type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg" :scale 1.2)

(find-image '((:type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")))
==> (image :type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")

Contrary to images returned by `create-image', which are automatically scaled
when no :scale property is provided, those returned by `find-image' are not.

I propose the below patch to have `find-image' use `create-image' in order to get
consistent results from both functions. Another advantage of using `create-image' is
that it is no longer necessary to provide the :type property to `find-image'.
The correct type is determined by `create-image' with the proper default scaling.

Here is the result of previous example with patched `find-image':

(find-image '((:file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")))
==> (image :type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg" :scale 1.2)

Please eventually consider this enhancement to Emacs, if it makes sense.

Thanks & regards,
David Ponce

diff --git a/installs/emacs/lisp/image.el b/emacs.d/image.el
index 4ea8594..046af95 100644
--- a/installs/emacs/lisp/image.el
+++ b/emacs.d/image.el
@@ -687,13 +687,15 @@ SPECS is a list of image specifications.
   
   Each image specification in SPECS is a property list.  The contents of
   a specification are image type dependent.  All specifications must at
-least contain the properties `:type TYPE' and either `:file FILE' or
-`:data DATA', where TYPE is a symbol specifying the image type,
-e.g. `xbm', FILE is the file to load the image from, and DATA is a
-string containing the actual image data.  The specification whose TYPE
-is supported, and FILE exists, is used to construct the image
-specification to be returned.  Return nil if no specification is
-satisfied.
+least contain the either the property `:file FILE' or `:data DATA',
+where FILE is the file to load the image from, and DATA is a string
+containing the actual image data.  If the property `:type TYPE' is
+omitted or nil, try to determine the image type from its first few
+bytes of image data.  If that doesn’t work, and the property `:file
+FILE' provide a file name, use its file extension as image type. If
+the property `:type TYPE' is provided, it must match the actual type
+determined for FILE or DATA by `create-image'.  Return nil if no
+specification is satisfied.
   
   The image is looked for in `image-load-path'.
   
@@ -703,16 +705,39 @@ Image files should not be larger than specified by `max-image-size'."
         (let* ((spec (car specs))
   	     (type (plist-get spec :type))
   	     (data (plist-get spec :data))
-	     (file (plist-get spec :file))
-	     found)
-	(when (image-type-available-p type)
-	  (cond ((stringp file)
-		 (if (setq found (image-search-load-path file))
-		     (setq image
-			   (cons 'image (plist-put (copy-sequence spec)
-						   :file found)))))
-		((not (null data))
-		 (setq image (cons 'image spec)))))
+	     (file (plist-get spec :file)))
+	(cond
+         ((stringp file)
+	  (when (setq file (image-search-load-path file))
+            ;; At this point, remove the :type and :file properties.
+            ;; `create-image' will set them depending on image file.
+            (setq image (cons 'image (copy-sequence spec)))
+            (setf (image-property image :type) nil)
+            (setf (image-property image :file) nil)
+            (and (setq image (ignore-errors
+                               (apply #'create-image file nil nil
+                                      (cdr image))))
+                 ;; Ensure, if a type has been provided, it is
+                 ;; consistent with the type returned by
+                 ;; `create-image'. If not, return nil.
+                 (not (null type))
+                 (not (eq type (image-property image :type)))
+                 (setq image nil))))
+	  ((not (null data))
+            ;; At this point, remove the :type and :data properties.
+            ;; `create-image' will set them depending on image data.
+           (setq image (cons 'image (copy-sequence spec)))
+           (setf (image-property image :type) nil)
+           (setf (image-property image :data) nil)
+	   (and (setq image (ignore-errors
+                              (apply #'create-image data nil t
+                                     (cdr image))))
+                ;; Ensure, if a type has been provided, it is
+                ;; consistent with the type returned by
+                ;; `create-image'. If not, return nil.
+                (not (null type))
+                (not (eq type (image-property image :type)))
+                (setq image nil))))
   	(setq specs (cdr specs))))
       image))
   

-------- Forwarded Message --------
From: Juri Linkov <juri@linkov.net>
To: David Ponce <da_vid@orange.fr>
Subject: Re: create-image and find-image consistency
Date: Mon, 20 Apr 2020 02:24:14 +0300

> I propose the below patch to have `find-image' use `create-image' in order to get
> consistent results from both functions. Another advantage of using `create-image' is
> that it is no longer necessary to provide the :type property to `find-image'.
> The correct type is determined by `create-image' with the proper default scaling.
>
> Here is the result of previous example with patched `find-image':
>
> (find-image '((:file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")))
> ==> (image :type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg" :scale 1.2)
>
> Please eventually consider this enhancement to Emacs, if it makes sense.

I haven't tested it yet, but it makes sense indeed.

-------- Forwarded Message --------
From: Lars Ingebrigtsen <larsi@gnus.org>
To: David Ponce <da_vid@orange.fr>
Cc: emacs-devel@gnu.org
Subject: Re: create-image and find-image consistency
Date: Thu, 30 Apr 2020 07:08:54 +0200

David Ponce <da_vid@orange.fr> writes:

> I am using Emacs latest master, and noticed some inconsistency between the
> results of function `create-image' and `find-image'.
>
> Here is an example:
>
> (create-image "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")
> ==> (image :type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg" :scale 1.2)
>
> (find-image '((:type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")))
> ==> (image :type svg :file "[EMACS-DIR]/etc/images/icons/hicolor/scalable/apps/emacs.svg")
>
> Contrary to images returned by `create-image', which are automatically scaled
> when no :scale property is provided, those returned by `find-image' are not.
>
> I propose the below patch to have `find-image' use `create-image' in
> order to get consistent results from both functions.

I think it conceptually makes sense to have find-image use create-image,
but looking at the code, and where find-image is used, it looks like the
use case it for creating toolbars and the like, where you want to pick
out one of the (built-in) image formats that Emacs supports...

Looking at your patch, you remove the (image-type-available-p type), and
instead rely on create-image not bugging out instead?  That feels like a
less obvious way to do the test (and more breakable; there may be other
reasons create-image fails).

And I'm not 100% sure that we want to auto-scale toolbars and the like.
I'm pretty sure we do, but perhaps somebody else has an opinion here?

Anyway, I think you should file this as a bug report so that the patch
doesn't get lost, because I think you're basically correct that the
find-image behaviour should be changed.

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