alphapapa / ement.el

A Matrix client for GNU Emacs
GNU General Public License v3.0
488 stars 44 forks source link

User-specified percentage image scaling #251

Open phil-s opened 8 months ago

phil-s commented 8 months ago

I've just tried this and it seems to work.

When using M-RET or <mouse-1> on an image you can either supply a numeric prefix arg as a scale percentage, or with C-u it will prompt you for the percentage.

The last-prefix-arg bit for the mouse-driven command was a hunch that seems to be correct, but I've not gone digging to find out exactly why that happens (my guess is that it's because there are two mouse events when you click-and-release, with <mouse-1> following from <down-mouse-1>).

modified   ement-room.el
@@ -4935,14 +4935,19 @@ ement-room-image-scale-mouse
   (let* ((pos (event-start event))
          (window (posn-window pos)))
     (with-selected-window window
-      (ement-room-image-scale (posn-point pos)))))
+      ;; Any prefix arg has been demoted to `last-prefix-arg' by this point.
+      (ement-room-image-scale (posn-point pos) last-prefix-arg))))

-(defun ement-room-image-scale (pos)
+(defun ement-room-image-scale (pos &optional scale)
   "Toggle scale of image at POS.
 Scale image to fit within the window's body.  If image is already
 fit to the window, reduce its max-height to 10% of the window's
-height."
-  (interactive "d")
+height.
+
+With a prefix argument, scale the image to the specified
+percentage of its actual size (if numeric), or prompt for a
+percentage (if not numeric)."
+  (interactive "d\nP")
   (pcase-let* ((image (get-text-property pos 'display))
                (window-width (window-body-width nil t))
                (window-height (window-body-height nil t))
@@ -4951,14 +4956,24 @@ ement-room-image-scale
                (new-height (if (= window-height (or (image-property image :max-height) -1))
                                (/ window-height 10)
                              window-height)))
-    (when (fboundp 'imagemagick-types)
-      ;; Only do this when ImageMagick is supported.
-      ;; FIXME: When requiring Emacs 27+, remove this (I guess?).
-      (setf (image-property image :type) 'imagemagick))
-    ;; Set :scale to nil since image scaling commands might have changed it.
-    (setf (image-property image :scale) nil
-          (image-property image :max-width) window-width
-          (image-property image :max-height) new-height)))
+    (if scale
+        (let ((scale (if (consp scale)
+                         (read-number "Scale (% of original): " 100
+                                      'read-number-history)
+                       scale)))
+          (unless (cl-plusp scale)
+            (error "Not a positive number: %s" scale))
+          (setf (image-property image :scale) (/ scale 100.0)
+                (image-property image :max-width) nil
+                (image-property image :max-height) nil))
+      (when (fboundp 'imagemagick-types)
+        ;; Only do this when ImageMagick is supported.
+        ;; FIXME: When requiring Emacs 27+, remove this (I guess?).
+        (setf (image-property image :type) 'imagemagick))
+      ;; Set :scale to nil since image scaling commands might have changed it.
+      (setf (image-property image :scale) nil
+            (image-property image :max-width) window-width
+            (image-property image :max-height) new-height))))

(Edit: In hindsight, the 'imagemagick' test should be evaluated outside of the if form, as it would be relevant in both cases.)

phil-s commented 8 months ago

For ease of testing by others, that diff gives me:

(defun ement-room-image-scale-mouse (event)
  "Toggle scale of image at mouse EVENT.
Scale image to fit within the window's body.  If image is already
fit to the window, reduce its max-height to 10% of the window's
height."
  (interactive "e")
  (let* ((pos (event-start event))
         (window (posn-window pos)))
    (with-selected-window window
      ;; Any prefix arg has been demoted to `last-prefix-arg' by this point.
      (ement-room-image-scale (posn-point pos) last-prefix-arg))))

(defun ement-room-image-scale (pos &optional scale)
  "Toggle scale of image at POS.
Scale image to fit within the window's body.  If image is already
fit to the window, reduce its max-height to 10% of the window's
height.

With a prefix argument, scale the image to the specified
percentage of its actual size (if numeric), or prompt for a
percentage (if not numeric)."
  (interactive "d\nP")
  (pcase-let* ((image (get-text-property pos 'display))
               (window-width (window-body-width nil t))
               (window-height (window-body-height nil t))
               ;; Image scaling commands set :max-height and friends to nil so use the
               ;; impossible dummy value -1.  See <https://github.com/alphapapa/ement.el/issues/39>.
               (new-height (if (= window-height (or (image-property image :max-height) -1))
                               (/ window-height 10)
                             window-height)))
    (if scale
        (let ((scale (if (consp scale)
                         (read-number "Scale (% of original): " 100
                                      'read-number-history)
                       scale)))
          (unless (cl-plusp scale)
            (error "Not a positive number: %s" scale))
          (setf (image-property image :scale) (/ scale 100.0)
                (image-property image :max-width) nil
                (image-property image :max-height) nil))
      (when (fboundp 'imagemagick-types)
        ;; Only do this when ImageMagick is supported.
        ;; FIXME: When requiring Emacs 27+, remove this (I guess?).
        (setf (image-property image :type) 'imagemagick))
      ;; Set :scale to nil since image scaling commands might have changed it.
      (setf (image-property image :scale) nil
            (image-property image :max-width) window-width
            (image-property image :max-height) new-height))))
alphapapa commented 8 months ago

Well, I'm not necessarily opposed to this, but I'm not sure how useful it would be to have to manually specify a scale, guessing at which number would provide a useful result (i.e. one that fits within the window while still being large enough to see details, or something like that).

Would it be better to provide commands to increase/decrease the scale by certain amounts, either a constant like 10% or some kind of computed ratio between the window size and the image size?

phil-s commented 8 months ago

My first motivation was the default input of 100% -- to be able to show the image at its real size, even if larger than the window. My second motivation had been to work around the scrolling bug when scaling to the window height, but I've figured out how to fix that. I still think the 100% option is useful (and better than scaling up to that in steps, which can be very slow).

I've noted that we have access to image-map as a parent (which gives us stepped scaling), but that image-mode-map has a bunch of other transformations that seem like they would be quite nice to inherit as well -- but we don't want to set that as a parent map because it has other bindings which are very specific to the major mode.

I think a quick "jump to 100%" is a useful thing, and being able to set an arbitrary percentage was just a consequence of providing that (probably less useful, but it nevertheless seemed like an obvious enhancement).

I'll have a ponder as I work on the other image changes.