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.

Sign up for GitHub

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

Jump to bottom

Fix #171: Introduce support for diffing subsets of data in generic RecyclerView adapter #1359

Merged
merged 21 commits into from
Aug 11, 2020
Merged

Fix #171: Introduce support for diffing subsets of data in generic RecyclerView adapter #1359

merged 21 commits into from
Aug 11, 2020

Conversation

anandwana001
Copy link
Contributor

@anandwana001 anandwana001 commented Jun 18, 2020 •
edited
Loading

Explanation

Fix #171
Introduced a diffing logic using DiffUtil.
Added test cases which passed with diffutil implementation.

How to test

Check all possible screens where RecyclerView is used.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@BenHenning
Copy link
Sponsor Member

Aha, this is very interesting. :) I want to take a closer look at this, but I probably won't be able to until early next week.

In the meantime, @anandwana001 could you add tests for the diffing logic? I think we should try to encapsulate what the desired API of the adapter is via tests, even if we choose a different implementation.

@anandwana001
Copy link
Contributor Author

Sure

BenHenning
BenHenning requested changes Jun 23, 2020
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! This looks really good. Left a few comments, otherwise waiting for tests.

app/src/main/java/org/oppia/app/recyclerview/RecyclerDataDiffCallback.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/recyclerview/RecyclerDataDiffCallback.kt Show resolved Hide resolved
app/src/main/java/org/oppia/app/recyclerview/BindableAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/recyclerview/BindableAdapter.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Jun 23, 2020
@anandwana001
Copy link
Contributor Author

Thanks @anandwana001! This looks really good. Left a few comments, otherwise waiting for tests.

Could you give some hints, like what all cases we can cover with test?

BenHenning
BenHenning requested changes Jun 24, 2020
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! Left some more comments, mostly regarding the new tests.

app/src/main/java/org/oppia/app/recyclerview/BindableAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/recyclerview/RecyclerDataDiffCallback.kt Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
@anandwana001
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
27b26e1 
… introduce-diffing-generic-recyclerview
@anandwana001
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
3824ceb 
… introduce-diffing-generic-recyclerview

# Conflicts:
#	app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt
@anandwana001
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
5441eb9 
… introduce-diffing-generic-recyclerview
@anandwana001
added BindableAdapter test case list update
1cf9dd6
@anandwana001
fix ktlint issue
34ae007
@anandwana001 anandwana001 requested a review from rt4914 July 13, 2020 10:48
BenHenning
BenHenning reviewed Jul 14, 2020
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment •
edited by anandwana001
Loading

Choose a reason for hiding this comment

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

Thanks @anandwana001 --I took another pass on this.

app/src/main/java/org/oppia/app/recyclerview/BindableAdapter.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/recyclerview/BindableAdapter.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Jul 14, 2020
@anandwana001
Copy link
Contributor Author

anandwana001 commented Jul 16, 2020 •
edited
Loading

@BenHenning @rt4914 Added one test where I am checking if reminding is happening or not if in future we move to their implementation this test will fail as DiffUtil support no binding of the same data.
I should look into other approaches like inserting, deleting or moving of items in the list.
If you could add some feedback on this commit, this will help me in the other approaches for inserting, deleting or moving of items in the list.

@rt4914
Copy link
Contributor

rt4914 commented Jul 16, 2020

@BenHenning @rt4914 Added one test where I am checking if reminding is happening or not if in future we move to their implementation this test will fail as DiffUtil support no binding of the same data.
I should look into other approaches like inserting, deleting or moving of items in the list.
If you could add some feedback on this commit, this will help me in the other approaches for inserting, deleting or moving of items in the list.

@anandwana001 Actually I will defer with Ben on the test case approach because I am not confident about the correct approach in this case.

@rt4914 rt4914 removed their assignment Jul 16, 2020
@BenHenning
Copy link
Sponsor Member

Apologies for the delay, but I will need to take a pass on this PR tomorrow. I ran out of time to do so today.

BenHenning
BenHenning requested changes Jul 18, 2020
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! The test seems like a great start. I left some comments to help generalize it to multiple behaviors.

app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Jul 18, 2020
BenHenning
BenHenning approved these changes Aug 11, 2020
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! This looks good to me. Please address my last comment—either @rt4914 or I can verify it’s correct.

app/src/sharedTest/java/org/oppia/app/recyclerview/BindableAdapterTest.kt Outdated Show resolved Hide resolved
@anandwana001 anandwana001 assigned rt4914 and unassigned anandwana001 Aug 11, 2020
rt4914
rt4914 approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@anandwana001 anandwana001 mentioned this pull request Aug 11, 2020
Open
@rt4914
Copy link
Contributor

rt4914 commented Aug 11, 2020

This PR also solves #508

@rt4914 rt4914 merged commit 5532009 into oppia:develop Aug 11, 2020
@BenHenning
Copy link
Sponsor Member

@anandwana001 did you verify that the exploration player & all interactions still work? It seems this caused some regressions: #1625 and #1626.

BenHenning added a commit that referenced this pull request Aug 12, 2020
@BenHenning
Revert "Fix #171: Introduce support for diffing subsets of data in ge…
4481bba 
…neric RecyclerView adapter (#1359)"

This reverts commit 5532009.
@BenHenning BenHenning mentioned this pull request Aug 12, 2020
Merged
BenHenning added a commit that referenced this pull request Aug 12, 2020
@BenHenning
Revert "Fix #171: Introduce support for diffing subsets of data in ge…
de7487a 
…neric RecyclerView adapter (#1359)" (#1627)

This reverts commit 5532009.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aggarwalpulkit596 aggarwalpulkit596 aggarwalpulkit596 left review comments

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce support for diffing subsets of data in generic RecyclerView adapter
4 participants
@anandwana001 @BenHenning @rt4914 @aggarwalpulkit596