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
Fix part of #4865 and Fix #4986 : Move all fragment arguments, activity intent extras, and saved instance state over to protos #5248
base: develop
Are you sure you want to change the base?
Conversation
...ia/android/app/devoptions/markchapterscompleted/testing/MarkChaptersCompletedTestActivity.kt
Outdated
Show resolved
Hide resolved
@adhiamboperes, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vishwajith-Shettigar, there is one unresolved comment. PTAL.
Unassigning @adhiamboperes since the review is done. |
Hi @Vishwajith-Shettigar, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
@adhiamboperes, I have addressed all of your comments, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Vishwajith-Shettigar, this LGTM!
Assigning @BenHenning for code owner reviews. Thanks! |
Hi @Vishwajith-Shettigar, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delayed review.
Thanks @Vishwajith-Shettigar! Had a few high-level comments before taking a deeper pass.
} | ||
|
||
// Represents the parameters needed to create MarkChaptersCompletedTestActivity. | ||
message MarkChaptersCompletedTestActivityParams{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure there are spaces before opening curly braces here & elsewhere in this proto.
|
||
// Arguments required when creating a StateFragment. | ||
message StateFragmentArguments { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit here & above & elsewhere: please remove extra starting newline for formatting consistency.
// Arguments required when creating a MarkChaptersCompletedFragment. | ||
message MarkChaptersCompletedFragmentArguments{ | ||
// The ID of the profile that wants to mark chapters as completed. | ||
int32 internal_profile_id = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here & elsewhere in this proto: could we perhaps use ProfileId instead? That's the structure meant for passing around a profile ID rather than exposing internal_profile_id directly.
"CurrentUserProfileIdIntentDecorator.profile_id_bundle_key" | ||
|
||
/** Decorator that allows an activity to wrap a user's [ProfileId] within its intent. */ | ||
object CurrentUserProfileIdIntentDecorator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my comment in the proto file, is this actually needed if we're including ProfileId directly in the params & arguments used to create activities & fragments?
Explanation
Fixes #4986
Move all fragment arguments, activity extras, and saved instance state bundle usage over to using protos entirely.
Fix part of #4865
Introduce a ProfileID decorator to pack ProfileId into all Intents so that it can be easily accessed by underlying fragments and child classes. Some classes already use this utility where it was not easy to add profileId to the intent proto bundle.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: