Skip to content
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

[Android] Fixed CollectionView reordering last item #17825

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vitalii-vov
Copy link

Description of Change

Fixed application crash when dragging an element to the end of a collection on Android.

Issues Fixed

Fixes #17823

@vitalii-vov vitalii-vov requested a review from a team as a code owner October 4, 2023 12:55
@ghost ghost added the community ✨ Community Contribution label Oct 4, 2023
@ghost
Copy link

ghost commented Oct 4, 2023

Hey there @vitalii-vov! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@Eilon Eilon added the area-controls-collectionview CollectionView, CarouselView, IndicatorView label Oct 4, 2023
@samhouts samhouts added this to the Under Consideration milestone Oct 5, 2023
@vitalii-vov vitalii-vov requested a review from rmarinho October 6, 2023 13:30
@akhanalcs
Copy link

@vitalii-vov Can you add a unit test for this?

@vitalii-vov
Copy link
Author

@rmarinho
Perhaps I didn’t understand something, but for some reason the tests on GitHub were not launched?
If you think this fix is not correct, you may want to undo it or run tests.
I also marked PullRequest as "Allow edits by maintainers" and you can change it if you see fit.
As I can see, issue #17823 is affecting many users and will continue to do so in the future. The essence of the problem is known - it is the index offset in the RecyclerView after adding a Header to it.

@jfversluis
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

Could you share a sample where could reproduce the issue before the changes?. I would use it to validate in addition to update the PR adding an UI Test.

@vitalii-vov
Copy link
Author

Hello,
Here is the HeaderTemplate Example #17825 (comment)

Another example is just add

<CollectionView.Header>
    <Label Text="Hello"/>
</CollectionView.Header>

to your CollectionView

@Domik234
Copy link

Hello,
will this be released for next SR? @jsuarezruiz , @rmarinho

The base of this fix is already visible on lines 36 and 41.

var fromItemIndex = fromIndex - (fromItemsSource?.HasHeader == true ? 1 : 0);

var toItemIndex = toIndex - (toItemsSource?.HasHeader == true ? 1 : 0);

Author just recreated fix used in the situation where "IsGrouped == true && Header != null" the same way for case where "IsGrouped = false && Header != null".

I've already tested this and I am currently using it in project the hard way (recreating classes and overriding methods).

This can solve application crashing for ppl using Header or HeaderTemplate and not using IsGrouped at the same time so from the base of logic this should be free fix even for testing.
Understandable could be request to make it work the same way as for the Grouped ones (using directly value from .HasHeader than from custom bool field) but holding this PR back just leaves unnecessary bug in code.

Video - Testing
PR17825-2.webm

Starts with No Fix
00:14 - Adding items to collection
00:18 -> 00:20 - Drag test - crashed
00:33 - Reapplying fix (based on @vitalii-vov )

Fixed
00:42 - Rebuilding application with fix
01:05 - Adding items to collection
01:09 -> 01:12 - Drag test - successful
01:14 -> 01:20 - Custom logic test (drag and drop based on item type)

Thanks,
Dominik Švarc

@PureWeen PureWeen removed this from the Under Consideration milestone Aug 2, 2024
@rmarinho
Copy link
Member

/rebase

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

We need to rebase, but also add a test for this issue.
@jfversluis do you know if we have any reorder test ?

@rmarinho
Copy link
Member

rmarinho commented Dec 9, 2024

/rebase

@rmarinho
Copy link
Member

rmarinho commented Dec 9, 2024

Needs rebase and add a test

@rmarinho rmarinho changed the title Fixed CollectionView reordering last item [Android] Fixed CollectionView reordering last item Dec 11, 2024
@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

We should use HasHeader The adapter seems to have a one

ItemsViewAdapter.ItemsSource.HasHeader;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution stale Indicates a stale issue/pr and will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android app crashes when dragging into CollectionView
9 participants