Re: [PATCH v2] Add native image scaling (bug#33587)

[Eli Zaretskii]

> Date: Wed, 2 Jan 2019 21:12:41 +0000
> From: Alan Third <address@hidden>
> Cc: address@hidden
> 
> I think this is the final version.

Thanks.  A few minor gotchas.

> I would appreciate if someone who knows their way around image
> handling would be able to test it. I’m particularly concerned that
> I’ve probably broken masks. I can’t find any examples of how to use
> them online, and they don’t work at all in NS, so I don’t know if they
> get any use.

Seconded.

> address@hidden :max-width @var{max-width}, :max-height @var{max-height}
> +The @code{:max-width} and @code{:max-height} keywords are used for
> +scaling if the size of the image of the image exceeds these values.
                       ^^^^^^^^^^^^^^^^^^^^^^^^^
Redundant repetition.

> +If @code{:width} is set it will have precedence over @code{max-width},
                          ^
A comma is missing there.

> +and if @code{:height} is set it will have precedence over
                               ^
Likewise here.

> +       @code{:max-width} and @code{:max-height} will always preserve
> +the aspect ratio.

I don't think I understand what that means in practice.  Does this
allude to the issue described below with using :max-width and
:max-height in preference to :with and :height?  If so, I suggest to
describe that only once.

> +If both @code{:width} and @code{:max-height} has been set (but
                                                ^^^
"have"

> address@hidden:height} has not been set), then @code{:max-height} will have
> +precedence.  The same is the case for the opposite combination: The
> +``max'' keyword has precedence.

This confused me until I've read the example.  having read the
example, I suggest to describe this differently:

  If both @code{:max-width} and @code{:height} are specified, but
  @code{:width} is not, preserving the aspect ratio might require that
  width exceeds @code{:max-width}.  If this happens, scaling will use a
  smaller value for the height so as to preserve the aspect ratio while
  not exceeding @code{:max-width}.  Similarly when both
  @code{:max-height} and @code{:width} are specified, but @code{:height}
  is not.

> address@hidden :scale @var{scale}
> +This should be a number, where values higher than 1 means to increase
> +the size, and lower means to decrease the size.  For instance, a value
> +of 0.25 will make the image a quarter size of what it originally was.

I think we should say here explicitly that scale multiplies both
height and width, because people might otherwise think the scaling
applies to the area of the image.

> address@hidden image-scaling-p &optional frame
> +This function returns @code{t} if @var{frame} supports image scaling.
> address@hidden @code{nil} or omitted means to use the selected frame
> +(@pxref{Input Focus}).
> +
> +If image scaling is not supported, @code{:width}, @code{:height},
> address@hidden:scale}, @code{:max-width} and @code{:max-height} will only be
> +usable through ImageMagick, if available (@pxref{ImageMagick Images}).
> address@hidden defun

Shouldn't image-scaling-p return non-nil if ImageMagick is available?
I think that would allow a simpler Lisp code.