Adding the ability to subclass UIGestureRecognizer by pittmab · Pull Request #1002 · microsoft/WinObjC

pittmab · 2016-09-13T14:43:00Z

removing the hard coded preference of gestures
now enumerating the list of gestures in the order in which they are added
supporting some of the callbacks for UIGestureRecognizerDelegate
Adding a callback to check if the first active gesture can prevent other potential gestures in the gesture recognizer list.

Fix: 489
Fix: 475

This change is

msftclas · 2016-09-13T14:43:04Z

Hi @pittmab, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

aballway · 2016-09-13T15:19:26Z

+    UIGestureRecognizer* activeGesture = nil;
+
+    int count = [g_currentlyTrackingGesturesList count];
+    for (int i = count - 1; i >= 0; i--) {


nit: this could be converted into a for in loop

@aballway I am guessing that you would prefer that we use a reverseObjectEnumerator to traverse in reverse order. I have noticed that the code in this file uses a mixture of old for counting loops and for in loops. Should all new code be for in when it can? I initially used the style I found in the existing code.

I think it's better style to use for in loops unless the index is needed or the entire file is in counting style. Given this file is mixed either one would work, but I believe using for/in is clearer.

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

@aballway I agree that for in is quicker to read so I will make the changes. Just curious as to what your team prefers given the mix in this file. I will make the changes.

jaredhms · 2016-09-13T16:33:39Z

@davelamb, please take a look.

jaredhms · 2016-09-13T17:02:16Z

+        }
+    }
+
+    if ( activeGesture ) {


activeGesture [](start = 8, length = 15)

super-nit: several spots with inconsistent spacing; it may be beneficial to run pre_commit(.ps1) or pre_push(.ps1) which would run clang-format over your changes.

@jaredhms I just ran it and it doesn't indicate that any changes are necessary. Is there something that I am missing? Thanks

do you have this file staged with changes?

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

I edited the file and ran the pre-commit check. The output was Performing pre-commit validation... Commit approved.

Strange; it should have modified the file in place to remove the spacing inconsistencies. it might be worth trying .\tools\scripts\clang-format.ps1 -File to see if you get different results?

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

Yes. I have to run clang-format. Thanks

Thanks - that's a bit unfortunate. Might be worth trying pre_push next time (after you commit, before you push)...

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

jaredhms · 2016-09-13T17:08:55Z

+        // remove those gesturee
+        for ( UIGestureRecognizer* curGesture in gesturesToRemove ) {
+            [curGesture reset];
+            TraceVerbose(TAG, L"Removing gesture %hs %x state=%d", object_getClassName(curGesture), curGesture );


TraceVerbose(TAG, L"Removing gesture %hs %x state=%d", object_getClassName(curGesture), curGesture ); [](start = 9, length = 104)

can you please wrap this in an 'if (DEBUG_GESTURES) {}' ?

@jaredhms Added and other changes have been made. Please take a look.

jaredhms · 2016-09-13T17:57:38Z

+    for (UIGestureRecognizer* curGesture in [g_currentlyTrackingGesturesList reverseObjectEnumerator]) {
+        id curgestureClass = [curGesture class];
+        if ([curgestureClass _fireGestures:@[ curGesture ]]) {
+            handled = false;


This should still be 'true', shouldn't it?

Also can we please add the old tracing back?

if (handled && DEBUG_GESTURES) { TraceVerbose(TAG, L"Gesture (%hs) handled.", object_getClassName(curgestureClass)); }

@jaredhms Fixed and the original debugging code put back. Thanks

- removing the hard coded preference of gestures - now enumerating the list of gestures in the order in which they are added - supporting some of the callbacks for UIGestureRecognizerDelegate - Adding a callback to check if the first active gesture can prevent other potential gestures in the gesture recognizer list. Fix: 489 Fix: 475

jaredhms · 2016-09-14T23:12:55Z

bbowman · 2016-09-23T21:56:29Z

Seems like this would merit some sort of UT to validate. I know @davelamb mentioned not having UI automation yet but I don't think the test needs to be that extreme to prove out that the subclassing works and that the code you changed is operating correctly / not regressing things etc.

bbowman · 2016-09-23T21:57:12Z

🕐

yiyang-msft · 2017-02-03T00:02:05Z

Hi, @pittmab,

I merge your code into a new pull request #1909, would you mind taking a look? The gesture code has gone through significant changes since this request. It would be a loteasier for me to pull your code into latest code and working on top of it.

Thanks
Yi

tadam-msft · 2017-04-11T16:59:45Z

As seen in the thread the pull request was incorporated into another pull request and checked in. This pull request is no longer needed.

msftclas added the cla-already-signed label Sep 13, 2016

aballway reviewed Sep 13, 2016
View reviewed changes

jaredhms reviewed Sep 13, 2016
View reviewed changes

pittmab force-pushed the Subclass-UIGestureRecognizer branch from 08aa54c to 7269fc0 Compare September 13, 2016 17:05

jaredhms reviewed Sep 13, 2016
View reviewed changes

pittmab force-pushed the Subclass-UIGestureRecognizer branch from 7269fc0 to dc5fe0e Compare September 13, 2016 17:37

jaredhms reviewed Sep 13, 2016
View reviewed changes

pittmab force-pushed the Subclass-UIGestureRecognizer branch from dc5fe0e to 1c72432 Compare September 14, 2016 15:31

rajsesh added the Area-User Experience label Sep 20, 2016

This was referenced Oct 12, 2016

Controlling Simultaneous Gesture Recognition not supported #475

Closed

Creating a custom gesture recognizer is not supported #489

Closed

yiyang-msft mentioned this pull request Feb 2, 2017

Supporting gesture subclassing and firing delegates for supporting gestures concurrent firing negotiations. #1909

Merged

tadam-msft closed this Apr 11, 2017

Conversation

pittmab commented Sep 13, 2016 • edited by rajsesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhms commented Sep 14, 2016

Uh oh!

bbowman commented Sep 23, 2016

Uh oh!

bbowman commented Sep 23, 2016

Uh oh!

yiyang-msft commented Feb 3, 2017

Uh oh!

tadam-msft commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pittmab commented Sep 13, 2016 •

edited by rajsesh

Loading