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

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.

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

[Po Lu]

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?

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 18:03:31 +0800
> 
> > 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.

Yes, but you could generate several Emacs events in make_lispy_event,
can't you?  Or maybe simply ignore the angle data?

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

I don't think I understand how read-event with a timeout will help
here: you still get a series of events, AFAIU.

Besides, I think doing such processing in Lisp is not the best way,
because the facilities are quite crude and not meant for doing this.
Walking the event queue looking a for a series of pinch events is much
easier.

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

In "Text Scale", I guess (which currently doesn't document
mouse-wheel-text-scale, which it should).

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, but you could generate several Emacs events in make_lispy_event,
> can't you?  Or maybe simply ignore the angle data?

I meant to say that those angle deltas only make sense as part of a
pinch event, and they're not supposed to be treated as separate rotation
events.  (Which we will want to expose to Lisp separately, once the
XInput developers add support for them.)

> I don't think I understand how read-event with a timeout will help
> here: you still get a series of events, AFAIU.

Yes, but it solves the flickering problem.

> Walking the event queue looking a for a series of pinch events is much
> easier.

Yes, but limiting the frequency of these events is something not all
commands might want.

> In "Text Scale", I guess (which currently doesn't document
> mouse-wheel-text-scale, which it should).

Thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 18:39:09 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, but you could generate several Emacs events in make_lispy_event,
> > can't you?  Or maybe simply ignore the angle data?
> 
> I meant to say that those angle deltas only make sense as part of a
> pinch event, and they're not supposed to be treated as separate rotation
> events.  (Which we will want to expose to Lisp separately, once the
> XInput developers add support for them.)

Please explain how the angle deltas make sense as part of a pinch
gesture, I don't think I understand that.

> > I don't think I understand how read-event with a timeout will help
> > here: you still get a series of events, AFAIU.
> 
> Yes, but it solves the flickering problem.

That's not the only possible solution, and neither is it the cleanest
one, IMO.

> > Walking the event queue looking a for a series of pinch events is much
> > easier.
> 
> Yes, but limiting the frequency of these events is something not all
> commands might want.

We are talking about the situation that several pinch events were
reported to Emacs since the last time it entered the idle loop and
checked for input.  Which commands do you envision that would like to
process only some of that gesture, and leave the rest in the input
queue?  AFAIU, all it will do is cause some kind of "inertia", in that
the reaction to user gestures will lag after the gestures.  Why would
that make sense?

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Please explain how the angle deltas make sense as part of a pinch
> gesture, I don't think I understand that.

Basically, this delta is only reported by the X server when the finger
rotates _after_ a pinch gesture is detected, for example if the user
zooms in, and while doing so, also rotates his fingers.  The rationale
is that some applications might have a reason to handle these events
separately (such as to ignore them, or to do something else with them),
instead of simply rotating the display.

> We are talking about the situation that several pinch events were
> reported to Emacs since the last time it entered the idle loop and
> checked for input.  Which commands do you envision that would like to
> process only some of that gesture, and leave the rest in the input
> queue?  AFAIU, all it will do is cause some kind of "inertia", in that
> the reaction to user gestures will lag after the gestures.  Why would
> that make sense?

I see your point now.  I'll look into making keyboard.c merge multiple
pinch events in the event queue.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 19:09:01 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please explain how the angle deltas make sense as part of a pinch
> > gesture, I don't think I understand that.
> 
> Basically, this delta is only reported by the X server when the finger
> rotates _after_ a pinch gesture is detected, for example if the user
> zooms in, and while doing so, also rotates his fingers.  The rationale
> is that some applications might have a reason to handle these events
> separately (such as to ignore them, or to do something else with them),
> instead of simply rotating the display.

Then the current text should be amended, because it entirely misses
this aspect.

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Then the current text should be amended, because it entirely misses
> this aspect.

Thanks, I'll try to fix that.

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

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> I see your point now.  I'll look into making keyboard.c merge multiple
> pinch events in the event queue.

Does this change in keyboard.c look okay to you?  Thanks.

@@ -4037,6 +4037,42 @@ kbd_buffer_get_event (KBOARD **kbp,
 	     and build a real event from the queue entry.  */
 	  if (NILP (obj))
 	    {
+	      /* Pinch events are often sent in rapid succession, so
+		 large amounts of such events have the potential to
+		 queue up inside the keyboard buffer.  In that case,
+		 find the last pinch event in succession on the same
+		 frame with the same modifiers, and send that instead.  */
+
+	      if (event->ie.kind == PINCH_EVENT
+		  /* Ignore if this is the start of a pinch sequence.
+		     These events should always be sent so that we
+		     never miss a sequence starting, and they don't
+		     have the potential to queue up.  */
+		  && (XFLOAT_DATA (XCAR (event->ie.arg)) != 0.0
+		      || XFLOAT_DATA (XCAR (XCDR (event->ie.arg))) != 0.0
+		      || XFLOAT_DATA (XCAR (XCDR (XCDR (event->ie.arg)))) != 1.0))
+		{
+		  union buffered_input_event *maybe_event = next_kbd_event (event);
+		  while (maybe_event != kbd_store_ptr
+			 && maybe_event->ie.kind == PINCH_EVENT
+			 /* Make sure we never miss an event that has
+			    different modifiers.  */
+			 && maybe_event->ie.modifiers == event->ie.modifiers
+			 /* Make sure that the event is for the same
+			    frame.  */
+			 && EQ (maybe_event->ie.frame_or_window,
+				event->ie.frame_or_window)
+			 /* Make sure that the event isn't the start
+			    of a new pinch gesture sequence.  */
+			 && (XFLOAT_DATA (XCAR (maybe_event->ie.arg)) != 0.0
+			     || XFLOAT_DATA (XCAR (XCDR (maybe_event->ie.arg))) != 0.0
+			     || XFLOAT_DATA (XCAR (XCDR (XCDR (maybe_event->ie.arg)))) != 1.0))
+		    {
+		      event = maybe_event;
+		      maybe_event = next_kbd_event (event);
+		    }
+		}
+
 	      obj = make_lispy_event (&event->ie);

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 19:44:22 +0800
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > I see your point now.  I'll look into making keyboard.c merge multiple
> > pinch events in the event queue.
> 
> Does this change in keyboard.c look okay to you?  Thanks.

Yes.  But I think you should accumulate the DX, DY, SCALE, and ANGLE
values, so that the event you deliver expresses the entire change?

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes.  But I think you should accumulate the DX, DY, SCALE, and ANGLE
> values, so that the event you deliver expresses the entire change?

ANGLE, DX and DY should be accumulated, yes.  However, I don't think
SCALE should, as it's an absolute value and not a relative one.

Thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 19:53:34 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes.  But I think you should accumulate the DX, DY, SCALE, and ANGLE
> > values, so that the event you deliver expresses the entire change?
> 
> ANGLE, DX and DY should be accumulated, yes.  However, I don't think
> SCALE should, as it's an absolute value and not a relative one.

Right.

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org
>> Date: Sun, 26 Dec 2021 19:53:34 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Yes.  But I think you should accumulate the DX, DY, SCALE, and ANGLE
>> > values, so that the event you deliver expresses the entire change?
>> 
>> ANGLE, DX and DY should be accumulated, yes.  However, I don't think
>> SCALE should, as it's an absolute value and not a relative one.
>
> Right.

Thanks, now done.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 18:03:31 +0800
> 
> >>                                       @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.

I tried to fix this in a followup change.  But now I wonder if I
understood the semantics correctly:

> + All arguments after @var{position} are floating point numbers.

Does this mean DX and DY are also floats?  If so, are they in pixels
or are they some kind of relative change?  If they are relative, then
relative to what?  And why does text-scale-pinch require that both DX
and DY be zero?

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> I tried to explain what a sequence is later on, and yes, I meant the
>> change in distance.
>
> I tried to fix this in a followup change.  But now I wonder if I
> understood the semantics correctly:
>
>> + All arguments after @var{position} are floating point numbers.
>
> Does this mean DX and DY are also floats?  If so, are they in pixels
> or are they some kind of relative change?  If they are relative, then
> relative to what?  And why does text-scale-pinch require that both DX
> and DY be zero?

When DX, DY and ANGLE are all zero, then the event is the start of a
pinch gesture sequence; text-scale-pinch uses it to set the "base"
scale, which is then modified by events further in the sequence.

DX and DY are represented in an imaginary unit where 1.0 is the
horizontal width and vertical height of the touchpad (or more precisely,
depedent touch device, which might include Wacom tablets if their
drivers are configured in a special way) respectively.

Thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 18:51:21 +0800
> 
> DX and DY are represented in an imaginary unit where 1.0 is the
> horizontal width and vertical height of the touchpad (or more precisely,
> depedent touch device, which might include Wacom tablets if their
> drivers are configured in a special way) respectively.

This should be documented.  But I wonder how these two values are
useful, if their units are not pixels and not anything that can be
converted to pixels?

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org
>> Date: Sun, 26 Dec 2021 18:51:21 +0800
>> 
>> DX and DY are represented in an imaginary unit where 1.0 is the
>> horizontal width and vertical height of the touchpad (or more precisely,
>> depedent touch device, which might include Wacom tablets if their
>> drivers are configured in a special way) respectively.

> This should be documented.

Yes, I'll do that.

> But I wonder how these two values are useful, if their units are not
> pixels and not anything that can be converted to pixels?

You are supposed to multiply it by the dimensions of whatever object is
onscreen: in image-mode, it could be the dimensions of the image, while
in a message buffer it could instead be the dimensions of the window's
text area.

Thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Sun, 26 Dec 2021 18:58:50 +0800
> 
> >> DX and DY are represented in an imaginary unit where 1.0 is the
> >> horizontal width and vertical height of the touchpad (or more precisely,
> >> depedent touch device, which might include Wacom tablets if their
> >> drivers are configured in a special way) respectively.
> 
> > This should be documented.
> 
> Yes, I'll do that.
> 
> > But I wonder how these two values are useful, if their units are not
> > pixels and not anything that can be converted to pixels?
> 
> You are supposed to multiply it by the dimensions of whatever object is
> onscreen: in image-mode, it could be the dimensions of the image, while
> in a message buffer it could instead be the dimensions of the window's
> text area.

But you said above that the value 1.0 is the size of the touchpad, not
of the image or text window?  So now I'm confused.

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org
>> Date: Sun, 26 Dec 2021 18:58:50 +0800
>> 
>> >> DX and DY are represented in an imaginary unit where 1.0 is the
>> >> horizontal width and vertical height of the touchpad (or more precisely,
>> >> depedent touch device, which might include Wacom tablets if their
>> >> drivers are configured in a special way) respectively.
>> 
>> > This should be documented.
>> 
>> Yes, I'll do that.
>> 
>> > But I wonder how these two values are useful, if their units are not
>> > pixels and not anything that can be converted to pixels?
>> 
>> You are supposed to multiply it by the dimensions of whatever object is
>> onscreen: in image-mode, it could be the dimensions of the image, while
>> in a message buffer it could instead be the dimensions of the window's
>> text area.
>
> But you said above that the value 1.0 is the size of the touchpad, not
> of the image or text window?  So now I'm confused.

Yes.  The idea is that the size of the touchpad corresponds (in the mind
of the user) to the size of whatever is displayed onscreen underneath
the mouse pointer.