bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Stefan Kangas]

I find the semantics of `image-transform-resize' inconsistent.  That
also goes for `image-auto-resize' and the corresponding functions.

"Fit height" and "fit width" both scales an image up or down, but "fit
height and width" only scales an image down.  Is that intentional?

I would have expected that they all scale in the same way, i.e. they
should all either scale up or down, or scale down only, or scale up
only.

My preference is that they scale both up or down, no matter which kind
of fitting you ask for.  The below diff seems to achieve what I want.
However, I'm not sure if the semantics should be changed in this way,
as I don't know if the old semantics were intentional or not.


diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index 4a326cdc69..97efd8254d 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -880,7 +880,7 @@ image-toggle-display-image
                                   :format (and filename data-p))
 		  (create-image file-or-data type data-p :scale 1
 				:max-width max-width
-				:max-height max-height
+                                :height max-height
                                 ;; Type hint.
                                 :format (and filename data-p))))

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> "Fit height" and "fit width" both scales an image up or down, but "fit
> height and width" only scales an image down.  Is that intentional?

Yes, I think so?  The point of the latter is that too-big images are
pretty useless -- you want to scale them down so that you can actually
see them.  But it doesn't therefore follow that you want to scale tiny
icons up to fill the screen.

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

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> "Fit height" and "fit width" both scales an image up or down, but "fit
>> height and width" only scales an image down.  Is that intentional?
>
> Yes, I think so?  The point of the latter is that too-big images are
> pretty useless -- you want to scale them down so that you can actually
> see them.  But it doesn't therefore follow that you want to scale tiny
> icons up to fill the screen.

Right, and that's a valid use case of course.  I still find the
interface inconsistent, as the naming scheme suggests that these three
options should behave similarly.

I made a quick review of other image viewers:

emacs                  gthumb          geeqie                eog
-----                  ------          ------                ---
fit height and width   Automatic       Zoom 1:1              Best fit
<missing>              Fit to window   Fit image to window   <missing>
fit to width           Fit to width    <missing>             <missing>
fit to height          Fit to height   <missing>             <missing>

How about renaming "fit height and width" to something that suggests
that it behaves differently from "fit to width" and "fit to height", and
then adding a new option "fit to window" that scales up or down as
needed?

Perhaps we could even have a "smart" option that only scales images up
larger than some height and width, and otherwise leaves them in their
original size.  That's probably the one I would like to use, now that I
think about it.  (I usually prefer to scale images up, but as you point
out it's pretty useless to scale small icons to fit the window.)

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Juri Linkov]

> I made a quick review of other image viewers:
>
> emacs                  gthumb          geeqie                eog
> -----                  ------          ------                ---
> fit height and width   Automatic       Zoom 1:1              Best fit
> <missing>              Fit to window   Fit image to window   <missing>

What is “Fit to window”?  Does it distort the image
by changing its height/width ratio?

> fit to width           Fit to width    <missing>             <missing>
> fit to height          Fit to height   <missing>             <missing>
>
> How about renaming "fit height and width" to something that suggests
> that it behaves differently from "fit to width" and "fit to height", and
> then adding a new option "fit to window" that scales up or down as
> needed?

Its menu item already displays a hint “Best Fit”:

	["Fit Image to Window (Best Fit)" image-transform-fit-both
	 :help "Resize image to match the window height and width"]

> Perhaps we could even have a "smart" option that only scales images up
> larger than some height and width, and otherwise leaves them in their
> original size.  That's probably the one I would like to use, now that I
> think about it.  (I usually prefer to scale images up, but as you point
> out it's pretty useless to scale small icons to fit the window.)

Maybe a new user option (disabled by default) could scale up
like ImageMagick's '-resize' does.

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

>> I made a quick review of other image viewers:
>>
>> emacs                  gthumb          geeqie                eog
>> -----                  ------          ------                ---
>> fit height and width   Automatic       Zoom 1:1              Best fit
>> <missing>              Fit to window   Fit image to window   <missing>
>
> What is “Fit to window”?  Does it distort the image
> by changing its height/width ratio?

No, it preserves the aspect ratio.

>> Perhaps we could even have a "smart" option that only scales images up
>> larger than some height and width, and otherwise leaves them in their
>> original size.  That's probably the one I would like to use, now that I
>> think about it.  (I usually prefer to scale images up, but as you point
>> out it's pretty useless to scale small icons to fit the window.)
>
> Maybe a new user option (disabled by default) could scale up
> like ImageMagick's '-resize' does.

What does that option do?  (And yes, I imagine the above "smart"
resizing idea to be both optional and separate from the standard
options.)

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Juri Linkov]

> Juri Linkov <juri@linkov.net> writes:
>
>>> I made a quick review of other image viewers:
>>>
>>> emacs                  gthumb          geeqie                eog
>>> -----                  ------          ------                ---
>>> fit height and width   Automatic       Zoom 1:1              Best fit
>>> <missing>              Fit to window   Fit image to window   <missing>
>>
>> What is “Fit to window”?  Does it distort the image
>> by changing its height/width ratio?
>
> No, it preserves the aspect ratio.

What is the difference between “Automatic” and “Fit to window”?

>>> Perhaps we could even have a "smart" option that only scales images up
>>> larger than some height and width, and otherwise leaves them in their
>>> original size.  That's probably the one I would like to use, now that I
>>> think about it.  (I usually prefer to scale images up, but as you point
>>> out it's pretty useless to scale small icons to fit the window.)
>>
>> Maybe a new user option (disabled by default) could scale up
>> like ImageMagick's '-resize' does.
>
> What does that option do?  (And yes, I imagine the above "smart"
> resizing idea to be both optional and separate from the standard
> options.)

It scales up, unless a special character > is used at the end.

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

>>>> I made a quick review of other image viewers:
>>>>
>>>> emacs                  gthumb          geeqie                eog
>>>> -----                  ------          ------                ---
>>>> fit height and width   Automatic       Zoom 1:1              Best fit
>>>> <missing>              Fit to window   Fit image to window   <missing>
>>>
>>> What is “Fit to window”?  Does it distort the image
>>> by changing its height/width ratio?
>>
>> No, it preserves the aspect ratio.
>
> What is the difference between “Automatic” and “Fit to window”?

In gthumb, "Automatic" is like our "fit height and width" in that it
only scales down as needed, while "Fit to window" scales both up and
down.

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Right, and that's a valid use case of course.  I still find the
> interface inconsistent, as the naming scheme suggests that these three
> options should behave similarly.

Yes.  I think that, basically, the fit to height/width commands are
pretty useless -- nobody wants that, because it'll inevitably make some
images impossible to view.

The two cases that make sense are "scale down so I can see the images"
and "both scale down and scale up, because I want to see as much detail
as possible".

> Perhaps we could even have a "smart" option that only scales images up
> larger than some height and width, and otherwise leaves them in their
> original size.  That's probably the one I would like to use, now that I
> think about it.  (I usually prefer to scale images up, but as you point
> out it's pretty useless to scale small icons to fit the window.)

Sure, or a "don't scale up more than 200%", perhaps.  That's less
finicky, I think.

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

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

tags 51596 + patch
thanks

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Right, and that's a valid use case of course.  I still find the
>> interface inconsistent, as the naming scheme suggests that these three
>> options should behave similarly.

I've attached a patch below which implements the behavior I propose.

> Yes.  I think that, basically, the fit to height/width commands are
> pretty useless -- nobody wants that, because it'll inevitably make some
> images impossible to view.
>
> The two cases that make sense are "scale down so I can see the images"
> and "both scale down and scale up, because I want to see as much detail
> as possible".

That's true, now that you mention it.  The first patch below therefore
obsoletes the old commands, mostly to free up the "s w" binding which
gives the new, and strictly better, behavior.

We could also leave them unobsoleted and still take over the key.  It
won't hurt anyone because, as you point out, the current command is only
different in the cases where its worse.

> Sure, or a "don't scale up more than 200%", perhaps.  That's less
> finicky, I think.

Yes, that's probably better.  For this part, see my second patch below,
which builds on the first one.

[-- Attachment #2: 0001-New-command-image-transform-fit-to-window.patch --]
[-- Type: text/x-diff, Size: 8636 bytes --]

From c5760898d1e396682c4388dc8b474e0830dd2b2e Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 5 Nov 2021 03:24:50 +0100
Subject: [PATCH 1/2] New command image-transform-fit-to-window

* lisp/image-mode.el (image-auto-resize, image-transform-resize):
Add new value 'fit-window', meaning to scale the image up or down
to fit the window.  (Bug#)
(image-transform-fit-to-window): New command.
(image-transform-fit-to-height, image-transform-fit-to-width):
Make obsolete in favor of above new command.
(image-mode-map): Bind "s w" to 'image-transform-fit-to-window'.
Move binding for obsolete command 'image-transform-fit-to-width'
to "s i".
(image-mode-map): Add menu entry for
'image-transform-fit-to-window'.  Remove menu entries for above
obsolete commands.
* doc/emacs/files.texi (Image Mode): Update documentation.
(image-transform-fit-both): Doc fix.
---
 doc/emacs/files.texi |  4 ++--
 etc/NEWS             | 18 ++++++++++++++
 lisp/image-mode.el   | 57 +++++++++++++++++++++++++++++++-------------
 3 files changed, 60 insertions(+), 19 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 65a57ccd31..3e0788307a 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -2205,11 +2205,11 @@ Image Mode
 behavior by using the options @code{image-auto-resize} and
 @code{image-auto-resize-on-window-resize}.
 
-@findex image-transform-fit-both
+@findex image-transform-fit-to-window
 @findex image-transform-set-scale
 @findex image-transform-reset
 To resize the image manually you can use the command
-@code{image-transform-fit-both} bound to @kbd{s b}
+@code{image-transform-fit-to-window} bound to @kbd{s w}
 that fits the image to both the window height and width.
 To scale the image specifying a scale factor, use the command
 @code{image-transform-set-scale} bound to @kbd{s s}.
diff --git a/etc/NEWS b/etc/NEWS
index 899f3567e6..ffb3345a0c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -268,6 +268,24 @@ To improve security, if an sql product has ':password-in-comint' set
 to t, a password supplied via the minibuffer will be sent in-process,
 as opposed to via the command-line.
 
+** Image Mode
+
++++
+*** New command 'image-transform-fit-to-window'.
+This command fits the image to the current window by scaling down or
+up as necessary.  Unlike 'image-transform-fit-both', this does not
+only scale the image down, but up as well.  It is bound to "s w" in
+Image Mode by default.
+
++++
+*** 'image-transform-fit-to-(height|width)' are now obsolete.
+Use the new command 'image-transform-fit-to-window' instead.
+The keybinding for 'image-transform-fit-to-width' is now 's i'.
+
+---
+*** User option 'image-auto-resize' can now be set to 'fit-window'.
+This works like 'image-transform-fit-to-window'.
+
 ** Image-Dired
 
 +++
diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index 4a326cdc69..a911027a9a 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -58,16 +58,20 @@ image-auto-resize
   "Non-nil to resize the image upon first display.
 Its value should be one of the following:
  - nil, meaning no resizing.
- - t, meaning to fit the image to the window height and width.
+ - t, meaning to scale the image down to fit in the window.
+ - `fit-window', meaning to fit the image to the window.
  - `fit-height', meaning to fit the image to the window height.
  - `fit-width', meaning to fit the image to the window width.
- - A number, which is a scale factor (the default size is 1)."
+ - A number, which is a scale factor (the default size is 1).
+
+Resizing will always preserve the aspect ratio of the image."
   :type '(choice (const :tag "No resizing" nil)
-                 (other :tag "Fit height and width" t)
-                 (const :tag "Fit height" fit-height)
-                 (const :tag "Fit width" fit-width)
+                 (const :tag "Fit to window" fit-window)
+                 (const :tag "Fit to window height" fit-height)
+                 (const :tag "Fit to window width" fit-width)
+                 (other :tag "Scale down to fit window" t)
                  (number :tag "Scale factor" 1))
-  :version "27.1"
+  :version "29.1"
   :group 'image)
 
 (defcustom image-auto-resize-on-window-resize 1
@@ -82,12 +86,16 @@ image-auto-resize-on-window-resize
 
 (defvar-local image-transform-resize nil
   "The image resize operation.
+Non-nil to resize the image upon first display.
 Its value should be one of the following:
  - nil, meaning no resizing.
- - t, meaning to fit the image to the window height and width.
+ - t, meaning to scale the image down to fit in the window.
+ - `fit-window', meaning to fit the image to the window.
  - `fit-height', meaning to fit the image to the window height.
  - `fit-width', meaning to fit the image to the window width.
- - A number, which is a scale factor (the default size is 1).")
+ - A number, which is a scale factor (the default size is 1).
+
+Resizing will always preserve the aspect ratio of the image.")
 
 (defvar-local image-transform-scale 1.0
   "The scale factor of the image being displayed.")
@@ -455,8 +463,9 @@ image-mode-map
 
     ;; Transformation keys
     (define-key map "sf" 'image-mode-fit-frame)
+    (define-key map "sw" 'image-transform-fit-to-window)
     (define-key map "sh" 'image-transform-fit-to-height)
-    (define-key map "sw" 'image-transform-fit-to-width)
+    (define-key map "si" 'image-transform-fit-to-width)
     (define-key map "sb" 'image-transform-fit-both)
     (define-key map "ss" 'image-transform-set-scale)
     (define-key map "sr" 'image-transform-set-rotation)
@@ -511,12 +520,10 @@ image-mode-map
 	"--"
 	["Fit Frame to Image" image-mode-fit-frame :active t
 	 :help "Resize frame to match image"]
-	["Fit Image to Window (Best Fit)" image-transform-fit-both
-	 :help "Resize image to match the window height and width"]
-	["Fit to Window Height" image-transform-fit-to-height
-	 :help "Resize image to match the window height"]
-	["Fit to Window Width" image-transform-fit-to-width
-	 :help "Resize image to match the window width"]
+        ["Fit Image to Window" image-transform-fit-to-window
+         :help "Resize image to match the window height and width"]
+        ["Fit Image to Window (Scale down only)" image-transform-fit-both
+         :help "Scale image down to match the window height and width"]
 	["Zoom In" image-increase-size
 	 :help "Enlarge the image"]
 	["Zoom Out" image-decrease-size
@@ -837,7 +844,8 @@ image-toggle-display-image
 	    filename))
 	 ;; If we have a `fit-width' or a `fit-height', don't limit
 	 ;; the size of the image to the window size.
-	 (edges (when (eq image-transform-resize t)
+         (edges (when (or (eq image-transform-resize t)
+                          (eq image-transform-resize 'fit-window))
 		  (window-inside-pixel-edges (get-buffer-window))))
 	 (max-width (when edges
 		      (- (nth 2 edges) (nth 0 edges))))
@@ -884,6 +892,13 @@ image-toggle-display-image
                                 ;; Type hint.
                                 :format (and filename data-p))))
 
+    ;; Handle `fit-window'.
+    (when (eq image-transform-resize 'fit-window)
+      (setq image
+            (cons (car image)
+                  (plist-put (cdr image) :width
+                             (plist-get (cdr image) :max-width)))))
+
     ;; Discard any stale image data before looking it up again.
     (image-flush image)
     (setq image (append image (image-transform-properties image)))
@@ -1494,21 +1509,29 @@ image-transform-set-scale
 (defun image-transform-fit-to-height ()
   "Fit the current image to the height of the current window."
   (interactive)
+  (declare (obsolete nil "29.1"))
   (setq image-transform-resize 'fit-height)
   (image-toggle-display-image))
 
 (defun image-transform-fit-to-width ()
   "Fit the current image to the width of the current window."
+  (declare (obsolete nil "29.1"))
   (interactive)
   (setq image-transform-resize 'fit-width)
   (image-toggle-display-image))
 
 (defun image-transform-fit-both ()
-  "Fit the current image both to the height and width of the current window."
+  "Scale the current image down to fit in the current window."
   (interactive)
   (setq image-transform-resize t)
   (image-toggle-display-image))
 
+(defun image-transform-fit-to-window ()
+  "Fit the current image to the height and width of the current window."
+  (interactive)
+  (setq image-transform-resize 'fit-window)
+  (image-toggle-display-image))
+
 (defun image-transform-set-rotation (rotation)
   "Prompt for an angle ROTATION, and rotate the image by that amount.
 ROTATION should be in degrees."
-- 
2.30.2


[-- Attachment #3: 0002-New-user-option-image-auto-resize-max-scale-percent.patch --]
[-- Type: text/x-diff, Size: 3218 bytes --]

From 85b8b99b3a14b62fe770670ba795bba40538447c Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 5 Nov 2021 04:22:12 +0100
Subject: [PATCH 2/2] New user option image-auto-resize-max-scale-percent

* lisp/image-mode.el (image-auto-resize-max-scale-percent): New
user option to limit how much 'fit-window' will scale up an image.
(image--scale-within-limits-p): New function.
(image-toggle-display-image): Respect above new user option.
---
 etc/NEWS           |  4 ++++
 lisp/image-mode.el | 27 ++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index ffb3345a0c..d2b348d5b1 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -286,6 +286,10 @@ The keybinding for 'image-transform-fit-to-width' is now 's i'.
 *** User option 'image-auto-resize' can now be set to 'fit-window'.
 This works like 'image-transform-fit-to-window'.
 
+*** New user option 'image-auto-resize-max-scale-percent'.
+The new 'fit-window' options will never scale an image more than this
+much (in percent).  It is nil by default.
+
 ** Image-Dired
 
 +++
diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index a911027a9a..624c852cb8 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -74,6 +74,15 @@ image-auto-resize
   :version "29.1"
   :group 'image)
 
+(defcustom image-auto-resize-max-scale-percent nil
+  "Max size (in percent) to scale up to when `image-auto-resize' is `fit-window'.
+Can be either a number larger than 100, or nil, which means no
+max size."
+  :type '(choice (const nil "No max")
+                 natnum)
+  :version "29.1"
+  :group 'image)
+
 (defcustom image-auto-resize-on-window-resize 1
   "Non-nil to resize the image whenever the window's dimensions change.
 This will always keep the image fit to the window.
@@ -810,6 +819,21 @@ archive-superior-buffer
 (defvar tar-superior-buffer)
 (declare-function image-flush "image.c" (spec &optional frame))
 
+(defun image--scale-within-limits-p (image)
+  "Return t if `fit-window' will scale image within the customized limits.
+The limits are given by the user option
+`image-auto-resize-max-scale-percent'."
+  (or (not image-auto-resize-max-scale-percent)
+      (let ((scale (/ image-auto-resize-max-scale-percent 100))
+            (mw (plist-get (cdr image) :max-width))
+            (mh (plist-get (cdr image) :max-height))
+            ;; Note: `image-size' looks up and thus caches the
+            ;; untransformed image.  There's no easy way to
+            ;; prevent that.
+            (size (image-size image t)))
+        (or (<= mw (* (car size) scale))
+            (<= mh (* (cdr size) scale))))))
+
 (defun image-toggle-display-image ()
   "Show the image of the image file.
 Turn the image data into a real image, but only if the whole file
@@ -893,7 +917,8 @@ image-toggle-display-image
                                 :format (and filename data-p))))
 
     ;; Handle `fit-window'.
-    (when (eq image-transform-resize 'fit-window)
+    (when (and (eq image-transform-resize 'fit-window)
+               (image--scale-within-limits-p image))
       (setq image
             (cons (car image)
                   (plist-put (cdr image) :width
-- 
2.30.2

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> That's true, now that you mention it.  The first patch below therefore
> obsoletes the old commands, mostly to free up the "s w" binding which
> gives the new, and strictly better, behavior.

Both patches make sense to me.

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

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Juri Linkov]

>> That's true, now that you mention it.  The first patch below therefore
>> obsoletes the old commands, mostly to free up the "s w" binding which
>> gives the new, and strictly better, behavior.
>
> Both patches make sense to me.

➕1️⃣

bug#51596: image-transform-resize has inconsistent semantics wrt scaling up/down

[Stefan Kangas]

close 51596 29.1
thanks

Juri Linkov <juri@linkov.net> writes:

>>> That's true, now that you mention it.  The first patch below therefore
>>> obsoletes the old commands, mostly to free up the "s w" binding which
>>> gives the new, and strictly better, behavior.
>>
>> Both patches make sense to me.
>
> ➕1️⃣

💯

Pushed to master as commit fd141116f6 and 569d7f6a73.