migueldeicaza / MonoTouch.Dialog

Tools to simplify creating dialogs with the user using MonoTouch
MIT License
430 stars 211 forks source link

[MT.D] Provide a way to remove subscribed [OnTap] events on reflection API #251

Closed dalexsoto closed 6 years ago

dalexsoto commented 6 years ago

Currently there is no way to unsubscribe to events derived from [OnTap] and this causes that DialogViewController to leak really bad. Take into consideration the following example:

[Preserve (AllMembers = true)]
class Settings {
[Section]
    [OnTap ("FetchValues")]
    public string FetchValues;

[Section]
    public bool AccountEnabled;
}

public class ReflectionDialogViewController : DialogViewController {
    Settings settings;
    BindingContext bc;

    public ReflectionDialogViewController () : base (UITableViewStyle.Grouped, null)
    {
        settings = new Settings { AccountEnabled = true };

        bc = new BindingContext (this, settings, $"Settings");
        Root = bc.Root;
    }

    public void FetchValues ()
    {
        bc.Fetch ();

        Console.WriteLine ($"Current status:");
        Console.WriteLine ($"AccountEnabled:  {settings.AccountEnabled}\n");
        Console.WriteLine ();
    }

    ~ReflectionDialogViewController () => Console.WriteLine ($"Finalized: ~ReflectionDialogViewController ()");

    public override void ViewDidLoad ()
    {
        base.ViewDidLoad ();

        NavigationItem.LeftBarButtonItem = new UIBarButtonItem (UIBarButtonSystemItem.Done);
        NavigationItem.LeftBarButtonItem.Clicked += DoneButtonItem_Clicked;
    }

    void DoneButtonItem_Clicked (object sender, EventArgs e) => DismissViewController (false, null);

    public override void ViewDidDisappear (bool animated)
    {
        base.ViewDidDisappear (animated);
        NavigationItem.LeftBarButtonItem.Clicked -= DoneButtonItem_Clicked;
    }
}

In the above example FetchValues would be added as a Tapped event handler using an anonymous delegate which turns to be imposible to be removed from the created StringElement and leading into a retain cicle impossible to be broken.

By adding a 1:1 of handlerMappings (Element to anonymous delegate) we are able to unsubscribe using the anonymous delegate by manually calling Dispose in the BindingContext object, this will take care to unsubscribe any Tapped event and avoid any retain cycle.

So now when we are done with the DialogViewController and the BindingContext, you would need to manually call Dispose on the BindingContext typically in ViewDidDisappear of the DialogViewController when it is no longer needed.

For this particular example:

public override void ViewDidDisappear (bool animated)
{
    base.ViewDidDisappear (animated);
    NavigationItem.LeftBarButtonItem.Clicked -= DoneButtonItem_Clicked;

    // Ensure any OnTap handlers are removed by calling dispose
    bc.Dispose ();
}

Test Case

I have attached a test case that demonstrates that the fix works.

MonoTouch.Dialog-OnTapFix.zip

  1. Open MonoTouch.Dialog.sln => MTDRetainCicles sample and run it
  2. Click on Reflection DVC
  3. Click on Done
  4. Repeat steps 2 and 3 several times
  5. Observe Finalized: ~ReflectionDialogViewController in console output meaning that finalizer got called.
  6. Stop debugging and remove bc.Dispose (); from ViewDidDisappear and repeat step 4
  7. Observe that the finalizer is never called.