Re: master 101bbd1392: Add support for pinch gestures to the XI2 build

[Eli Zaretskii]

> branch: master
> commit 101bbd1392077e26e904c70fead7f7d7dce595f7
> Author: Po Lu <luangruo@yahoo.com>
> Commit: Po Lu <luangruo@yahoo.com>

I have a few comments and issues about this changeset:

> +@item (pinch @var{position} @var{dx} @var{dy} @var{scale} @var{angle})
> +This kind of event is generated by the user performing a ``pinch''
> +gesture with two fingers on a touchpad.

This should explain what is a "pinch" gesture.  It isn't self-evident.
The same goes for the NEWS entries.

>                                           @var{position} is a mouse
> +position list (@pxref{Click Events}) detailing the position of the
> +mouse cursor when the event occured,
   ^^^^^^^^^^^^
"mouse pointer"

>                                       @var{dx} is the distance between
> +the horizontal positions of the fingers since the last event in the
> +same sequence, @var{dy} is the vertical movement of the fingers since
> +the last event in the same sequence,

What is the meaning of "the distance ... since last event"?  Did you
mean the _change_ in the distance since the last event?  And anyway,
what does "last event" mean in this context -- this is not explained
anywhere.

>                                       @var{scale} is the division of
> +the current distance between the fingers and the distance at the start

Not "the division of X and Y", but "the ratio of X to Y".

> +of the sequence, and @var{angle} is the delta in degrees between the
> +angles of the fingers in this event and the fingers in the last event
> +of the same sequence.
> +
> +All arguments after @var{position} are floating point numbers.
> +
> +This event is usually sent as part of a sequence, which begins with
> +the user placing two fingers on the touchpad and ends with the user
> +removing those fingers.  @var{dx}, @var{dy}, and @var{angle} will be
> +@code{0.0} in the first event sent after a sequence begins.

And I think this kind of event is too high-level.  Usually, pinch
(a.k.a. "zoom") gestures and rotation gestures are reported
separately, because they have different semantics and will probably
have different command bindings.  Why should be lump them together?
Moreover, in the corresponding command you require that the rotation
angle be zero, something that will be hard on the users, I think.

> +(defun text-scale-pinch (event)
> +  "Adjust the height of the default face by the scale in EVENT."

The doc string should say something about "pinch", and perhaps point
to the manual.

> +  (interactive "e")
> +  (let ((window (posn-window (nth 1 event)))
> +        (scale (nth 4 event))
> +        (dx (nth 2 event))
> +        (dy (nth 3 event))
> +        (angle (nth 5 event)))
> +    (with-selected-window window
> +      (when (and (zerop dx)
> +                 (zerop dy)
> +                 (zerop angle)
> +                 (equal scale 1.0))
> +        (setq text-scale--pinch-start-scale
> +              (if text-scale-mode text-scale-mode-amount 0)))
> +      (text-scale-set
> +       (+ text-scale--pinch-start-scale
> +          (round (log scale text-scale-mode-step)))))))

Shouldn't this command ensure it gets a pinch event?

And I'd prefer to have separate zoom and rotation events, with
corresponding separate commands.  That would be a more logical
division of gestures.

Also, does Emacs have any control on how frequently these events will
be sent as long as the user keeps the fingers on the touchpad?  We
could potentially be flooded with input messages, and the question is
whether the above command should process all the pending events of
this time at once?  If it should, perhaps keyboard.c should fetch all
the consecutive pinch events and produce a single event out of it, or
at least produce a structure that supplies all the events as one form
to the application level?  At least for text-scale, I think you will
have a lot of unpleasant flickering if you process each event
separately.

Finally, as this is a user command, we should start documenting such
commands in the user manual.

Thanks.