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

Fixes #3095, #2824: Use protos with intent extras in RecentlyPlayedActivity #4511

Merged
merged 78 commits into from
Oct 5, 2022
Merged

Fixes #3095, #2824: Use protos with intent extras in RecentlyPlayedActivity #4511

merged 78 commits into from
Oct 5, 2022

Conversation

vrajdesai78
Copy link
Contributor

@vrajdesai78 vrajdesai78 commented Aug 19, 2022 •
edited by BenHenning
Loading

Explanation

Fixes #3095
Fixes #2824

Added RecentlyPlayedActivityParams and DestinationScreen proto.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

Video

recently_played.mp4

rt4914
rt4914 reviewed Aug 19, 2022
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.

PTAL

@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Aug 19, 2022
@vrajdesai78 vrajdesai78 assigned rt4914 and unassigned vrajdesai78 Aug 19, 2022
@vrajdesai78 vrajdesai78 changed the title Fixes 3095: Use protos with intent extras in RecentlyPlayedActivity Fixes #3095: Use protos with intent extras in RecentlyPlayedActivity Aug 19, 2022
@BenHenning
Copy link
Member

@vrajdesai78 I believe your Dagger error is coming from the fact that you're using @Binds instead of @Provides in ActivityRouterModule. Dagger doesn't know how to turn DestinationScreen into DestinationScreen.DestinationScreenCase, so it's throwing an error. I think you want to use @Provides here so that you can add code for converting between the two, or you'll want to change the types so that Dagger can correctly convert between the two types when generating the binding code.

@vrajdesai78
Copy link
Contributor Author

@vrajdesai78 I believe your Dagger error is coming from the fact that you're using @Binds instead of @Provides in ActivityRouterModule. Dagger doesn't know how to turn DestinationScreen into DestinationScreen.DestinationScreenCase, so it's throwing an error. I think you want to use @Provides here so that you can add code for converting between the two, or you'll want to change the types so that Dagger can correctly convert between the two types when generating the binding code.

Thanks @BenHenning, I have replaced @BINDS with @provides and now error is solved.

@vrajdesai78 vrajdesai78 self-assigned this Aug 19, 2022
@vrajdesai78 vrajdesai78 changed the title Fixes #3095: Use protos with intent extras in RecentlyPlayedActivity Fixes #3095 & #2824: Use protos with intent extras in RecentlyPlayedActivity Aug 19, 2022
@vrajdesai78 vrajdesai78 changed the title Fixes #3095 & #2824: Use protos with intent extras in RecentlyPlayedActivity Fixes #3095, #2824: Use protos with intent extras in RecentlyPlayedActivity Aug 19, 2022
rt4914
rt4914 suggested changes Aug 19, 2022
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.

@vrajdesai78 In source activity i.e., HomeActivity or ProfileProgressActivity please add tests showing that intents are getting passed correctly for the RecentlyPlayedActivity

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivityPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedActivityPresenter.kt Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/profileprogress/ProfileProgressActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/profileprogress/ProfileProgressHeaderViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/home/HomeActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/activity/route/RouteRule.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Aug 19, 2022
@vrajdesai78 vrajdesai78 requested a review from BenHenning October 4, 2022 20:23
BenHenning
BenHenning requested changes Oct 5, 2022
View reviewed changes
Copy link
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 @vrajdesai78. Took another pass--PTAL.

app/src/main/java/org/oppia/android/app/activity/route/ActivityRouter.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterModuleTest.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterModuleTest.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterModuleTest.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterModuleTest.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterModuleTest.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterTest.kt Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Oct 5, 2022
@vrajdesai78 vrajdesai78 requested a review from BenHenning October 5, 2022 03:30
BenHenning
BenHenning reviewed Oct 5, 2022
View reviewed changes
Copy link
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 @vrajdesai78. Just had 2 follow-up comments, and per chat CI is failing & needs to be fixed.

app/src/test/java/org/oppia/android/app/activity/route/ActivityRouterModuleTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfileProgressFragmentTest.kt Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Oct 5, 2022
@vrajdesai78
Copy link
Contributor Author

vrajdesai78 commented Oct 5, 2022 •
edited
Loading

Thanks @vrajdesai78. Just had 2 follow-up comments, and per chat CI is failing & needs to be fixed.

Followed up on your comments and fixed failing CI checks. Still I didn't find a good name for tests in ProfileProgressFragmentTest which fits in character limit

@vrajdesai78 vrajdesai78 requested a review from BenHenning October 5, 2022 05:22
BenHenning
BenHenning reviewed Oct 5, 2022
View reviewed changes
Copy link
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 @vrajdesai78. Just had a couple of follow-ups--PTAL.

app/src/main/java/org/oppia/android/app/activity/route/ActivityRouter.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfileProgressFragmentTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfileProgressFragmentTest.kt Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Oct 5, 2022
vrajdesai78 and others added 3 commits October 5, 2022 11:37
@vrajdesai78 @BenHenning
Update app/src/sharedTest/java/org/oppia/android/app/profileprogress/…
3c141fb 
…ProfileProgressFragmentTest.kt

Co-authored-by: Ben Henning <[email protected]>
@vrajdesai78
Revert "Update app/src/sharedTest/java/org/oppia/android/app/profilep…
19b8fa3 
…rogress/ProfileProgressFragmentTest.kt"

This reverts commit 3c141fb.
@vrajdesai78
Updated testname
13cb226
@vrajdesai78 vrajdesai78 requested a review from BenHenning October 5, 2022 06:12
BenHenning
BenHenning reviewed Oct 5, 2022
View reviewed changes
Copy link
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.

@vrajdesai78 PTAL at follow-up comment.

app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfileProgressFragmentTest.kt Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/activity/route/ActivityRouter.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Oct 5, 2022
@vrajdesai78
Copy link
Contributor Author

@BenHenning, updated ActivityRouter based on your suggestion. Thanks

@vrajdesai78 vrajdesai78 requested a review from BenHenning October 5, 2022 08:06
BenHenning
BenHenning approved these changes Oct 5, 2022
View reviewed changes
Copy link
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 @vrajdesai78. This LGTM.

@BenHenning BenHenning merged commit 162a50e into oppia:develop Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@BenHenning BenHenning

@vrajdesai78 vrajdesai78

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@vrajdesai78 @BenHenning @rt4914