Created attachment 318424 [details] [review]
Patch for the gestures demo to keep the angle and zoom factor

This is a patch for the gestures demo to keep the zoom and rotate state after the gesture is finished. The original demo shows the zoom and rotate only while the gesture is happening. 

This behavior produces strange behavior with the quartz gesture patch which is based on NSEventTypeMagnify and NSEventTypeRotate events. These events from MacOS do not allow to identify if we are in the middle of a gesture or if the gesture is finished.

Review of attachment 318423 [details] [review]:

Looks generally good to me, and I assume you tested it.

Yes, it's not nice that you have to create three synthetic events in order to catch the touchpad gesture event - but gestures are at the GTK level, and GDK has no knowledge of them, except in terms of events.

Review of attachment 318424 [details] [review]:

I'm not overly fond of this, as it introduces a discrepancy between the libinput way and the OSX way - i.e. existing apps will need to handle gestures differently, now.

Hi Emmanuele,

thanks for the fast reviews! I tested this on MacOS El Capitan 10.11.2 with gtk+ from git in branch gtk-3-18. The other libraries are from macports. 

The difference between the libinput way and the quartz way is that MacOS does not track the gesture. However, if you zoom/rotate and keep the resulting zoom factor as in a web browser for example, then there is no difference. 

The difference is visible in the gestures demo. If you only want to temporarily zoom while your are in the middle of the gesture as it is done in the demo, then this will just more or less not rotate/zoom on MacOS. If you keep the result of the zoom, then there is no difference.

So the patch would improve in the situation of keeping the zoom/rotation as you probably would do it in a web browser or picture viewer for example.

It would just not zoom/rotate in situations where you expect that a substantial angle or zoom factor is produced in the update phase of the gesture tracking because the produced angle/zoom factor in the update phase is just small.

Thanks!

Friedrich

Hi Emmanuele,

I do not have a system where I can test the behavior with libinput. But I think you are right, that my changes to the gesture demo are not correct. 

During the update phase the lib input would report 1.01, 1.05, 1.1, 1.15 with 1.05 and 1.1 as intermediate values during update phase. Multiplying this all together is different from having just the final 1.15. 

The quartz version would report 1.035, 1.035, 1.035, 1.035 which multiplied together would result in 1.15. 

Therefore the demo change is not o.k. 

Friedrich

Created attachment 318437 [details] [review]
Updated patch for the gesture demo - keep zoom/rotate state after gesture

This is an updated patch for the gestures demo which keeps the scale and angle after the gesture has ended. I do not have a libinput system here where I can test that this really works with libinput.

Created attachment 321834 [details] [review]
Patch for gdkevents-quartz.c - zoom/rotate gestures - compliant to libinput

This patch adds NSEventTypeMagnify and NSEventTypeRotate support for quartz. The event sequences are translated to a sequence of GDK_TOUCHPAD_PINCH events. These gdk events are interpreted in the upper gtk layers to Zoom and Rotate gestures.

I tested the patch on gtk-3-18 in the macports environment on MacOS 10.11.3 (El Capitan) with the gtk3-demo (gestures) application. The patch is versus the gtk-3-18 branch.

The difference to the previous patch proposal 318423 (now obsolete) is that the full sequence of GDK_TOUCHPAD_GESTURE_PHASE_(Begin/Update/.../Update/End) is produced. So this is compliant to the behavior of the event sequence that libinput produces. Patch 318437 is therefore obsolete, because no changes to the demo are required.

I used the NSEvent.phase information to identify the start/update/end phases of the sequences for this.

Review of attachment 321834 [details] [review]:

I like this much more, thanks!

::: gdk/quartz/gdkevents-quartz.c
@@ +909,3 @@
+{
+  static gdouble scale = 1.0;
+   PINCH(STARTED) event.

Coding style:

 * use `double`, not `gdouble`
 * `scale` and `state` should probably be renamed `last_scale` and `last_state`, so we know what they represent
 * you should break down the enumeration declaration, something like:

```
    static enum {
      FP_STATE_IDLE,
      FP_STATE_UPDATE
    } last_state = FP_STATE_IDLE;
```

@@ +977,3 @@
+    {
+    case NSEventTypeMagnify:
+          state = FP_STATE_UPDATE;

Coding style: too much whitespace between assignment and [nsevent ...]

@@ +1621,3 @@
       }
       break;
+    case NSEventTypeMagnify:

Should we have a runtime/compile time check?

Looking at the API reference for NSEvent, I see that NSEventTypeMagnify and NSEventTypeRotate were introduced after 10.5, and we definitely require something newer than that, but I just wanted to make sure — and eventually leave a comment to that effect.

Created attachment 321852 [details] [review]
Patch for gdkevents-quartz.c - zoom/rotate gestures - including changes due to review comments

This patch includes the changes according to review comments from Comment #8. It obsoletes attachment 321834 [details] [review] as it is again a full patch.

The changes are:
* Changed gdouble to double.
* Renamed scale and state to last_scale and last_state
* Split the enumeration declaration to several lines
* Removed whitespace
* Added Compile time and Runtime check for OSX >= 10.7 as the [NSEvent phase] method was introduced in OSX 10.7 (NSEventTypeMagnify and NSEventTypeRotate exist since 10.5).

Thanks for the review!

Review of attachment 321852 [details] [review]:

Looks great, thanks!

Created attachment 321859 [details] [review]
Patch for gdkevents-quartz.c - zoom/rotate gestures - removed a newline

This patch just removes an accidental newline compared to the patch in attachment 321852 [details] [review]. It is again the full patch.

Review of attachment 321859 [details] [review]:

ok then

Thanks everybody for the reviews and the hints. Thanks Matthias for committing the patch to the git database. It ended here:

https://git.gnome.org/browse/gtk+/commit/?id=887905288763da0378162dc1e67560320dd7709d

Cheers

Friedrich

This fails to build on 10.7 because NSEventPhaseMayBegin has apparently only been introduced in 10.8 (the official documentation seems to be wrong).

Here is what /System/Library/Frameworks/AppKit.framework/Headers/NSEvent.h contains on 10.7:

#if MAC_OS_X_VERSION_10_7 <= MAC_OS_X_VERSION_MAX_ALLOWED
enum {
    NSEventPhaseNone        = 0, // event not associated with a phase.
    NSEventPhaseBegan       = 0x1 << 0,
    NSEventPhaseStationary  = 0x1 << 1,
    NSEventPhaseChanged     = 0x1 << 2,
    NSEventPhaseEnded       = 0x1 << 3,
    NSEventPhaseCancelled   = 0x1 << 4,
};
#endif


I wasn't sure if I should open a new ticket for that or not. I'm attaching the patch here because it's strictly related (patching a patch :), but I can open a new ticket if needed.

After the patch is applied, GTK+ 3.20.2 builds on 10.7, but I would be grateful for some guidelines about how to actually test the functionality given that a check for NSEventPhaseMayBegin is missing. Is there any "hello-world example" or some existing (not too complex) app that supports these events?

See also https://trac.macports.org/ticket/51052.

Created attachment 325461 [details] [review]
A patch for gdkevents-quartz.c to a allow building on OS X 10.7

Hi,

I reopened the case due to the build problems on 10.7. The proposed patch looks good to me but I do not have a possibility to test this on 10.7. As mentioned on the macports error report you can test the behavior with

gtk3-demo --run gestures

If everything works, then you can zoom and rotate a square. 

Either we change the existing version check from 10.7 to 10.8 or we apply the proposed
patch which might work on 10.7 also. 

The current version simply does not build on 10.7

Friedrich

Review of attachment 325461 [details] [review]:

From the functional point this should work as the event NSEventPhaseMayBegin will only result in canceling the gesture phase recognition in the upper gtk3 layers. On 10.11 I did not see this events during my tests. I only added them for completeness. However, it would be good to test the actual behavior on 10.7 to see if the events are generated as expected.

Mojcas patch attachment 325461 [details] [review] compiles on 10.7 but crashes when gtk3-demo --run gestures is run. 

See: https://trac.macports.org/ticket/51052

So maybe we should restrict this zoom/magnitude change to 10.8 onwards. I will provide a patch which at least results in no build errors on 10.7. I will follow the plan that has been introduced in macports, i.e. disable everything for 10.7

For me the crash report on 10.7 means that the Magnify event cannot provide the phase property and crashes when this is attempted.

Friedrich

Created attachment 325520 [details] [review]
gdkevents-quartz.c patch to enable zoom/rotate only for 10.8 and following

This introduces the zoom/rotate function only from 10.8 and following. It is removed for 10.7.

I tested this on 10.11 El Capitan.

Review of attachment 325520 [details] [review]:

Sounds okay. Thanks for the detailed commit message, much appreciated.