Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Android] Fix exception removing item from CarouselView #11074

Merged
merged 12 commits into from
Oct 29, 2020
Merged

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Jun 16, 2020

Description of Change

Fix exception removing item from CarouselView on Android.

Deleting items (or adding) sets the CarouselView CurrentItem. To update the current item, we use the IItemsViewSource GetItem method that gets the item based on the position. The error was that the position value was -1 (incorrect) because the comparison of items made in the IItemsViewSource GetPosition method was incorrect.

NOTE: Tested the same on iOS and is working without problems.

Issues Resolved

API Changes

None

Platforms Affected

  • Android

Behavioral/Visual Changes

None

Before/After Screenshots

Before

Crash! (Add Item, remove it and try to add again).

After

fix10865

Testing Procedure

Launch Core Gallery and navigate to the issue10865. Click add until you have more than one item, then scroll to the last item and remove the last item. Without exception, the test has passed.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@jsuarezruiz jsuarezruiz added t/bug 🐛 p/Android i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often a/carouselview labels Jun 16, 2020
}

[Preserve(AllMembers = true)]
public class Issue10865ViewModel
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be automated.

Copy link
Contributor

@hartez hartez Jun 24, 2020

Choose a reason for hiding this comment

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

In fact, since it's just a test of GetPosition, it could just be a platform test; no UI test necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the test instructions do not match the instructions from the original issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, added unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unit test looks good, can we just remove this screen entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can remove it, but, why not leave the screen next to the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's one more screen that we have to maintain as things change, and one more class/screen to load every time we load the solution or test.

@samhouts samhouts added the Core label Jul 7, 2020
@samhouts samhouts changed the base branch from 4.7.0 to 4.8.0 July 8, 2020 18:13
@samhouts samhouts removed this from In Review in 4.7.0 Jul 8, 2020
@samhouts samhouts added this to In Review in CollectionView Jul 13, 2020
@samhouts samhouts added this to In Review in CarouselView Jul 13, 2020
@samhouts samhouts added this to In Progress in vCurrent (4.8.0) Jul 30, 2020
@samhouts samhouts requested a review from hartez August 4, 2020 22:11
@samhouts samhouts changed the base branch from 4.8.0 to 5.0.0 August 4, 2020 22:11
@samhouts samhouts added this to the 5.0.0 milestone Aug 4, 2020
@samhouts samhouts removed this from In Progress in vCurrent (4.8.0) Aug 4, 2020
@samhouts samhouts added this to In Progress in vNext+1 (5.0.0) Aug 11, 2020
Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

Wondering if we can just remove the Issue10865 files entirely since we have the unit test.

}

[Preserve(AllMembers = true)]
public class Issue10865ViewModel
Copy link
Contributor

Choose a reason for hiding this comment

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

Unit test looks good, can we just remove this screen entirely?

@rmarinho rmarinho merged commit fd123d9 into 5.0.0 Oct 29, 2020
CollectionView automation moved this from In Review to Done Oct 29, 2020
CarouselView automation moved this from In Review to Done Oct 29, 2020
@rmarinho rmarinho deleted the fix-10865 branch October 29, 2020 14:49
vNext+1 (5.0.0) automation moved this from In Progress to Done Oct 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/carouselview a/collectionview Core i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often p/Android t/bug 🐛
Projects
CarouselView
  
Done
Development

Successfully merging this pull request may close these issues.

[Bug] CarouselView Throws when removing an item.
4 participants