bug#35421: Also bind left image rotation key

bug#35421: Also bind left image rotation key

[積丹尼 Dan Jacobson]

When viewing an image we type C-h m

   Image[imagemagick] mode defined in ‘image-mode.el’:

there is only one rotate command bound

   r		image-rotate

and it only rotates right. To rotate left one must hit it three times.

Also if we could see the whole image before rotation, its bottom is now
cut off after rotation. (landscape->portrait).

bug#35421: Also bind left image rotation key

[Lars Ingebrigtsen]

積丹尼 Dan Jacobson <jidanni@jidanni.org> writes:

> When viewing an image we type C-h m
>
>    Image[imagemagick] mode defined in ‘image-mode.el’:
>
> there is only one rotate command bound
>
>    r		image-rotate
>
> and it only rotates right. To rotate left one must hit it three times.

I don't think it's worth adding more keystrokes to the image map --
images can appear in any mode and it's better to keep the number of
keystrokes limited.

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

bug#35421: Also bind left image rotation key

[積丹尼 Dan Jacobson]

Can an adding an argument reverse the direction?

bug#35421: Also bind left image rotation key

[積丹尼 Dan Jacobson]

Or maybe [argument x 90 degrees].

bug#35421: Also bind left image rotation key

[Basil L. Contovounesios]

reopen 35421
tags 35421 - wontfix
quit

Lars Ingebrigtsen <larsi@gnus.org> writes:

> 積丹尼 Dan Jacobson <jidanni@jidanni.org> writes:
>
>> When viewing an image we type C-h m
>>
>>    Image[imagemagick] mode defined in ‘image-mode.el’:
>>
>> there is only one rotate command bound
>>
>>    r		image-rotate
>>
>> and it only rotates right. To rotate left one must hit it three times.
>
> I don't think it's worth adding more keystrokes to the image map --
> images can appear in any mode and it's better to keep the number of
> keystrokes limited.

I had a patch prepared for giving image-rotate an optional prefix
argument, but then I got distracted by some imagemagick vs native
transformation issues, so I was waiting for the native support to
stabilise a bit before returning to this in earnest.  Should have
something to show within a day or two.

Thanks,

-- 
Basil

bug#35421: Also bind left image rotation key

[積丹尼 Dan Jacobson]

args
1,2,3=
90,180,270
or something like that.

bug#35421: Also bind left image rotation key

[Basil L. Contovounesios]

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

tags 35421 + patch
quit

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I had a patch prepared for giving image-rotate an optional prefix
> argument, but then I got distracted by some imagemagick vs native
> transformation issues, so I was waiting for the native support to
> stabilise a bit before returning to this in earnest.  Should have
> something to show within a day or two.

With a slight delay due to a camping trip, here it is:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-counter-clockwise-rotations-in-image-rotate.patch --]
[-- Type: text/x-diff, Size: 4663 bytes --]

From 38e676d04f12a69289d4095bb9f61247c4f08ae8 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 16 Jul 2019 22:51:27 +0100
Subject: [PATCH] Allow counter-clockwise rotations in image-rotate

* lisp/image.el (image-rotate): Extend with an optional argument
specifying the rotation in degrees (bug#35421).
* doc/lispref/display.texi (Showing Images):
* etc/NEWS: Document the change.
* test/lisp/image-tests.el (image-rotate): New test.
---
 doc/lispref/display.texi |  3 ++-
 etc/NEWS                 |  5 +++++
 lisp/image.el            | 22 +++++++++++++---------
 test/lisp/image-tests.el | 23 +++++++++++++++++++++++
 4 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index a38569f726..4b10788862 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5992,7 +5992,8 @@ Showing Images
 of @samp{4} means to decrease the size by 40%.  The default is 20%.
 
 @item r
-Rotate the image by 90 degrees (@code{image-rotate}).
+Rotate the image by 90 degrees clockwise (@code{image-rotate}).
+A prefix means to rotate by 90 degrees counter-clockwise instead.
 
 @item o
 Save the image to a file (@code{image-save}).
diff --git a/etc/NEWS b/etc/NEWS
index 76ea1df821..4cc30dfcbd 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2237,6 +2237,11 @@ The image parameters 'image-transform-rotation',
 buffer-local, so each buffer could have its own values for these
 parameters.
 
++++
+*** The command 'image-rotate' now accepts a prefix argument.
+With a prefix argument, 'image-rotate' now rotates the image at point
+90 degrees counter-clockwise, instead of the default clockwise.
+
 ** Modules
 
 *** The function 'load' now behaves correctly when loading modules.
diff --git a/lisp/image.el b/lisp/image.el
index b58b1dc954..c3e28655c3 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -1028,16 +1028,20 @@ image--current-scaling
         (display-width (car (image-size image t))))
     (/ (float display-width) image-width)))
 
-(defun image-rotate ()
-  "Rotate the image under point by 90 degrees clockwise."
-  (interactive)
+(defun image-rotate (&optional angle)
+  "Rotate the image under point by ANGLE degrees clockwise.
+If nil, ANGLE defaults to 90.  Interactively, rotate the image 90
+degrees clockwise with no prefix argument, and counter-clockwise
+with a prefix argument.  Note that most image types support
+rotations by only multiples of 90 degrees."
+  (interactive (and current-prefix-arg '(-90)))
   (let ((image (image--get-imagemagick-and-warn)))
-    (plist-put (cdr image) :rotation
-               (float (mod (+ (or (plist-get (cdr image) :rotation) 0) 90)
-                           ;; We don't want to exceed 360 degrees
-                           ;; rotation, because it's not seen as valid
-                           ;; in exif data.
-                           360)))))
+    (setf (image-property image :rotation)
+          (float (mod (+ (or (image-property image :rotation) 0)
+                         (or angle 90))
+                      ;; We don't want to exceed 360 degrees rotation,
+                      ;; because it's not seen as valid in Exif data.
+                      360)))))
 
 (defun image-save ()
   "Save the image under point."
diff --git a/test/lisp/image-tests.el b/test/lisp/image-tests.el
index 5a5b8ea1f7..01c81e3022 100644
--- a/test/lisp/image-tests.el
+++ b/test/lisp/image-tests.el
@@ -21,6 +21,8 @@
 
 (require 'ert)
 (require 'image)
+(eval-when-compile
+  (require 'cl-lib))
 
 (defconst image-tests--emacs-images-directory
   (expand-file-name "../etc/images" (getenv "EMACS_TEST_DIRECTORY"))
@@ -53,4 +55,25 @@ image-type-from-file-header-test
 	       (expand-file-name "splash.svg"
 				 image-tests--emacs-images-directory)))))
 
+(ert-deftest image-rotate ()
+  "Test `image-rotate'."
+  (cl-letf* ((image (list 'image))
+             ((symbol-function 'image--get-imagemagick-and-warn)
+              (lambda () image)))
+    (let ((current-prefix-arg '(4)))
+      (call-interactively #'image-rotate))
+    (should (equal image '(image :rotation 270.0)))
+    (call-interactively #'image-rotate)
+    (should (equal image '(image :rotation 0.0)))
+    (image-rotate)
+    (should (equal image '(image :rotation 90.0)))
+    (image-rotate 0)
+    (should (equal image '(image :rotation 90.0)))
+    (image-rotate 1)
+    (should (equal image '(image :rotation 91.0)))
+    (image-rotate 1234.5)
+    (should (equal image '(image :rotation 245.5)))
+    (image-rotate -154.5)
+    (should (equal image '(image :rotation 91.0)))))
+
 ;;; image-tests.el ends here
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 518 bytes --]


I think this is a good solution for several reasons:

1. It does not require a new key binding, and it does not overly
   complicate the calling convention of the current key binding.

2. It turns image-rotate into a general image-rotating subroutine,
   which users and library authors alike can reuse.

2.1. It does not limit rotations of imagemagick images to multiples of
     90 degrees.

and I don't see any drawbacks, so I would like to push this to master,
subject to comments/objections.

Thanks,

-- 
Basil

bug#35421: Also bind left image rotation key

[Basil L. Contovounesios]

積丹尼 Dan Jacobson <jidanni@jidanni.org> writes:

> args
> 1,2,3=
> 90,180,270
> or something like that.

Image viewers like gThumb provide buttons for rotations in steps of 90
degrees, and Emacs isn't even an image viewer, so there's no need to
complicate and restrict (w.r.t. future changes) the command's calling
convention like that.

Instead, the proposed patch[1] makes the simplest possible change to
support counter-clockwise rotations in one step, rather than three.  In
addition, it makes image-rotate accept arbitrary rotation angles when
called from Lisp, so you can use it to easily write your own
image-rotating command, with your preferred calling convention.

[1]: https://debbugs.gnu.org/35421#31

-- 
Basil

bug#35421: Also bind left image rotation key

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I think this is a good solution for several reasons:
>
> 1. It does not require a new key binding, and it does not overly
>    complicate the calling convention of the current key binding.
>
> 2. It turns image-rotate into a general image-rotating subroutine,
>    which users and library authors alike can reuse.
>
> 2.1. It does not limit rotations of imagemagick images to multiples of
>      90 degrees.
>
> and I don't see any drawbacks, so I would like to push this to master,
> subject to comments/objections.

Looks good to me.

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

bug#35421: Also bind left image rotation key

[積丹尼 Dan Jacobson]

OK but add to Docstring/Info:
To rotate (right) by 90, hit ...
To rotate (right) by 180, hit ...
To rotate (right) by 270, hit ...
As these are the most common tasks.

(I'm most curious as to the recommended sequences the users should hit,
especially for 180.

bug#35421: Also bind left image rotation key

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Looks good to me.

Thanks, pushed to master:

Allow counter-clockwise rotations in image-rotate
b728620a75 2019-07-20 16:00:31 +0100
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b728620a756db78b8cb0a41afa72db6209102cdf

-- 
Basil

bug#35421: Also bind left image rotation key

[Basil L. Contovounesios]

tags 35421 fixed
close 35421 27.1
quit

積丹尼 Dan Jacobson <jidanni@jidanni.org> writes:

> OK but add to Docstring/Info:
> To rotate (right) by 90, hit ...
> To rotate (right) by 180, hit ...
> To rotate (right) by 270, hit ...
> As these are the most common tasks.

That would be superfluous, as the documentation in the applied patch
already explains how to rotate by multiples of 90 degrees interactively,
and by an arbitrary angle from Lisp.

So I'm closing this report as done.

> (I'm most curious as to the recommended sequences the users should hit,
> especially for 180.

A rotation of 180 degrees is equivalent to two consecutive rotations of
90 degrees in either direction.

In vanilla Emacs that corresponds to 'r r' or 'C-u r C-u r'.
In custom Emacs it could be bound to any number of preferred keys.

Thanks,

-- 
Basil