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

Feature: ReactiveRecyclerViewAdapter.GetItemViewType now passed view model type #2051

Conversation

Projects
None yet
3 participants
@clintonrocksmith
Copy link
Contributor

commented May 26, 2019

What kind of change does this PR introduce?
Feature

What is the current behavior?
The GetItemViewType is standard with only a position ID, which cannot be used to work out what item belongs in the sourceList.

What is the new behavior?
The GetItemViewType addition, now bubbles up the current position as well as the ViewModel so that the cell view type can be worked out. This is important following best practices when a RecyclerView has different ViewHolders inside it. With different View Cells.

What might this PR break?
Unsure

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

clintonrocksmith added some commits May 26, 2019

Added new method to provide the ViewModel and position Id
Also guarded against possible null exceptions with the lists by trying to get the ViewModel at a position

@clintonrocksmith clintonrocksmith requested a review from reactiveui/android-team as a code owner May 26, 2019

@glennawatson glennawatson changed the title Feature get item view type added that includes the view model for the cell Feature: ReactiveRecyclerViewAdapter.GetItemViewType now passed view model type May 26, 2019

@glennawatson glennawatson merged commit 090e6d4 into reactiveui:master May 26, 2019

4 checks passed

ReactiveUI-CI Build #9.16.6+9410541f9c succeeded
Details
ReactiveUI-CI (Mac) Mac succeeded
Details
ReactiveUI-CI (Windows) Windows succeeded
Details
license/cla All CLA requirements met.
Details
@@ -90,7 +90,7 @@ protected override void Dispose(bool disposing)

private TViewModel GetViewModelByPosition(int position)
{
return _list.Count > position ? null : _list.Items.ElementAt(position);
return position > _list.Count ? null : _list.Items.ElementAt(position);

This comment has been minimized.

Copy link
@worldbeater

worldbeater May 27, 2019

Member

Won't this throw IndexOutOfBoundsException here? E.g. in case when position equals to _list.Count. Probably worth changing > to >= and covering this stuff with some unit tests if possible to verify such cases

This comment has been minimized.

Copy link
@glennawatson

glennawatson May 27, 2019

Contributor

Hard to do unit tests at the moment with Android. Since we are limited to azure DevOps. There is a task for adding them with tutorial how to achieve it.

This comment has been minimized.

Copy link
@clintonrocksmith

clintonrocksmith May 29, 2019

Author Contributor

Position is zero based whereas count starts from 1, if it hits null then there is some other problem
The usual code is _list.Items.ElemantAt(position) without the check. At least it returns an Empty ViewModel for the implementation. I might have to re-think how this works though. Hmmmmm fresh eyes.

ReacvityRecyclerViewAdapter cannot be used directly, it needs to be used in-directly

Thanks for the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.