create-image and find-image consistency

create-image and find-image consistency

[David Ponce]

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

Re: create-image and find-image consistency

[Juri Linkov]

> 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.

Re: create-image and find-image consistency

[Lars Ingebrigtsen]

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

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

[David Ponce]

On 30/04/2020 07:08, Lars Ingebrigtsen wrote:
> 
> 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.
> 

Hi Lars,

I created bug report #40978 as suggested.

My idea was to better separate the role of find-image from
create-image:

- create-image to actually make a new image based on given specs, and
   maybe some common options and device capabilities (for example, auto
   scaling based on screen resolution).
   
- find-image to lookup for an image in the file system or in raw data,
   but delegating to create-image the actual creation of the image.

Currently find-image & create-image can return a different image from
the same spec, which is not consistent. My patch proposes to fix such
potential inconsistencies.
When you don't need auto-scaling for example, you can pass :scale 1.0
to find-image, like you would have done with create-image :-)
IMHO, auto-scaling is particularly useful with graphics elements like
tool bars, tab bars, and widgets ;-)

Thanks for taking into account this proposal!

Re: create-image and find-image consistency

[Clément Pit-Claudel]

On 30/04/2020 05.24, David Ponce wrote:
> On 30/04/2020 07:08, Lars Ingebrigtsen wrote:
>>
>> 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.
>>
> 
> Hi Lars,
> 
> I created bug report #40978 as suggested.
> 
> My idea was to better separate the role of find-image from
> create-image:
> 
> - create-image to actually make a new image based on given specs, and
>   maybe some common options and device capabilities (for example, auto
>   scaling based on screen resolution).
>   - find-image to lookup for an image in the file system or in raw data,
>   but delegating to create-image the actual creation of the image.
> 
> Currently find-image & create-image can return a different image from
> the same spec, which is not consistent. My patch proposes to fix such
> potential inconsistencies.
> When you don't need auto-scaling for example, you can pass :scale 1.0
> to find-image, like you would have done with create-image :-)
> IMHO, auto-scaling is particularly useful with graphics elements like
> tool bars, tab bars, and widgets ;-)

One problem is that SVG scaling is broken at the moment, so auto-scaling SVGs yields blurry results, so it's not clear that scaling is in fact desirable for tool bars.
Additionally, the scaling that create-image does is based on font size, right?  Do we expect font size changes to change the size of tab bar icons?