Implement DMGestureRecognizer to start/stop directmanipluation on scrollveiwer by yiyang-msft · Pull Request #1305 · microsoft/WinObjC

yiyang-msft · 2016-11-02T22:58:47Z

Implement DMGestureRecognizer to start/stop direct manipluation on XAML Scrollveiwer so that gestures attached to subviews within a UIScrollview can be properly triggered. and at the same time, direct manipulation can be triggered if no other gestured are recognized.

This change is

…llviewer so that other gestures attached to subviews inside UIScrollview can be properly triggered

DHowett-MSFT · 2016-11-03T19:02:58Z

        int count = (i == 0) ? gestureCount : dmGestureCount;
        for (int k = 0; k < count; k++) {
-            UIGestureRecognizer* curgesture = recognizers[i];
+            UIGestureRecognizer* curgesture = ( i==0 ) ? recognizers[k] : dmrecognizers[k];


Please format! #Resolved

DHowett-MSFT · 2016-11-03T19:04:26Z

    BOOL _isZooming;
    BOOL _isZoomingToRect;

+    // Dimanip gesture recognizer, used for detecting if Dimanip should start or not


Dimanip [](start = 57, length = 7)

nit: Shouldn't these be DManip, not Dimanip? #Resolved

DHowett-MSFT · 2016-11-03T19:05:05Z

 - (void)dealloc {
    _zoomView = nil;
+    _dmGesture = nil;



You do not need to set StrongId instances to nil. #Resolved

DHowett-MSFT · 2016-11-03T19:05:53Z

    return [_UIScrollViewCALayer class];
 }

+void _setManipulationMode(UIScrollView* self, WUXIManipulationModes mode, BOOL recursive) {


_setManipulationMode [](start = 5, length = 20)

Make this static?

It could also be a method.. #Resolved

jaredhms · 2016-11-03T19:06:59Z

+        while (toLayer.superlayer != nil) {
+            toLayer = toLayer.superlayer;
+        }
+    }


I thought you weren't including the changes to this file? #Pending

i can revert the change here and wait for your change to come in. but this is going to leave corresponding issue hanging around a little longer given ux/uxe will probably take longer to arrive develop. On the other hand, for the merge conflicts that is going to occur if I leave this change here, you can just reject mine and accept yours when merging since you already have equivalent? Let me know what you think. I can do either.

In reply to: 86420400 [](ancestors = 86420400)

jaredhms · 2016-11-03T19:08:07Z

+};
+
+@interface UIPanGestureRecognizer()
+    -(const std::vector<TouchInfo>&)getTouches;


getTouches [](start = 36, length = 10)

shouldn't this be _getTouches since it's private/internal? #Resolved

jaredhms · 2016-11-03T19:09:24Z

 static const bool DEBUG_DELEGATE = DEBUG_ALL || false;
 static const bool DEBUG_INSETS = DEBUG_ALL || false;
 static const bool DEBUG_ZOOM = DEBUG_ALL || false;
+static const bool DEBUG_DMGesture = DEBUG_ALL || false;


DEBUG_DMGesture [](start = 18, length = 15)

nit: nobody will know what 'dm' is when they come by later. Can we call this DEBUG_DMANIP_GESTURE? #Resolved

jaredhms · 2016-11-03T19:09:43Z

    BOOL _isZoomingToRect;

+    // Dimanip gesture recognizer, used for detecting if Dimanip should start or not
+    StrongId<_UIDMPanGestureRecognizer> _dmGesture;


_dmGesture [](start = 40, length = 10)

nit: _dManipGesture #Resolved

jaredhms · 2016-11-03T19:10:24Z

    _scrollViewer.minZoomFactor = 1.0f;
    _scrollViewer.zoomMode = WXCZoomModeDisabled;
+
+    // setting transparent background


setting transparent background [](start = 7, length = 30)

...'so we can receive touch input' #Resolved

jaredhms · 2016-11-03T20:28:02Z

+#import "Starboard.h"
+#import "UIGestureRecognizerInternal.h"
+#import "UIPanGestureRecognizerInternal.h"
+#import "_UIDMPanGestureRecognizer.h"


DM [](start = 12, length = 2)

Everywhere you use 'DM', can we use something more descriptive - maybe DManip? #Resolved

jaredhms · 2016-11-03T20:28:39Z

+}
+
+static void commonInit(UIPanGestureRecognizer* self) {
+    [self _setDragSlack:7.0f];


[self _setDragSlack:7.0f]; [](start = 1, length = 29)

why 7.0f? can you add a comment for this? #Resolved

jaredhms · 2016-11-03T20:30:11Z

    if (g_resetAllTrackingGestures) {
        g_resetAllTrackingGestures = FALSE;
-        //  Find gesture recognizers in the heirarchy, back-first
+        // Find gesture recognizers in the heirarchy, back-first


heirarchy [](start = 43, length = 9)

super-nit: hierarchy #Resolved

jaredhms · 2016-11-03T20:31:31Z

+    const static int MAXIMUM_DMGESTURE_ALLOW = 16;

+    UIGestureRecognizer* recognizers[MAXIMUM_GESTURE_ALLOW];
+    UIGestureRecognizer* dmrecognizers[MAXIMUM_DMGESTURE_ALLOW];


dm [](start = 25, length = 2)

more use of 'dm' - could definitely use more descriptive names across all of these files/file names/etc. #Resolved

jaredhms · 2016-11-03T20:33:17Z

+            if (dmGestureCount < MAXIMUM_DMGESTURE_ALLOW) {
+                dmrecognizers[dmGestureCount++] = curgesture;
+            } else {
+                TraceWarning(TAG, L"DMGestures exceed maxinum allowed, ignoring the rest");


maxinum [](start = 54, length = 7)

nit: maximum #Resolved

jaredhms · 2016-11-03T20:36:55Z

Do we already have an existing test for this in XamlCatalog or WoCCatalog? #Resolved

jaredhms · 2016-11-03T20:38:15Z

-    for (int i = 0; i < viewDepth; i++) {
+    // sendTouch to gesture for state transiton
+    BOOL gestureOnGoing = NO; 
+    for (int i = 0; i < gestureCount; i++) {


you should be able to do range-based for loops over both of these gesture arrays #Resolved

jaredhms · 2016-11-03T20:39:08Z

+
+    // scanning DM Getures, if a gestures is on going, cancel DM Gestures
+    // otherwise, send Touch to DM gestures
+    for (int i = 0; i < dmGestureCount; i++) {


for (int i = 0; i < dmGestureCount; i++) { [](start = 3, length = 43)

nit: range-based for here, too #Resolved

jaredhms · 2016-11-03T20:40:39Z

+                    [dmGesture performSelector:eventName withObject:[NSMutableSet setWithObject:touch] withObject:event];
+                    if (DEBUG_GESTURES) {
+                        TraceVerbose(TAG, L"Send Touch with phase=%d to DMGesture %hs.", touch.phase, object_getClassName(dmGesture));
+                    }


nit: why not move this into the above 'if' and ditch the 'send' variable? #WontFix

the current form is actually better. notice default is true. If remove send, the conditional clause becomes much more complex and not clear.

In reply to: 86438414 [](ancestors = 86438414)

jaredhms · 2016-11-03T20:43:46Z

-        UIGestureRecognizerState state = (UIGestureRecognizerState)[curgesture state];
+    //  Removed/reset failed/done gestures, including gestures and DMGestures
+    for (int i = 0; i < 2; i++) {
+        int count = (i == 0) ? gestureCount : dmGestureCount;


kind of strange; how about pulling this out into a shared function or lambda that gets called for both arrays? #Resolved

jaredhms · 2016-11-03T20:48:08Z

+
+    // setting transparent background so we can receive touch input
+    WUColor* convertedColor = ConvertUIColorToWUColor([UIColor clearColor]);
+    WUXMSolidColorBrush* brush = [WUXMSolidColorBrush makeInstanceWithColor:convertedColor];


how about just setting directly to the projected transparent brush rather than creating a UIColor only to convert to the projected type? Something like [WUXMSolidColorBrush makeInstanceWithColor:[WUColor transparent]] ? #Resolved

at which point you could just do _contentCanvas.background = [WUXMSolidColorBrush makeInstanceWithColor:[WUColor transparent]]

In reply to: 86439907 [](ancestors = 86439907)

like that

In reply to: 86440003 [](ancestors = 86440003,86439907)

oliversa-msft · 2016-11-03T20:54:17Z

+    // get the backing xaml UIElement for toLayer
+    WXUIElement* toLayerElement = _getBackingXamlElementForCALayer(toLayer);
+
+    // set up transform from xaml elment in fromLayer to xaml element in toLayer


Typo - element #Resolved

jaredhms · 2016-11-03T20:57:29Z

+    [self _setManipulationMode:WUXIManipulationModesSystem recursive:YES];
+
+    const std::vector<TouchInfo>& pointers = [gesture _getTouches];
+    for (size_t i = 0; i < pointers.size(); ++i) {


for (size_t i = 0; i < pointers.size(); ++i) { [](start = 2, length = 48)

range-based for #Resolved

jaredhms · 2016-11-03T20:57:58Z

+        // start Direct manipuation only if active touch presents
+        if (touch.phase != UITouchPhaseCancelled && touch->_routedEventArgs.pointer.pointerDeviceType == WDIPointerDeviceTypeTouch) {
+            if (![WXFrameworkElement tryStartDirectManipulation:touch->_routedEventArgs.pointer]) {
+                TraceWarning(TAG, L"DManip didn't start");


didn't [](start = 43, length = 6)

nit: 'failed to' might be more clear #Resolved

yiyang-msft · 2016-11-04T01:05:25Z

yes, we do. In WoCcatalog, the textview is a good example of gesture attached to subview get fired first and DM will be cancelled. and also, when DM gesture started, it can be chained to upper UIScrollview as well.

In reply to: 258266383 [](ancestors = 258266383)

yiyang-msft · 2016-11-04T01:11:24Z

 - (void)scrollViewDidZoom:(UIScrollView*)aScrollView {
    // don't have to do anything special to centralize the imageView after zoom when zoomFactor is less than 1
    // xaml scrollview does that automatically now
+    CGFloat offsetX = (scrollView.bounds.size.width > scrollView.contentSize.width) ?


CGFloat [](start = 4, length = 7)

accidental, i will revert. #Resolved

oliversa-msft · 2016-11-03T20:56:14Z

    return didRecognize;
 }

+- (const std::vector<TouchInfo>&)_getTouches {


Out of interest, why is it a reference and not just a copy? #Resolved

copies are wasteful

In reply to: 86441503 [](ancestors = 86441503)

However, if this case is it also why we need to keep the StrongId<> around? If we did make a copy are things cleaner? #Pending

you mean StrongId _routedEventArgs;? StrongId here is to make sure UITouch has the same life time for routedEventArgs, as we see in multiple-finger gesture cases, routedEventArgs can be become invalid though UITouch is still valid. Still a mystery to figure out why. But copy won't solve this problem either plus add additional cost.

In reply to: 86604604 [](ancestors = 86604604)

oliversa-msft · 2016-11-03T21:00:28Z

+    const std::vector<TouchInfo>& pointers = [gesture _getTouches];
+    for (size_t i = 0; i < pointers.size(); ++i) {
+        UITouch* touch = pointers[i].touch;
+        // start Direct manipuation only if active touch presents


I'm really bad at this but my eyes are drawn to capitalization changes - direct manipulation is what you used above. #Resolved

oliversa-msft · 2016-11-03T21:01:53Z

+    for (size_t i = 0; i < pointers.size(); ++i) {
+        UITouch* touch = pointers[i].touch;
+        // start Direct manipuation only if active touch presents
+        if (touch.phase != UITouchPhaseCancelled && touch->_routedEventArgs.pointer.pointerDeviceType == WDIPointerDeviceTypeTouch) {


Is _routedEventArgs.pointer guaranteed to be populated and non-null? #Resolved

yes

In reply to: 86442520 [](ancestors = 86442520)

oliversa-msft · 2016-11-03T22:19:45Z

    g_curGesturesDict = [NSMutableDictionary new];

-    UIGestureRecognizer* recognizers[128];
+    const static int MAXIMUM_GESTURE_ALLOW = 128;


Super nit: MAXIMUM_GESTURE_ALLOWED #Resolved

Wrong button pressed

jaredhms · 2016-11-04T17:46:57Z

 static const bool DEBUG_DELEGATE = DEBUG_ALL || false;
 static const bool DEBUG_INSETS = DEBUG_ALL || false;
 static const bool DEBUG_ZOOM = DEBUG_ALL || false;
+static const bool DEBUG_DMANIP_GESTURE = DEBUG_ALL || true;


true [](start = 54, length = 4)

set back to false before pushing? #Resolved

yeah.

In reply to: 86598268 [](ancestors = 86598268)

jaredhms · 2016-11-04T17:47:19Z

+    const std::vector<TouchInfo>& pointers = [gesture _getTouches];
+    for (auto const& pointer : pointers) {
+        UITouch* touch = pointer.touch;
+        // start Direct manipuation only if active touch presents


manipuation [](start = 24, length = 11)

nit: Typo #Resolved

jaredhms · 2016-11-04T17:48:17Z

+    // setting it to 7.0 which is slightly larger than 6.0 which is used
+    // by PanGestureRecognizer, this is to ensure PanGestureRecognizer kicks
+    // in first before us.
+    [self _setDragSlack:7.0f];


nit: maybe we should expose this off of a private UIPanGestureRecognizer property, so we can just +1 here? #Resolved

jaredhms · 2016-11-04T19:16:01Z


 #import <math.h>
 #import <string>
+#import <array>


remove? sorry for the noise! :/ #Resolved

jaredhms · 2016-11-04T19:31:42Z

Implement DMGestureRecognizer to start/stop directmanipluatio on scro…

82d3047

…llviewer so that other gestures attached to subviews inside UIScrollview can be properly triggered

yiyang-msft assigned davelamb, jaredhms, oliversa-msft and tadam-msft Nov 2, 2016

msftclas added the cla-not-required label Nov 2, 2016

yiyang-msft added 2 commits November 3, 2016 11:39

Fix wrong index

4cdcf03

More correction

553b3d2

DHowett-MSFT reviewed Nov 3, 2016

View reviewed changes

jaredhms reviewed Nov 3, 2016

View reviewed changes

comments

e12653b

jaredhms reviewed Nov 3, 2016

View reviewed changes

oliversa-msft reviewed Nov 3, 2016

View reviewed changes

jaredhms reviewed Nov 3, 2016

View reviewed changes

comments

b0a0702

yiyang-msft commented Nov 4, 2016

View reviewed changes

revert accidental change

7695d8f

oliversa-msft previously requested changes Nov 4, 2016

View reviewed changes

jaredhms reviewed Nov 4, 2016

View reviewed changes

more comments

26de622

jaredhms reviewed Nov 4, 2016

View reviewed changes

comments

810d387

remove unused variable

f8e82ba

yiyang-msft merged commit d6e3a6f into develop Nov 4, 2016

yiyang-msft deleted the Gesture branch November 4, 2016 21:00

Conversation

yiyang-msft commented Nov 2, 2016 • edited by rajsesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett-MSFT Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiyang-msft Nov 3, 2016

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms commented Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms Nov 3, 2016 • edited by yiyang-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiyang-msft Nov 4, 2016

Choose a reason for hiding this comment

yiyang-msft commented Nov 2, 2016 •

edited by rajsesh

Loading

DHowett-MSFT Nov 3, 2016 •

edited by yiyang-msft

Loading

DHowett-MSFT Nov 3, 2016 •

edited by yiyang-msft

Loading

DHowett-MSFT Nov 3, 2016 •

edited by yiyang-msft

Loading

DHowett-MSFT Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms commented Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

oliversa-msft Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 3, 2016 •

edited by yiyang-msft

Loading

yiyang-msft Nov 4, 2016 •

edited

Loading

oliversa-msft Nov 3, 2016 •

edited by yiyang-msft

Loading

oliversa-msft Nov 4, 2016 •

edited by yiyang-msft

Loading

oliversa-msft Nov 3, 2016 •

edited by yiyang-msft

Loading

oliversa-msft Nov 3, 2016 •

edited by yiyang-msft

Loading

oliversa-msft Nov 3, 2016 •

edited by yiyang-msft

Loading

jaredhms Nov 4, 2016 •

edited by yiyang-msft

Loading