unsolicited patch to image-mode.el -- "fit to window"

unsolicited patch to image-mode.el -- "fit to window"

[Greg Minshall]

hi.  the following patch, from the emacs-24 branch, adds an
(image-transform-fit-to-window) to imagemagick mode, similar to
(image-transform-fit-to-width) and (image-transform-fit-to-height).
and, similar to those, a "Fit to Window" menu item.  basically, you get
the smaller of the (image-transfer-fit-to-height) and
(image-transfer-fit-to-width) (which is typically what i want when using
either of those two functions).

(i also have a much smaller patch to image.el which increases the chance
of using imagemagick mode (as opposed to, e.g., jpeg mode) on an image
file.  i'm happy to submit it if there's any interest.)

apologies if there is any protocol i'm missing; i'm a new kid in town.

cheers, Greg Minshall

ps -- details: possibly i went overboard in the lambda's below...
----
diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index 2ed7fbe..8811c72 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -385,6 +385,9 @@ call."
 	["Fit to Window Width" image-transform-fit-to-width
 	 :visible (eq image-type 'imagemagick)
 	 :help "Resize image to match the window width"]
+	["Fit to Window" image-transform-fit-to-window
+	 :visible (eq image-type 'imagemagick)
+	 :help "Resize image to fit in the window"]
 	["Rotate Image..." image-transform-set-rotation
 	 :visible (eq image-type 'imagemagick)
 	 :help "Rotate the image"]
@@ -895,6 +898,7 @@ Its value should be one of the following:
  - nil, meaning no resizing.
  - `fit-height', meaning to fit the image to the window height.
  - `fit-width', meaning to fit the image to the window width.
+ - `fit-window', meaning to fit the image to the window.
  - A number, which is a scale factor (the default size is 1).")
 
 (defvar image-transform-scale 1.0
@@ -1007,17 +1011,18 @@ of a rotated image."
   (when (and (not (numberp image-transform-resize))
 	     (boundp 'image-type)
 	     (eq image-type 'imagemagick))
-    (let ((size (image-display-size (image-get-display-property) t)))
-      (cond ((eq image-transform-resize 'fit-width)
-	     (cl-assert (= (car size)
-			(- (nth 2 (window-inside-pixel-edges))
+    (let ((size (image-display-size (image-get-display-property) t))
+          (width (- (nth 2 (window-inside-pixel-edges))
 			   (nth 0 (window-inside-pixel-edges))))
-		     t))
+          (height (- (nth 3 (window-inside-pixel-edges))
+			   (nth 1 (window-inside-pixel-edges)))))
+      (cond ((eq image-transform-resize 'fit-width)
+	     (cl-assert (= (car size) width) t))
 	    ((eq image-transform-resize 'fit-height)
-	     (cl-assert (= (cdr size)
-			(- (nth 3 (window-inside-pixel-edges))
-			   (nth 1 (window-inside-pixel-edges))))
-		     t))))))
+	     (cl-assert (= (cdr size) height) t) )
+	    ((eq image-transform-resize 'fit-window)
+	     (cl-assert (or (= (car size) width)
+                            (= (cdr size) height)) t))))))
 
 (defun image-transform-properties (spec)
   "Return rescaling/rotation properties for image SPEC.
@@ -1032,7 +1037,26 @@ compiled with ImageMagick support."
 	    (/= image-transform-rotation 0.0))
     ;; Note: `image-size' looks up and thus caches the untransformed
     ;; image.  There's no easy way to prevent that.
-    (let* ((size (image-size spec t))
+    (let* ((size (image-size spec t))   ; (car size) == width;
+                                        ; (cdr size) == height
+           (ifwidth (lambda () 
+                      (image-transform-fit-width
+                       (car size) (cdr size)
+                       (- (nth 2 (window-inside-pixel-edges))
+                          (nth 0 (window-inside-pixel-edges))))))
+           (ifheight (lambda ()
+                       (let ((res (image-transform-fit-width
+                                   (cdr size) (car size)
+                                   (- (nth 3 (window-inside-pixel-edges))
+                                      (nth 1 (window-inside-pixel-edges))))))
+                         (cons (cdr res) (car res)))))
+           (ifwindow (lambda ()
+                       (let ((ifw (funcall ifwidth))
+                             (ifh (funcall ifheight)))
+                         (if (< (/ (float (car ifw)) (car size))
+                                (/ (float (cdr ifh)) (cdr size)))
+                             ifw
+                           ifh))))
 	   (resized
 	    (cond
 	     ((numberp image-transform-resize)
@@ -1040,16 +1064,11 @@ compiled with ImageMagick support."
 		(setq image-transform-scale image-transform-resize)
 		(cons nil (floor (* image-transform-resize (cdr size))))))
 	     ((eq image-transform-resize 'fit-width)
-	      (image-transform-fit-width
-	       (car size) (cdr size)
-	       (- (nth 2 (window-inside-pixel-edges))
-		  (nth 0 (window-inside-pixel-edges)))))
+              (funcall ifwidth))
 	     ((eq image-transform-resize 'fit-height)
-	      (let ((res (image-transform-fit-width
-			  (cdr size) (car size)
-			  (- (nth 3 (window-inside-pixel-edges))
-			     (nth 1 (window-inside-pixel-edges))))))
-		(cons (cdr res) (car res)))))))
+              (funcall ifheight))
+             ((eq image-transform-resize 'fit-window)
+              (funcall ifwindow)))))
       `(,@(when (car resized)
 	    (list :width (car resized)))
 	,@(when (cdr resized)
@@ -1081,6 +1100,14 @@ ImageMagick support."
   (setq image-transform-resize 'fit-width)
   (image-toggle-display-image))
 
+(defun image-transform-fit-to-window ()
+  "Fit the current image to the width of the current window.
+This command has no effect unless Emacs is compiled with
+ImageMagick support."
+  (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.  This command has no effect unless

Re: unsolicited patch to image-mode.el -- "fit to window"

[Zack Piper]

> apologies if there is any protocol i'm missing; i'm a new kid in town.

No worries, heh.

You need to sign the FSF papers.


https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html

-- 
Zack Piper <zack@apertron.net> http://apertron.net

Re: unsolicited patch to image-mode.el -- "fit to window"

[Greg Minshall]

sadly, there's a bug with that patch (involving rotating images).  i
will try to fix and re-submit.

Re: unsolicited patch to image-mode.el -- "fit to window"

[Stefan Monnier]

> sadly, there's a bug with that patch (involving rotating images).  i
> will try to fix and re-submit.

Thanks.


        Stefan

Re: unsolicited patch to image-mode.el -- "fit to window"

[Greg Minshall]

sorry again for the defective patch.  below is a reformed patch that,
after more testing, appears to work.  again, this patch adds a
'fit-window option to the Image menu when viewing images, which resizes
the image (maintaining its aspect ratio) to fit within both the height
and width of the containing window.

i've filled out and submitted GNU paperwork.  please let me know if the
paperwork has, or if the patch retains, deficiencies.

cheers, Greg Minshall
----
diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index 2ed7fbe..6377699 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -385,6 +385,9 @@ call."
 	["Fit to Window Width" image-transform-fit-to-width
 	 :visible (eq image-type 'imagemagick)
 	 :help "Resize image to match the window width"]
+	["Fit to Window" image-transform-fit-to-window
+	 :visible (eq image-type 'imagemagick)
+	 :help "Resize image to fit in the window"]
 	["Rotate Image..." image-transform-set-rotation
 	 :visible (eq image-type 'imagemagick)
 	 :help "Rotate the image"]
@@ -895,6 +898,7 @@ Its value should be one of the following:
  - nil, meaning no resizing.
  - `fit-height', meaning to fit the image to the window height.
  - `fit-width', meaning to fit the image to the window width.
+ - `fit-window', meaning to fit the image to the window.
  - A number, which is a scale factor (the default size is 1).")
 
 (defvar image-transform-scale 1.0
@@ -952,6 +956,9 @@ a slightly different angle.  Currently this is done for values
 close to a multiple of 90, see `image-transform-right-angle-fudge'."
   (cond ((< (abs (- (mod (+ image-transform-rotation 90) 180) 90))
 	    image-transform-right-angle-fudge)
+         ;; in this case, there is either no rotation (close to zero)
+         ;; or a 180 degree rotation.  in either case, the width of
+         ;; the image's post-rotation bounding box doesn't change.
 	 (cl-assert (not (zerop width)) t)
 	 (setq image-transform-rotation
 	       (float (round image-transform-rotation))
@@ -959,6 +966,9 @@ close to a multiple of 90, see `image-transform-right-angle-fudge'."
 	 (cons length nil))
 	((< (abs (- (mod (+ image-transform-rotation 45) 90) 45))
 	    image-transform-right-angle-fudge)
+         ;; in this case, the rotation is 90 or 270 degrees, i.e., the
+         ;; width and height of the image are flipped.  so, we use the
+         ;; image height as the new image width.
 	 (cl-assert (not (zerop height)) t)
 	 (setq image-transform-rotation
 	       (float (round image-transform-rotation))
@@ -1007,17 +1017,18 @@ of a rotated image."
   (when (and (not (numberp image-transform-resize))
 	     (boundp 'image-type)
 	     (eq image-type 'imagemagick))
-    (let ((size (image-display-size (image-get-display-property) t)))
-      (cond ((eq image-transform-resize 'fit-width)
-	     (cl-assert (= (car size)
-			(- (nth 2 (window-inside-pixel-edges))
+    (let ((size (image-display-size (image-get-display-property) t))
+          (width (- (nth 2 (window-inside-pixel-edges))
 			   (nth 0 (window-inside-pixel-edges))))
-		     t))
+          (height (- (nth 3 (window-inside-pixel-edges))
+			   (nth 1 (window-inside-pixel-edges)))))
+      (cond ((eq image-transform-resize 'fit-width)
+	     (cl-assert (= (car size) width) t))
 	    ((eq image-transform-resize 'fit-height)
-	     (cl-assert (= (cdr size)
-			(- (nth 3 (window-inside-pixel-edges))
-			   (nth 1 (window-inside-pixel-edges))))
-		     t))))))
+	     (cl-assert (= (cdr size) height) t) )
+	    ((eq image-transform-resize 'fit-window)
+	     (cl-assert (or (= (car size) width)
+                            (= (cdr size) height)) t))))))
 
 (defun image-transform-properties (spec)
   "Return rescaling/rotation properties for image SPEC.
@@ -1032,24 +1043,38 @@ compiled with ImageMagick support."
 	    (/= image-transform-rotation 0.0))
     ;; Note: `image-size' looks up and thus caches the untransformed
     ;; image.  There's no easy way to prevent that.
-    (let* ((size (image-size spec t))
+    (let* ((isize (image-size spec t))  ; (car isize) == width;
+                                        ; (cdr isize) == height
+           (wpixels (window-inside-pixel-edges))
+           (wsize (cons (- (nth 2 wpixels) (nth 0 wpixels))
+                        (- (nth 3 wpixels) (nth 1 wpixels)))) ; ditto
+           (ifwidth (lambda ()
+                      (image-transform-fit-width
+                       (car isize) (cdr isize) (car wsize))))
+           (ifheight (lambda ()
+                       (let ((res (image-transform-fit-width
+                                   (cdr isize) (car isize) (cdr wsize))))
+                         (cons (cdr res) (car res)))))
+           (ifwindow (lambda ()
+              ;; is the constraint the *width* or the *height* of the
+              ;; image?  we can tell this by comparing the *aspect*
+              ;; ratio of the image with that of the window
+              (if (>= (/ (float (car isize)) (cdr isize))
+                      (/ (float (car wsize)) (cdr wsize)))
+                  (funcall ifwidth)
+                (funcall ifheight))))
 	   (resized
 	    (cond
 	     ((numberp image-transform-resize)
 	      (unless (= image-transform-resize 1)
 		(setq image-transform-scale image-transform-resize)
-		(cons nil (floor (* image-transform-resize (cdr size))))))
+		(cons nil (floor (* image-transform-resize (cdr isize))))))
 	     ((eq image-transform-resize 'fit-width)
-	      (image-transform-fit-width
-	       (car size) (cdr size)
-	       (- (nth 2 (window-inside-pixel-edges))
-		  (nth 0 (window-inside-pixel-edges)))))
+              (funcall ifwidth))
 	     ((eq image-transform-resize 'fit-height)
-	      (let ((res (image-transform-fit-width
-			  (cdr size) (car size)
-			  (- (nth 3 (window-inside-pixel-edges))
-			     (nth 1 (window-inside-pixel-edges))))))
-		(cons (cdr res) (car res)))))))
+              (funcall ifheight))
+             ((eq image-transform-resize 'fit-window)
+              (funcall ifwindow)))))
       `(,@(when (car resized)
 	    (list :width (car resized)))
 	,@(when (cdr resized)
@@ -1081,6 +1106,14 @@ ImageMagick support."
   (setq image-transform-resize 'fit-width)
   (image-toggle-display-image))
 
+(defun image-transform-fit-to-window ()
+  "Fit the current image to the width of the current window.
+This command has no effect unless Emacs is compiled with
+ImageMagick support."
+  (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.  This command has no effect unless

Re: unsolicited patch to image-mode.el -- "fit to window"

[martin rudalics]

+           (wpixels (window-inside-pixel-edges))
+           (wsize (cons (- (nth 2 wpixels) (nth 0 wpixels))
+                        (- (nth 3 wpixels) (nth 1 wpixels)))) ; ditto

Hopefully you should be able to replace this with

+           (wsize (cons (window-body-width nil t) (window-body-height nil t))

martin

Re: unsolicited patch to image-mode.el -- "fit to window"

[Greg Minshall]

Martin,

> +           (wpixels (window-inside-pixel-edges))
> +           (wsize (cons (- (nth 2 wpixels) (nth 0 wpixels))
> +                        (- (nth 3 wpixels) (nth 1 wpixels)))) ; ditto
> 
> Hopefully you should be able to replace this with
> 
> +           (wsize (cons (window-body-width nil t) (window-body-height nil t))

thanks, that's much clearer.

a "coding philosophy" question, though. there are a dozen or so other
references to (window-size-pixel-edges) (and its associated (nth)'s) in
the code.  my tendency would be to change none (in a patch) or all (in a
more substantive re-write).  changing all -- while "trivial" -- is, of
course, more likely to introduce bug, and i don't know of any test suite
for image mode?

thoughts?

cheers, Greg Minshall

Re: unsolicited patch to image-mode.el -- "fit to window"

[martin rudalics]

 > a "coding philosophy" question, though. there are a dozen or so other
 > references to (window-size-pixel-edges) (and its associated (nth)'s) in
 > the code.  my tendency would be to change none (in a patch) or all (in a
 > more substantive re-write).  changing all -- while "trivial" -- is, of
 > course, more likely to introduce bug, and i don't know of any test suite
 > for image mode?

If such code is not buggy, it should be changed only when and only by
the person(s) working on it.  Otherwise, as you say, it's too easy to
introduce a bug in code that was correct before.

martin

Re: unsolicited patch to image-mode.el -- "fit to window"

[Greg Minshall]

> If such code is not buggy, it should be changed only when and only by
> the person(s) working on it.  Otherwise, as you say, it's too easy to
> introduce a bug in code that was correct before.

right, that seems like a good rule to me.  so, i'd tend to leave my code
with the same pattern used in the rest of the file (i.e., using
(window-size-pixel-edges)), just for consistency.  (but, even a very
light breeze would be enough to cause me to change it to use the clearer
calls, so feel free to so breathe.)

cheers, Greg

Re: unsolicited patch to image-mode.el -- "fit to window"

[martin rudalics]

 > right, that seems like a good rule to me.  so, i'd tend to leave my code
 > with the same pattern used in the rest of the file (i.e., using
 > (window-size-pixel-edges)), just for consistency.

I don't understand the "so" ;-)

 > (but, even a very
 > light breeze would be enough to cause me to change it to use the clearer
 > calls, so feel free to so breathe.)

I can't test your patch because I build without image support.  But I
have two questions: (1) Why can't you use `fit-window-to-buffer'?  (2)
If the image appears on a frame alone, can you fit the frame to the
image?  Can `fit-frame-to-buffer' fit the frame to the image?  Both of
these should work, in principle.

martin

Re: unsolicited patch to image-mode.el -- "fit to window"

[martin rudalics]

 > I can't test your patch because I build without image support.  But I
 > have two questions: (1) Why can't you use `fit-window-to-buffer'?  (2)
 > If the image appears on a frame alone, can you fit the frame to the
 > image?  Can `fit-frame-to-buffer' fit the frame to the image?  Both of
 > these should work, in principle.

Disregard this.  After re-reading your patch I noticed that you fit the
image to the window and not vice-versa.

martin

Re: unsolicited patch to image-mode.el -- "fit to window"

[Ted Zlatanov]

On Thu, 23 Jul 2015 20:52:33 +0300 Greg Minshall <minshall@acm.org> wrote: 

GM> this patch adds a 'fit-window option to the Image menu when viewing
GM> images, which resizes the image (maintaining its aspect ratio) to
GM> fit within both the height and width of the containing window.

This is useful, thank you for writing it.  Do you think a default
keyboard shortcut might be a good idea for this command?  I'd find it
useful; the shorter the better (but I don't know if it's possible to
make it a single character without breaking other tools).

Ted

Re: unsolicited patch to image-mode.el -- "fit to window"

[Stefan Monnier]

> (i also have a much smaller patch to image.el which increases the chance
> of using imagemagick mode (as opposed to, e.g., jpeg mode) on an image
> file.  i'm happy to submit it if there's any interest.)

I can't say whether there'll be interest, but you can send it via M-x
report-emacs-bug, explaining why it's a good idea.

> apologies if there is any protocol i'm missing; i'm a new kid in town.

Patches sent here a fine, tho we also like to see them in bug-gnu-emacs
(e.g. via M-x report-emacs-bug) so they get a tracking number.


        Stefan