Skip to content

[UIKit] UIGestureRecognizer, support a way of unsubscribing without creating cycles #4729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rolfbjarne merged 6 commits into from
Oct 18, 2018
Merged

[UIKit] UIGestureRecognizer, support a way of unsubscribing without creating cycles #4729

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d282075
[UIKit] UIGestureRecognizer, support a way of unsubscribing without c…
migueldeicaza Aug 30, 2018
f4a8e9b
Simplify code a little bit.
rolfbjarne Sep 13, 2018
57f0cac
Merge remote-tracking branch 'origin/master' into uigesturerecognizer…
rolfbjarne Sep 13, 2018
2e28fa5
Merge remote-tracking branch 'origin/master' into uigesturerecognizer…
rolfbjarne Oct 15, 2018
440c38a
Add test.
rolfbjarne Oct 15, 2018
5871aa8
[tests] Add an NSAutoreleasePool to make GestureRecognizerTest.NoStro…
rolfbjarne Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 32 additions & 20 deletions src/UIKit/UIGestureRecognizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@

using System;
using System.Collections;
using System.Collections.Generic;
using Foundation;
using ObjCRuntime;
using CoreGraphics;

namespace UIKit {
public partial class UIGestureRecognizer {
object recognizers;
//
// Tracks the targets (NSObject, which we always enforce to be Token) to the Selector the point to, used when disposing
//
Dictionary<Token,IntPtr> recognizers = new Dictionary<Token,IntPtr> ();
const string tsel = "target";
internal const string parametrized_selector = "target:";
#if !XAMCORE_2_0
Expand All @@ -30,18 +34,26 @@ public partial class UIGestureRecognizer {
{
}

// Called by the Dispose() method
void OnDispose ()
{
foreach (var kv in recognizers)
RemoveTarget (kv.Key, kv.Value);
recognizers = null;
}

//
// Signature swapped, this is only used so we can store the "token" in recognizers
//
public UIGestureRecognizer (Selector sel, Token token) : this (token, sel)
{
recognizers = token;
recognizers [token] = sel.Handle;
Copy link
Contributor

@spouliot spouliot Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that can cause an NRE here since the check (in this) would be done later.

Copy link
Contributor Author

@migueldeicaza migueldeicaza Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean because the selector might be null? I do not follow the "would be done later" part

Copy link
Contributor Author

@migueldeicaza migueldeicaza Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would call the constructor generated for the init: method, and that one should throw already if either sel or token are null in this case.

Copy link
Contributor

@spouliot spouliot Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the input can be null, directly or indirectly, e.g. Selector.FromHandle can return null

new UIGestureRecognizer (null, token); would throw the NullReferenceException before it calls this (token, sel) which has the null check (and would throw an ArgumentNullException)

Copy link
Member

@rolfbjarne rolfbjarne Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spouliot the other (this) constructor is called first, not last.

MarkDirty ();
}

internal UIGestureRecognizer (IntPtr sel, Token token) : this (token, sel)
{
recognizers = token;
recognizers [token] = sel;
MarkDirty ();
}

Expand Down Expand Up @@ -111,17 +123,7 @@ void RegisterTarget (Token target, IntPtr sel)
{
AddTarget (target, sel);
MarkDirty ();
if (recognizers == null)
recognizers = target;
else {
Hashtable table = recognizers as Hashtable;
if (table == null){
table = new Hashtable ();
table [recognizers] = recognizers;
recognizers = table;
}
table [target] = target;
}
recognizers [target] = sel;
}

public void RemoveTarget (Token token)
Expand All @@ -130,12 +132,22 @@ public void RemoveTarget (Token token)
throw new ArgumentNullException ("token");
if (recognizers == null)
return;
if (recognizers == token)
recognizers = null;
Hashtable asHash = recognizers as Hashtable;
if (asHash != null)
asHash.Remove (token);
RemoveTarget (token, token is ParametrizedDispatch ? Selector.GetHandle (parametrized_selector) : Selector.GetHandle (tsel));
if (recognizers.ContainsKey (token)){
Copy link
Member

@rolfbjarne rolfbjarne Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor code improvement:

if (recognizers.TryGetValue (token, out var sel)) {
    recognizers.Remove (token);
    RemoveTarget (token, sel);
}

Copy link
Contributor Author

@migueldeicaza migueldeicaza Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it

Copy link
Contributor

@Therzok Therzok Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given XI targets modern mono with corefx APIs, it can just be:

if (recognizers.Remove (token, out var sel)) {
    RemoveTarget (token, sel);
}

var sel = recognizers [token];
recognizers.Remove (token);
RemoveTarget (token, sel);
}
}

//
// Used to enumerate all the registered handlers for this UIGestureRecognizer
//
public IEnumerable<Token> GetTargets ()
{
if (recognizers == null)
Copy link
Member

@rolfbjarne rolfbjarne Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be:

public IEnumerable<Token> GetTargets ()
{
    return recognizers?.Keys;
}

Copy link
Contributor Author

@migueldeicaza migueldeicaza Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to avoid the for loop, but I will keep the split yield break/yield return to avoid returning a null for empty cases, instead, by catching this early we would return an empty collection.

Copy link
Contributor

@Therzok Therzok Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recognizers?.Keys ?? Array.Empty<Token>? That would avoid allocating an Iterator for the method.

yield break;
foreach (var kv in recognizers)
yield return kv.Key;
}
}

Expand Down
1 change: 1 addition & 0 deletions src/uikit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5868,6 +5868,7 @@ partial interface UIFontDescriptor : NSSecureCoding, NSCopying {

#if !WATCH
[BaseType (typeof(NSObject), Delegates=new string [] {"WeakDelegate"}, Events=new Type[] {typeof (UIGestureRecognizerDelegate)})]
[Dispose ("OnDispose ();")]
interface UIGestureRecognizer {
[DesignatedInitializer]
[Export ("initWithTarget:action:")]
Expand Down