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?

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

[Lars Ingebrigtsen]

David Ponce <da_vid@orange.fr> writes:

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

Yes, I think that's a good idea.

> IMHO, auto-scaling is particularly useful with graphics elements like
> tool bars, tab bars, and widgets ;-)

Yeah, that's true.  I think.

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

bug#40978: bug#47039: 27.1; find-image ignores image-scaling-factor

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> IMHO, auto-scaling is particularly useful with graphics elements like
>> tool bars, tab bars, and widgets ;-)
>
> Yeah, that's true.  I think.

I was finally reminded that I should try David's patch after looking at
a --with-x-toolkit=lucid build where all the toolbar icons were
minuscule on this 4K 14" laptop screen.

And it does indeed fix these issues, as well as the tiny "Gnu" logo in
the Gnus Group mode line, etc.

So I've now pushed it to Emacs 29.

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

bug#40978: 28.0.50; 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))
   

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