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

[Po Lu <luangruo@yahoo.com>]

Eli Zaretskii <eliz@gnu.org> writes:

> I have a few comments and issues about this changeset:

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

Thanks.

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

Thanks.

>>                                       @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.

I tried to explain what a sequence is later on, and yes, I meant the
change in distance.

>>                                       @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".

Thanks.

> 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.

XInput doesn't yet have support for reporting rotation gestures; the
angle here is only reported as part of a pinch sequence, and pinch
sequence only started when actual pinching occurs.

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

Thanks.

>> +  (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?

Yes, I'll fix that.

> 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.

I didn't notice that, but it's because we round the result of the
scaling.  I will try to fix this, thanks.

However, I think TRT here is not for keyboard.c to "coalesce" these
events: the best approach would probably be to use `read-event' inside
individual user commands with a timeout.  That way, the commands
themselves get to decide the granularity of the gestures they receive.

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

Thanks, where would you prefer to put it?