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

[Core] Fix internal children clear logic #820

Merged
merged 8 commits into from Mar 20, 2017

Conversation

Projects
None yet
7 participants
@wplong11

wplong11 commented Mar 16, 2017

Description of Change

I think the code before the change was intended to remove all the items in the InternalChildren. However, the code is removing only the odd-numbered items from InternalChildren. This PR modifies the code to fit the original intent.

Bugs Fixed

This PR modifies the code to fit the original intent.

API Changes

None

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
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Mar 16, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

dnfclas commented Mar 16, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@wplong11

This comment has been minimized.

Show comment
Hide comment
@wplong11

wplong11 Mar 16, 2017

InternalChildren.Clear() is most suitable for removing all items of InternalChildren. However, It has an impact on the CollectionChanged event handler. So I used the Remove method to change the code at the smallest level.

wplong11 commented Mar 16, 2017

InternalChildren.Clear() is most suitable for removing all items of InternalChildren. However, It has an impact on the CollectionChanged event handler. So I used the Remove method to change the code at the smallest level.

@StephaneDelcroix

could you please add a unitest to make such a silly mistake doesn't happen anymore ?

Show outdated Hide outdated Xamarin.Forms.Core/TemplateUtilities.cs
{
self.InternalChildren.Remove(self.InternalChildren[i]);
self.InternalChildren.RemoveAt(0);

This comment has been minimized.

@Therzok

Therzok Mar 16, 2017

Member

This makes the clearing an O(n^2) operation. Changing t his to self.InternalChildren.RemoveAt(self.InternalChildren.Count - 1) would make it O(n).

@Therzok

Therzok Mar 16, 2017

Member

This makes the clearing an O(n^2) operation. Changing t his to self.InternalChildren.RemoveAt(self.InternalChildren.Count - 1) would make it O(n).

Show outdated Hide outdated Xamarin.Forms.Core/TemplateUtilities.cs
{
self.InternalChildren.Remove(self.InternalChildren[i]);
self.InternalChildren.RemoveAt(0);

This comment has been minimized.

@Therzok

Therzok Mar 16, 2017

Member

This makes the clearing an O(n^2) operation. Changing t his to self.InternalChildren.RemoveAt(self.InternalChildren.Count - 1) would make it O(n).

@Therzok

Therzok Mar 16, 2017

Member

This makes the clearing an O(n^2) operation. Changing t his to self.InternalChildren.RemoveAt(self.InternalChildren.Count - 1) would make it O(n).

@wplong11 wplong11 changed the title from Fix internal children clear logic to [Core] Fix internal children clear logic Mar 17, 2017

@rmarinho rmarinho merged commit f98e152 into xamarin:master Mar 20, 2017

7 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3749, ignored: 10
Details
Windows-Manual-Debug Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Manual PR Review : Tests passed: 3749, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details
@assemhakmeh

Just a small remark. Speeding up this loop would benefit from a for loop as the original code did but with i set to Count and decreasing to zero, also adding count to a local var for the loop check and finally calling RemoveAt(i). This would effectively do the same while removing the 2x Count property access on every iteration.

@samhouts samhouts added D-15.4 and removed cla-not-required 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