[iOS] Dispose pickers properly and automate GC checks #632

Merged
merged 5 commits into from Feb 15, 2017

Conversation

Projects
None yet
5 participants
@adrianknight89
Contributor

adrianknight89 commented Dec 10, 2016

Description of Change

Went through renderers for Picker, TimePicker, and DatePicker to add/enhance dispose methods. Also added a UI test to automate GC checks. This should be run on top of #524.

Bugs Fixed

  • N/A

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@StephaneDelcroix

left a comment, review for similar issue elsewhere, thanks

+ if (e.NewElement == null)
+ return;
+
+ if (Control == null)
{
var entry = new NoCaretField { BorderStyle = UITextBorderStyle.RoundedRect };

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 10, 2016

Member

this will re-create an entry if the renderer is reused, instead of re-using it

@StephaneDelcroix

StephaneDelcroix Dec 10, 2016

Member

this will re-create an entry if the renderer is reused, instead of re-using it

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 10, 2016

Member

scratch that, sorry, the Control == null prevents this

@StephaneDelcroix

StephaneDelcroix Dec 10, 2016

Member

scratch that, sorry, the Control == null prevents this

+ }
+
+ if(Element != null)
+ ((INotifyCollectionChanged)Element.Items).CollectionChanged -= RowsCollectionChanged;

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 10, 2016

Contributor

@StephaneDelcroix I'm not 100% sure about this one. In a working situation, oldElement and newElement should be changed in a balanced way. When the object is being disposed, newElement should be set to null and OnElementChanged should be fired. This should unregister the handler because oldElement is not null.

This is just extra measure in case things don't work. Perhaps it makes sense to not do this and potentially end up with a leak so it could be investigated further and the root cause can be fixed.

@adrianknight89

adrianknight89 Dec 10, 2016

Contributor

@StephaneDelcroix I'm not 100% sure about this one. In a working situation, oldElement and newElement should be changed in a balanced way. When the object is being disposed, newElement should be set to null and OnElementChanged should be fired. This should unregister the handler because oldElement is not null.

This is just extra measure in case things don't work. Perhaps it makes sense to not do this and potentially end up with a leak so it could be investigated further and the root cause can be fixed.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 10, 2016

Contributor

@jassmith Some renderers seem to be calling base.OnElementChanged(e) at the end whereas most call it at the beginning. I'd like to see a consistent approach. So maybe you guys could discuss this internally and make necessary adjustments across solution if needed.

Contributor

adrianknight89 commented Dec 10, 2016

@jassmith Some renderers seem to be calling base.OnElementChanged(e) at the end whereas most call it at the beginning. I'd like to see a consistent approach. So maybe you guys could discuss this internally and make necessary adjustments across solution if needed.

- _picker.ValueChanged -= OnValueChanged;
+ if (_picker != null)
+ {
+ _picker.RemoveFromSuperview();

This comment has been minimized.

@rmarinho

rmarinho Dec 14, 2016

Member

This is done on the tracker or packager i think..

@rmarinho

rmarinho Dec 14, 2016

Member

This is done on the tracker or packager i think..

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 14, 2016

Contributor

Where is it done exactly?

@adrianknight89

adrianknight89 Dec 14, 2016

Contributor

Where is it done exactly?

adrianknight89 added some commits Dec 10, 2016

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Feb 3, 2017

Member

Rebased. waiting for ui tests

Member

StephaneDelcroix commented Feb 3, 2017

Rebased. waiting for ui tests

@rmarinho rmarinho merged commit 340a705 into xamarin:master Feb 15, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 352, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3738, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 35…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 350…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 352…
Details

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment