-
Notifications
You must be signed in to change notification settings - Fork 499
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.
model/src/main/proto/arguments.proto
Outdated
} | ||
|
||
// 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.
model/src/main/proto/arguments.proto
Outdated
|
||
// 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.
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.
If i directly include ProfileId in proto, there could be many changes in code, like in activity creation methods, we passed internalProfileId(Int type) as an argument,
We have two options
- Either i create ProfileId using internalProfileId inside method (and pass it to proto and wrap it in bundle).
- or i change all the activity or fragment creation method to pass ProfileId instead of internalProfileId(Int type) to avoid option 1.
But here this issue #4865 already filed, which will remove all ProfileId from proto in future and use CurrentUserProfileIdIntentDecorator to pass ProfileId, that's the reason i kept internalProfileId in proto.
Can you please tell me whether I should keep it as is or replace internalProfileId (int type) with ProfileId type?
"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?
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.
Yes. The main reason for introducing CurrentUserProfileIdIntentDecorator in this PR is to support NavigationDrawerFragmentPresenter receive profileId from HomeActivity, HelpActivity, OptionActivity, AdministratorControlsActivity, DeveloperOptionsActivity using same key.
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. |
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: