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.

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

[iOS] Dispose Entry and Editor properly and automate GC checks #628

Merged
merged 7 commits into from Feb 3, 2017

Conversation

Projects
None yet
5 participants
@adrianknight89
Contributor

adrianknight89 commented Dec 9, 2016

Description of Change

When you have a ListView where each ViewCell contains an Entry, popping the current page does not release it because EntryRenderer is missing if (e.NewElement != null) in OnElementChanged. I have a UI test case that could be automated. Let me know if you want me to push it as well. Since I haven't created a bug, I do not know what to name the test class.

Please run this 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
Show outdated Hide outdated Xamarin.Forms.Platform.iOS/Renderers/EntryRenderer.cs
var textField = Control;
if (Control == null)
if (e.NewElement != null)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member
if (e.NewElement == null)
    return;
@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member
if (e.NewElement == null)
    return;
@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 9, 2016

Contributor

@StephaneDelcroix Is it OK if I go through all renderers across solution and fix similar issues? I think EntryRenderer is not the only one.

Contributor

adrianknight89 commented Dec 9, 2016

@StephaneDelcroix Is it OK if I go through all renderers across solution and fix similar issues? I think EntryRenderer is not the only one.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

@adrianknight89 you do whatever you want with your time :) You should probably first make a test for this one, and get this PR reviewed by @jassmith (I'm asking him to review) before spending too much time on it

Member

StephaneDelcroix commented Dec 9, 2016

@adrianknight89 you do whatever you want with your time :) You should probably first make a test for this one, and get this PR reviewed by @jassmith (I'm asking him to review) before spending too much time on it

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 9, 2016

Contributor

@StephaneDelcroix Do I need to create a bug in order to submit a test case since all UI tests seem to be placed in Bugzilla[ID]?

Contributor

adrianknight89 commented Dec 9, 2016

@StephaneDelcroix Do I need to create a bug in order to submit a test case since all UI tests seem to be placed in Bugzilla[ID]?

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Dec 9, 2016

Member

@adrianknight89 no.

we have some issues (at the very bottom) without any bug association. use this attribute:

[Issue (IssueTracker.None, 0, "Swipe back nav crash", PlatformAffected.iOS)]
Member

StephaneDelcroix commented Dec 9, 2016

@adrianknight89 no.

we have some issues (at the very bottom) without any bug association. use this attribute:

[Issue (IssueTracker.None, 0, "Swipe back nav crash", PlatformAffected.iOS)]
@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 9, 2016

Contributor

@StephaneDelcroix I'll submit separate PRs for the other renderers or else this PR will potentially become huge.

Contributor

adrianknight89 commented Dec 9, 2016

@StephaneDelcroix I'll submit separate PRs for the other renderers or else this PR will potentially become huge.

@jassmith

This comment has been minimized.

Show comment
Hide comment
@jassmith

jassmith Dec 9, 2016

Member

This looks fine, though due to the deep nature of the change we need to get all automated tests run, I have queued them up.

Member

jassmith commented Dec 9, 2016

This looks fine, though due to the deep nature of the change we need to get all automated tests run, I have queued them up.

@adrianknight89 adrianknight89 changed the title from [iOS] Fix Entry memory leak to [iOS] Dispose Entry and Editor properly and automate GC checks Dec 10, 2016

adrianknight89 added some commits Dec 9, 2016

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Feb 2, 2017

Member

I rebased your branch to try to get a successful test run

Member

StephaneDelcroix commented Feb 2, 2017

I rebased your branch to try to get a successful test run

@StephaneDelcroix StephaneDelcroix merged commit 0dff517 into xamarin:master Feb 3, 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: 3733, 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: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 349…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 351…
Details

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

@samhouts samhouts modified the milestone: 3.1.0 Jun 1, 2018

@samhouts samhouts added this to Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

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