bug#10758: thumbs and image-dired to use built-in ImageMagick

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Glenn Morris]

Package: emacs
Version: 24.0.93
Severity: wishlist

thumbs.el and image-dired.el both rely on using external ImageMagick
utilities to resize/rotate images.

If Emacs was compiled with ImageMagick support, then images of
type imagemagick can be resized and rotated internally (see
image-transform-*). These modes should use the internal support if
it exists.

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Glenn Morris <rgm@gnu.org> writes:

> Package: emacs
> Version: 24.0.93
> Severity: wishlist
>
> thumbs.el and image-dired.el both rely on using external ImageMagick
> utilities to resize/rotate images.
>
> If Emacs was compiled with ImageMagick support, then images of
> type imagemagick can be resized and rotated internally (see
> image-transform-*). These modes should use the internal support if
> it exists.

So now that we have native image rotation and ImageMagick is on its
way to become deprecated, I assume that this bug should be taken to
mean that we want to use the built-in support here instead.

Best regards,
Stefan Kangas

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> Glenn Morris <rgm@gnu.org> writes:
>
>> Package: emacs
>> Version: 24.0.93
>> Severity: wishlist
>>
>> thumbs.el and image-dired.el both rely on using external ImageMagick
>> utilities to resize/rotate images.
>>
>> If Emacs was compiled with ImageMagick support, then images of
>> type imagemagick can be resized and rotated internally (see
>> image-transform-*). These modes should use the internal support if
>> it exists.
>
> So now that we have native image rotation and ImageMagick is on its
> way to become deprecated, I assume that this bug should be taken to
> mean that we want to use the built-in support here instead.

Hmm, looking closer at this, both thumbs.el and image-dired.el modifies
the actual image file.  This is something that our built-in support
can't do, as we only have support for reading images.

However, the ImageMagick library has these capabilities.  Given that we
no longer compile with ImageMagick by default, I don't think this
request makes any sense as it stands.  Perhaps we could implement the
in-memory rotation in addition to the modifying one that we have now,
but this is unlikely to be very useful.  If I rotate an image in an
image viewer, it is almost certainly because I want to update the file;
at least this is always the case in my use.

Jpeg images can of course be losslessly rotated by just modifying the
exif data, so implementing built-in support for rotating jpeg images
seems quite doable.  However, we would still not be able to drop the
external imagemagick dependency, because we would then also lose the
capability to rotate other image formats.

So perhaps this feature request is relevant anymore.  WDYT?

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> So perhaps this feature request is relevant anymore.  WDYT?

I now see that Glenn mentioned resizing as well, where using the
built-in image scaling seems very relevant.  We currently jump through
some unnecessary hoops in `image-dired-display-image' when we could just
resize the image in memory.

Is there any reason why `image-dired-display-image-mode' doesn't just
inherit `image-mode'?  That would avoid code duplication and also allow
us to immediately benefit from any improvements to `image-mode'.

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> So perhaps this feature request is relevant anymore.  WDYT?
>
> I now see that Glenn mentioned resizing as well, where using the
> built-in image scaling seems very relevant.  We currently jump through
> some unnecessary hoops in `image-dired-display-image' when we could just
> resize the image in memory.
>
> Is there any reason why `image-dired-display-image-mode' doesn't just
> inherit `image-mode'?  That would avoid code duplication and also allow
> us to immediately benefit from any improvements to `image-mode'.

Resending to include Mathias Dahl in the conversation.

(I missed removing ".rem0veth1s" from his listed email address.)

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Mathias Dahl]

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

On Sun, Oct 24, 2021 at 11:45 AM Stefan Kangas <stefan@marxist.se> wrote:

> Stefan Kangas <stefan@marxist.se> writes:
>
> > Stefan Kangas <stefan@marxist.se> writes:
> >
> >> So perhaps this feature request is relevant anymore.  WDYT?
> >
> > I now see that Glenn mentioned resizing as well, where using the
> > built-in image scaling seems very relevant.  We currently jump through
> > some unnecessary hoops in `image-dired-display-image' when we could just
> > resize the image in memory.
> >
> > Is there any reason why `image-dired-display-image-mode' doesn't just
> > inherit `image-mode'?  That would avoid code duplication and also allow
> > us to immediately benefit from any improvements to `image-mode'.


Hi, sorry for coming late to the party.

I cannot remember any good reason for not inheriting from `image-mode', but
I opened image-dired.el now and found this:

(defvar image-dired-display-image-mode-map
  (let ((map (make-sparse-keymap)))
    ;; `image-mode-map' has bindings that do not make sense in image-dired
    ;; (set-keymap-parent map image-mode-map)
    (define-key map "f" 'image-dired-display-current-image-full)
    (define-key map "s" 'image-dired-display-current-image-sized)
    (define-key map "g" nil)

    ;; Useful bindings from `image-mode-map'
    (define-key map [remap forward-char] 'image-forward-hscroll)
    (define-key map [remap backward-char] 'image-backward-hscroll)
...

It seems I knew about `image-mode' but perhaps it never crossed my mind
to inherit from it. 16 years later, my memory fails me...

At any rate I have nothing against it, if someone wants to give it a try.

/Mathias

[-- Attachment #2: Type: text/html, Size: 2451 bytes --]

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Mathias Dahl <mathias.dahl@gmail.com> writes:

> It seems I knew about `image-mode' but perhaps it never crossed my mind
> to inherit from it. 16 years later, my memory fails me...
>
> At any rate I have nothing against it, if someone wants to give it a try.

Thank you, that is useful information.  I intend to look into this.

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

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

Stefan Kangas <stefan@marxist.se> writes:

> Mathias Dahl <mathias.dahl@gmail.com> writes:
>
>> It seems I knew about `image-mode' but perhaps it never crossed my mind
>> to inherit from it. 16 years later, my memory fails me...
>>
>> At any rate I have nothing against it, if someone wants to give it a try.

See the attached patch.  Unsurprisingly, this makes `image-dired' feel
quite a bit snappier, as we avoid a lot of unnecessary work.

[-- Attachment #2: 0001-Make-image-dired-display-image-mode-inherit-from-ima.patch --]
[-- Type: text/x-diff, Size: 13672 bytes --]

From 0dbcb88660b6c4c0d084446e9f367938319da254 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sat, 23 Oct 2021 16:06:05 +0200
Subject: [PATCH] Make image-dired-display-image-mode inherit from image-mode

* lisp/image-dired.el (image-dired-display-image-mode):
Inherit from 'image-mode'.  (Bug#10758)
(image-dired-display-image): Simplify: no longer generate
temporary image, which is unnecessary with the above change.
(image-dired-display-thumbnail-original-image)
(image-dired-dired-display-image)
(image-dired-mouse-display-image): Simplify.

(image-dired-cmd-create-temp-image-program)
(image-dired-cmd-create-temp-image-options)
(image-dired-display-window-width-correction)
(image-dired-display-window-height-correction)
(image-dired-display-window-width)
(image-dired-display-window-height)
(image-dired-window-height-pixels): Make obsolete.
(image-dired-create-display-image-buffer): Make into obsolete
function alias for 'ignore'.
(image-dired-minor-mode): Doc fix.
---
 lisp/image-dired.el | 194 +++++++++++++++++++-------------------------
 1 file changed, 82 insertions(+), 112 deletions(-)

diff --git a/lisp/image-dired.el b/lisp/image-dired.el
index 122d8a5d9a..ffbe89a429 100644
--- a/lisp/image-dired.el
+++ b/lisp/image-dired.el
@@ -278,27 +278,6 @@ image-dired-cmd-create-thumbnail-options
   :version "29.1"
   :type '(repeat (string :tag "Argument")))
 
-(defcustom image-dired-cmd-create-temp-image-program
-  (if (executable-find "gm") "gm" "convert")
-  "Executable used to create temporary image.
-Used together with `image-dired-cmd-create-temp-image-options'."
-  :type 'file
-  :version "29.1")
-
-(defcustom image-dired-cmd-create-temp-image-options
-  (let ((opts '("-size" "%wx%h" "%f[0]"
-                "-resize" "%wx%h>"
-                "-strip" "jpeg:%t")))
-    (if (executable-find "gm") (cons "convert" opts) opts))
-  "Options of command used to create temporary image for display window.
-Used together with `image-dired-cmd-create-temp-image-program',
-Available format specifiers are: %w and %h which are replaced by
-the calculated max size for width and height in the image display window,
-%f which is replaced by the file name of the original image and %t which
-is replaced by the file name of the temporary file."
-  :version "29.1"
-  :type '(repeat (string :tag "Argument")))
-
 (defcustom image-dired-cmd-pngnq-program
   ;; Prefer pngquant to pngnq-s9 as it is faster on my machine.
   ;;   The project also seems more active than the alternatives.
@@ -509,18 +488,6 @@ image-dired-thumbs-per-row
   "Number of thumbnails to display per row in thumb buffer."
   :type 'integer)
 
-(defcustom image-dired-display-window-width-correction 1
-  "Number to be used to correct image display window width.
-Change if the default (1) does not work (i.e. if the image does not
-completely fit)."
-  :type 'integer)
-
-(defcustom image-dired-display-window-height-correction 0
-  "Number to be used to correct image display window height.
-Change if the default (0) does not work (i.e. if the image does not
-completely fit)."
-  :type 'integer)
-
 (defcustom image-dired-track-movement t
   "The current state of the tracking and mirroring.
 For more information, see the documentation for
@@ -1014,15 +981,6 @@ image-dired-create-thumbnail-buffer
 (defvar image-dired-display-image-buffer "*image-dired-display-image*"
   "Where larger versions of the images are display.")
 
-(defun image-dired-create-display-image-buffer ()
-  "Create image display buffer and set `image-dired-display-image-mode'."
-  (let ((buf (get-buffer-create image-dired-display-image-buffer)))
-    (with-current-buffer buf
-      (setq buffer-read-only t)
-      (if (not (eq major-mode 'image-dired-display-image-mode))
-          (image-dired-display-image-mode)))
-    buf))
-
 (defvar image-dired-saved-window-configuration nil
   "Saved window configuration.")
 
@@ -1046,7 +1004,7 @@ image-dired-dired-with-window-configuration
 calling `image-dired-restore-window-configuration'."
   (interactive "DDirectory: \nP")
   (let ((buf (image-dired-create-thumbnail-buffer))
-        (buf2 (image-dired-create-display-image-buffer)))
+        (buf2 (get-buffer-create image-dired-display-image-buffer)))
     (setq image-dired-saved-window-configuration
           (current-window-configuration))
     (dired dir)
@@ -1783,13 +1741,11 @@ image-dired-thumbnail-mode
   (setq-local line-spacing (frame-char-width)))
 
 (define-derived-mode image-dired-display-image-mode
-  special-mode "image-dired-image-display"
+  image-mode "image-dired-image-display"
   "Mode for displaying and manipulating original image.
 Resized or in full-size."
-  (buffer-disable-undo)
-  (image-mode-setup-winprops)
-  (setq cursor-type nil)
-  (add-hook 'file-name-at-point-functions 'image-dired-file-name-at-point nil t))
+  :interactive nil
+  (add-hook 'file-name-at-point-functions #'image-dired-file-name-at-point nil t))
 
 (defvar image-dired-minor-mode-map
   (let ((map (make-sparse-keymap)))
@@ -1844,7 +1800,7 @@ image-dired-minor-mode-map
 (define-minor-mode image-dired-minor-mode
   "Setup easy-to-use keybindings for the commands to be used in Dired mode.
 Note that n, p and <down> and <up> will be hijacked and bound to
-`image-dired-dired-x-line'."
+`image-dired-dired-next-line' and `image-dired-dired-previous-line'."
   :keymap image-dired-minor-mode-map)
 
 (declare-function clear-image-cache "image.c" (&optional filter))
@@ -2007,11 +1963,6 @@ image-dired-window-width-pixels
   "Calculate WINDOW width in pixels."
     (* (window-width window) (frame-char-width)))
 
-(defun image-dired-window-height-pixels (window)
-  "Calculate WINDOW height in pixels."
-  ;; Note: The mode-line consumes one line
-    (* (- (window-height window) 1) (frame-char-height)))
-
 (defun image-dired-display-window ()
   "Return window where `image-dired-display-image-buffer' is visible."
   (get-window-with-predicate
@@ -2037,59 +1988,22 @@ image-dired-associated-dired-buffer-window
              (equal (window-buffer window) buf))))
       (error "No thumbnail image at point"))))
 
-(defun image-dired-display-window-width (window)
-  "Return width, in pixels, of WINDOW."
-  (- (image-dired-window-width-pixels window)
-     image-dired-display-window-width-correction))
-
-(defun image-dired-display-window-height (window)
-  "Return height, in pixels, of WINDOW."
-  (- (image-dired-window-height-pixels window)
-     image-dired-display-window-height-correction))
-
-(defun image-dired-display-image (file &optional original-size)
+(defun image-dired-display-image (file &optional _ignored)
   "Display image FILE in image buffer.
-Use this when you want to display the image, semi sized, in a new
-window.  The image is sized to fit the display window (using a
-temporary file, don't worry).  Because of this, it will not be as
-quick as opening it directly, but on most modern systems it
-should feel snappy enough.
-
-If optional argument ORIGINAL-SIZE is non-nil, display image in its
-original size."
-  (image-dired--check-executable-exists
-   'image-dired-cmd-create-temp-image-program)
-  (let ((new-file (expand-file-name image-dired-temp-image-file))
-        (window (image-dired-display-window))
-        (image-type 'jpeg))
-    (setq file (expand-file-name file))
-    (if (not original-size)
-        (let* ((spec
-                (list
-                 (cons ?p image-dired-cmd-create-temp-image-program)
-                 (cons ?w (image-dired-display-window-width window))
-                 (cons ?h (image-dired-display-window-height window))
-                 (cons ?f file)
-                 (cons ?t new-file)))
-               (ret
-                (apply #'call-process
-                       image-dired-cmd-create-temp-image-program nil nil nil
-                       (mapcar
-                        (lambda (arg) (format-spec arg spec))
-                        image-dired-cmd-create-temp-image-options))))
-          (when (not (zerop ret))
-            (error "Could not resize image")))
-      (setq image-type (image-type-from-file-name file))
-      (copy-file file new-file t))
-    (with-current-buffer (image-dired-create-display-image-buffer)
-      (let ((inhibit-read-only t))
-        (erase-buffer)
-        (clear-image-cache)
-        (image-dired-insert-image image-dired-temp-image-file image-type 0 0)
-        (goto-char (point-min))
-        (set-window-vscroll window 0)
-        (set-window-hscroll window 0)
-        (image-dired-update-property 'original-file-name file)))))
+Use this when you want to display the image, in a new window.
+The window will use `image-dired-display-image-mode' which is
+based on `image-mode'."
+  (declare (advertised-calling-convention (file) "29.1"))
+  (setq file (expand-file-name file))
+  (let ((buf (get-buffer image-dired-display-image-buffer))
+        (cur-win (selected-window)))
+    (when buf
+      (kill-buffer buf))
+    (when-let ((buf (find-file-other-window file)))
+      (display-buffer buf)
+      (rename-buffer image-dired-display-image-buffer)
+      (image-dired-display-image-mode)
+      (select-window cur-win))))
 
 (defun image-dired-display-thumbnail-original-image (&optional arg)
   "Display current thumbnail's original image in display buffer.
@@ -2103,8 +2017,6 @@ image-dired-display-thumbnail-original-image
           (message "No thumbnail at point")
         (if (not file)
             (message "No original file name found")
-	  (image-dired-create-display-image-buffer)
-          (display-buffer image-dired-display-image-buffer)
           (image-dired-display-image file arg))))))
 
 
@@ -2114,8 +2026,6 @@ image-dired-dired-display-image
 See documentation for `image-dired-display-image' for more information.
 With prefix argument ARG, display image in its original size."
   (interactive "P")
-  (image-dired-create-display-image-buffer)
-  (display-buffer image-dired-display-image-buffer)
   (image-dired-display-image (dired-get-filename) arg))
 
 (defun image-dired-image-at-point-p ()
@@ -2432,8 +2342,6 @@ image-dired-mouse-display-image
     (when file
       (if image-dired-track-movement
 	  (image-dired-track-original-file))
-      (image-dired-create-display-image-buffer)
-      (display-buffer image-dired-display-image-buffer)
       (image-dired-display-image file))))
 
 (defun image-dired-mouse-select-thumbnail (event)
@@ -2870,6 +2778,65 @@ 'tumme
 (define-obsolete-function-alias 'image-dired-setup-dired-keybindings
   #'image-dired-minor-mode "26.1")
 
+(defcustom image-dired-cmd-create-temp-image-program
+  (if (executable-find "gm") "gm" "convert")
+  "Executable used to create temporary image.
+Used together with `image-dired-cmd-create-temp-image-options'."
+  :type 'file
+  :version "29.1")
+(make-obsolete-variable 'image-dired-cmd-create-temp-image-program
+                        "no longer used." "29.1")
+
+(defcustom image-dired-cmd-create-temp-image-options
+  (let ((opts '("-size" "%wx%h" "%f[0]"
+                "-resize" "%wx%h>"
+                "-strip" "jpeg:%t")))
+    (if (executable-find "gm") (cons "convert" opts) opts))
+  "Options of command used to create temporary image for display window.
+Used together with `image-dired-cmd-create-temp-image-program',
+Available format specifiers are: %w and %h which are replaced by
+the calculated max size for width and height in the image display window,
+%f which is replaced by the file name of the original image and %t which
+is replaced by the file name of the temporary file."
+  :version "29.1"
+  :type '(repeat (string :tag "Argument")))
+(make-obsolete-variable 'image-dired-cmd-create-temp-image-options
+                        "no longer used." "29.1")
+
+(defcustom image-dired-display-window-width-correction 1
+  "Number to be used to correct image display window width.
+Change if the default (1) does not work (i.e. if the image does not
+completely fit)."
+  :type 'integer)
+(make-obsolete-variable 'image-dired-display-window-width-correction
+                        "no longer used." "29.1")
+
+(defcustom image-dired-display-window-height-correction 0
+  "Number to be used to correct image display window height.
+Change if the default (0) does not work (i.e. if the image does not
+completely fit)."
+  :type 'integer)
+(make-obsolete-variable 'image-dired-display-window-height-correction
+                        "no longer used." "29.1")
+
+(defun image-dired-display-window-width (window)
+  "Return width, in pixels, of WINDOW."
+  (declare (obsolete nil "29.1"))
+  (- (image-dired-window-width-pixels window)
+     image-dired-display-window-width-correction))
+
+(defun image-dired-display-window-height (window)
+  "Return height, in pixels, of WINDOW."
+  (declare (obsolete nil "29.1"))
+  (- (image-dired-window-height-pixels window)
+     image-dired-display-window-height-correction))
+
+(defun image-dired-window-height-pixels (window)
+  "Calculate WINDOW height in pixels."
+  (declare (obsolete nil "29.1"))
+  ;; Note: The mode-line consumes one line
+    (* (- (window-height window) 1) (frame-char-height)))
+
 (defcustom image-dired-cmd-read-exif-data-program "exiftool"
   "Program used to read EXIF data to image.
 Used together with `image-dired-cmd-read-exif-data-options'."
@@ -2912,6 +2879,9 @@ image-dired-get-exif-data
         (setq tag-value (buffer-substring (point-min) (point-max)))))
     tag-value))
 
+(define-obsolete-function-alias 'image-dired-create-display-image-buffer
+  #'ignore "29.1")
+
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;;;;;;;; TEST-SECTION ;;;;;;;;;;;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-- 
2.30.2

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Mathias Dahl <mathias.dahl@gmail.com> writes:
>>
>>> It seems I knew about `image-mode' but perhaps it never crossed my mind
>>> to inherit from it. 16 years later, my memory fails me...
>>>
>>> At any rate I have nothing against it, if someone wants to give it a try.
>
> See the attached patch.  Unsurprisingly, this makes `image-dired' feel
> quite a bit snappier, as we avoid a lot of unnecessary work.

No comments within a week, so I've now pushed to master as commit
73a6c8f98f with some further tweaks and a NEWS entry.

Please report here or open a new bug if you run into any issues.

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

>> Glenn Morris <rgm@gnu.org> writes:
>>
>>> thumbs.el and image-dired.el both rely on using external ImageMagick
>>> utilities to resize/rotate images.
>>>
>>> If Emacs was compiled with ImageMagick support, then images of
>>> type imagemagick can be resized and rotated internally (see
>>> image-transform-*). These modes should use the internal support if
>>> it exists.
>>
>> So now that we have native image rotation and ImageMagick is on its
>> way to become deprecated, I assume that this bug should be taken to
>> mean that we want to use the built-in support here instead.

I fixed this in `image-dired' (see separate patch).

I guess we could rebase thumbs.el to also use the built-in image viewing
support, but I took a look and this external conversion seems built into
the foundation of how that library works.  So this looks to me like it
would amount to a rewrite of thumbs.el.  I could be wrong of course, but
that was my conclusion after poking at it for an hour or so.

Furthermore, at this point, image-dired.el is by far the more powerful
package, and it seems better implemented overall.  Although it borrows
heavily from thumbs.el (or at least it used to), thumbs.el seems fully
redundant these days.  However, it has some features that image-dired.el
does not, especially when it comes to modifying images.

So rather than attempting a rewrite of thumbs.el, the better solution
seems to me to add any important missing features from thumbs.el to
image-dired.el, and then obsoleting the former (not necessarily in that
order, but either way is fine to me).

bug#10758: thumbs and image-dired to use built-in ImageMagick

[Stefan Kangas]

close 10758 29.1
thanks

Stefan Kangas <stefan@marxist.se> writes:

> I fixed this in `image-dired' (see separate patch).
>
> I guess we could rebase thumbs.el to also use the built-in image viewing
> support, but I took a look and this external conversion seems built into
> the foundation of how that library works.  So this looks to me like it
> would amount to a rewrite of thumbs.el.  I could be wrong of course, but
> that was my conclusion after poking at it for an hour or so.
>
> Furthermore, at this point, image-dired.el is by far the more powerful
> package, and it seems better implemented overall.  Although it borrows
> heavily from thumbs.el (or at least it used to), thumbs.el seems fully
> redundant these days.  However, it has some features that image-dired.el
> does not, especially when it comes to modifying images.
>
> So rather than attempting a rewrite of thumbs.el, the better solution
> seems to me to add any important missing features from thumbs.el to
> image-dired.el, and then obsoleting the former (not necessarily in that
> order, but either way is fine to me).

No further comments within 6 weeks, so I think we can say that this bug
is resolved for now, and we can look into making thumbs.el obsolete
later.

I'm therefore closing this bug as fixed for image-dired.el.  If that
conclusion is incorrect, please re-open.