Fix #1587 : Made intent/bundle/saved/tags instance keys consistent #1717

The-Pascal · 2020-08-26T16:12:24Z

Explanation

Fix #1587 : Made intent/bundle/saved/tags instance keys consistent
For fragments:
<PROPERTY_NAME>_ARGUMENT_KEY = ".<property_name>"
Example: INTERNAL_PROFILE_ID_ARGUMENT_KEY = "SampleFragment.internal_profile_id"

For activities:
<PROPERTY_NAME>_EXTRA_KEY = ".<property_name>"
Example: INTERNAL_PROFILE_ID_EXTRA_KEY = "SampleActivity.internal_profile_id"

For saved instances:
<PROPERTY_NAME>_SAVED_KEY = ".<property_name>"
Example: INTERNAL_PROFILE_ID_SAVED_KEY = "SampleActivity.internal_profile_id"

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.

anandwana001 · 2020-08-27T06:27:50Z

@The-Pascal You can have a look at this wiki page, this will help you out in passing the ktlint check.
You can also run the ktlint on your machine before commit, just to ensure this will not fail here at GitHub action checks.

anandwana001 · 2020-08-27T06:44:55Z

🎉 Congratulations on your first PR 🎉

Few general points:

You will be able to access the Assignees and Reviewers section once your two PRs gets merged. By the time you can mention me or @rt4914 in your PR description so that we can review your PR.
In future make sure that your branch name is of lower case characters separated by underscore like made_intent_keys_consistent
Commit message should be something which helps in understanding what that particular commit does
In the PR description, if you think any point is missing or undone you can leave it unmarked and mention the cause in the PR description which helps us to resolve. Also, currently, there is a space between x which a markdown failure. You can follow [x] ->

The PR is made from a branch that's not called "develop".

anandwana001

Few nit changes.

@rt4914 What about key's like

const val LAST_LOADED_FRAGMENT_KEY = "LAST_LOADED_FRAGMENT_KEY"
    internal const val KEY_APP_LANGUAGE_PREFERENCE_TITLE = "APP_LANGUAGE_PREFERENCE"
    internal const val KEY_APP_LANGUAGE_PREFERENCE_SUMMARY_VALUE =
      "APP_LANGUAGE_PREFERENCE_SUMMARY_VALUE"
    internal const val KEY_SELECTED_LANGUAGE = "SELECTED_LANGUAGE"
private const val KEY_APP_LANGUAGE_PREFERENCE_TITLE = "APP_LANGUAGE_PREFERENCE"
private const val KEY_APP_LANGUAGE_PREFERENCE_SUMMARY_VALUE =
  "APP_LANGUAGE_PREFERENCE_SUMMARY_VALUE"
private const val KEY_SELECTED_LANGUAGE = "SELECTED_LANGUAGE"

and others like the same

anandwana001 · 2020-08-27T12:42:05Z

app/src/main/java/org/oppia/app/testing/TestFontScaleConfigurationUtilActivity.kt

@@ -20,7 +20,8 @@ class TestFontScaleConfigurationUtilActivity : InjectableAppCompatActivity() {
 }

 companion object {
- private const val FONT_SCALE_EXTRA_KEY = "FONT_SCALE_EXTRA_KEY"
+ private const val FONT_SCALE_EXTRA_KEY = 


No single space needed after =

anandwana001 · 2020-08-27T12:43:20Z

app/src/main/java/org/oppia/app/topic/revisioncard/RevisionCardActivity.kt

- internal const val SUBTOPIC_ID_EXTRA_KEY = "SUBTOPIC_ID_EXTRA_KEY"
+ internal const val INTERNAL_PROFILE_ID_EXTRA_KEY =
+ "RevisionCardActivity.internal_profile_id"
+ internal const val TOPIC_ID_EXTRA_KEY = 


Same here, no single space needed after =

anandwana001 · 2020-08-27T12:43:29Z

app/src/main/java/org/oppia/app/topic/revisioncard/RevisionCardActivity.kt

+ "RevisionCardActivity.internal_profile_id"
+ internal const val TOPIC_ID_EXTRA_KEY = 
+ "RevisionCardActivity.topic_id"
+ internal const val SUBTOPIC_ID_EXTRA_KEY = 


Same here, no single space needed after =

anandwana001 · 2020-08-27T12:55:14Z

app/src/main/java/org/oppia/app/hintsandsolution/HintsAndSolutionDialogFragment.kt

+private const val SOLUTION_INDEX_SAVED_KEY =
+ "HintsAndSolutionDialogFragment.solution_index"
+private const val IS_SOLUTION_REVEALED_SAVED_KEY =
+ "HintsAndSolutionDialogFragment.is_solution_revealed"

 /* Fragment that displays a fullscreen dialog for Hints and Solutions. */
 class HintsAndSolutionDialogFragment :


Below key is left untouched

internal const val ID_ARGUMENT_KEY = "ID" internal const val NEW_AVAILABLE_HINT_INDEX_ARGUMENT_KEY = "NEW_AVAILABLE_HINT_INDEX" internal const val ALL_HINTS_EXHAUSTED_ARGUMENT_KEY = "ALL_HINTS_EXHAUSTED"

anandwana001 · 2020-08-27T12:56:27Z

app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt

+const val STATE_FRAGMENT_PROFILE_ID_ARGUMENT_KEY = "StateFragmentPresenter.profile_id"
+const val STATE_FRAGMENT_TOPIC_ID_ARGUMENT_KEY = "StateFragmentPresenter.topic_id"
+const val STATE_FRAGMENT_STORY_ID_ARGUMENT_KEY = "StateFragmentPresenter.story_id"
+const val STATE_FRAGMENT_EXPLORATION_ID_ARGUMENT_KEY = "StateFragmentPresenter.exploration_id"
 private const val TAG_AUDIO_FRAGMENT = "AUDIO_FRAGMENT"


TAG should be changed too!!
as per the pr title says about Tags

@rt4914 please confirm here, as tags are not mentioned in the description.

Tags related to fragment / activities should be mentioned in their respective companion objects.

Example: internal const val TAG = "AudioFragment.tag"

rt4914 · 2020-08-27T14:24:02Z

const val LAST_LOADED_FRAGMENT_KEY = "LAST_LOADED_FRAGMENT_KEY"

@anandwana001 @The-Pascal

Because the LAST_LOADED_FRAGMENT_KEY is actually getting saved/fetched in onSaveInstance, we should name them like this

LAST_LOADED_FRAGMENT_SAVED_KEY = "AdministratorControlsActivity.last_loaded_fragment"

This should be inside a companion object.

rt4914

@The-Pascal
Please take a look at comments by @anandwana001 . Also, make sure that you check corresponding test cases files too because most of these keys are used in test cases too especially during intent.

Thanks.

The-Pascal · 2020-08-27T15:34:41Z

@anandwana001 Thank you so much for your advice. I will always remember that and hope to go a long way contributing in open source community.

@rt4914 Thank you for providing me the opportunity.

I have made changes considering all the previous comments. Please review these changes.

rt4914

@The-Pascal I have left some comments but those comments apply to your entire PR.

You should follow these rules:

TAG, EXTRA, ARGUMENT, SAVED_KEY all these types of keys should be mentioned in the activity/fragment's companion object.
If you are mentioning them in companion object make sure that they are internal, like : internal const val TAG = "HomeFragment.tag"
For tags you can directly write TAG instead of HOME_FRAGMENT_TAG.
Make sure you follow other rules too which are mentioned in the issue itself.

Also, please make sure that you reply to comments also, like "DONE", "Mention what change you have done", etc.

Thanks.

rt4914 · 2020-08-28T11:17:26Z

app/src/main/java/org/oppia/app/administratorcontrols/AdministratorControlsActivity.kt

@@ -12,7 +12,8 @@ import org.oppia.app.settings.profile.ProfileListActivity
 import javax.inject.Inject

 const val SELECTED_CONTROLS_TITLE_KEY = "SELECTED_CONTROLS_TITLE_KEY"


This need to be changed because this one is also getting used in savedInstanceState, check line 34 in code.

rt4914 · 2020-08-28T11:19:50Z

app/src/main/java/org/oppia/app/administratorcontrols/AdministratorControlsActivity.kt

@@ -12,7 +12,8 @@ import org.oppia.app.settings.profile.ProfileListActivity
 import javax.inject.Inject

 const val SELECTED_CONTROLS_TITLE_KEY = "SELECTED_CONTROLS_TITLE_KEY"
-const val LAST_LOADED_FRAGMENT_KEY = "LAST_LOADED_FRAGMENT_KEY"
+const val LAST_LOADED_FRAGMENT_SAVED_KEY =


This string is correct but it should be mentioned in the companion object.

companion object { internal const val LAST_LOADED_FRAGMENT_SAVED_KEY = "AdministratorControlsActivity.last_loaded_fragment" fun createAdministratorControlsActivityIntent(context: Context, profileId: Int?): Intent { val intent = Intent(context, AdministratorControlsActivity::class.java) intent.putExtra(KEY_NAVIGATION_PROFILE_ID, profileId) return intent } fun getIntentKey(): String { return KEY_NAVIGATION_PROFILE_ID } }

Once you have mentioned it in companion object you can use it anywhere like this:
AdministratorControlsActivity.LAST_LOADED_FRAGMENT_SAVED_KEY

rt4914 · 2020-08-28T11:21:16Z

app/src/main/java/org/oppia/app/help/HelpActivity.kt

@@ -28,7 +28,7 @@ class HelpActivity : InjectableAppCompatActivity(), RouteToFAQListListener {
 companion object {
 // TODO(#1655): Re-restrict access to fields in tests post-Gradle.
 const val BOOL_IS_FROM_NAVIGATION_DRAWER_EXTRA_KEY =
- "BOOL_IS_FROM_NAVIGATION_DRAWER_EXTRA_KEY"


Rename this to

const val IS_FROM_NAVIGATION_DRAWER_EXTRA_KEY = "HelpActivity.is_from_navigation_drawer"

else everything is correct.

rt4914 · 2020-08-28T11:22:41Z

app/src/main/java/org/oppia/app/hintsandsolution/HintsAndSolutionDialogFragment.kt

+ "HintsAndSolutionDialogFragment.is_hint_revealed"
+private const val SOLUTION_INDEX_SAVED_KEY =
+ "HintsAndSolutionDialogFragment.solution_index"
+private const val IS_SOLUTION_REVEALED_SAVED_KEY =


Shift all these private keys to companion object, similar to what I mentioned for AdministratorControlsActivity.

done
But I have used private keys only inside companion object also, so please check if I need to make those keys internal or not.

rt4914 · 2020-08-28T11:22:55Z

app/src/main/java/org/oppia/app/home/HomeActivityPresenter.kt

@@ -9,7 +9,7 @@ import org.oppia.app.activity.ActivityScope
 import org.oppia.app.drawer.NavigationDrawerFragment
 import javax.inject.Inject

-const val TAG_HOME_FRAGMENT = "HOME_FRAGMENT"
+const val TAG_HOME_FRAGMENT = "HomeFragment.tag"


Shift to companion object on HomeFragment.

Also please check this comment:
#1717 (comment)

As per this comment it should be named like this:
internal const val TAG = "HomeFragment.tag"

done
But am not sure the way I did it. So please check it and guide me if am wrong in this.

rt4914 · 2020-08-28T11:25:52Z

app/src/main/java/org/oppia/app/home/recentlyplayed/RecentlyPlayedFragment.kt

@@ -15,7 +15,7 @@ private const val RECENTLY_PLAYED_FRAGMENT_INTERNAL_PROFILE_ID_KEY =
 /** Fragment that contains all recently played stories. */
 class RecentlyPlayedFragment : InjectableFragment(), OngoingStoryClickListener {
 companion object {
- const val TAG_RECENTLY_PLAYED_FRAGMENT = "TAG_RECENTLY_PLAYED_FRAGMENT"
+ const val TAG_RECENTLY_PLAYED_FRAGMENT = "RecentlyPlayedFragment.tag"


Please take a look at this comment:
#1717 (comment)

As per this comment, you should rename like this:
internal const val TAG = "RecentlyPlayedFragment.tag

rt4914 · 2020-08-28T11:26:09Z

app/src/main/java/org/oppia/app/ongoingtopiclist/OngoingTopicListFragment.kt

@@ -13,7 +13,7 @@ class OngoingTopicListFragment : InjectableFragment() {

 companion object {
 // TODO(#1655): Re-restrict access to fields in tests post-Gradle.
- const val ONGOING_TOPIC_LIST_FRAGMENT_TAG = "TAG_ONGOING_TOPIC_LIST_FRAGMENT"
+ const val ONGOING_TOPIC_LIST_FRAGMENT_TAG = "OngoingTopicListFragment.tag"


Please take a look at this comment:
#1717 (comment)

anandwana001 · 2020-09-01T05:01:07Z

@The-Pascal Any update on this?

The-Pascal · 2020-09-02T05:16:32Z

I have committed changes according to the suggestions. Please have a look.

@rt4914 and @anandwana001 I really appreciate your time and support for guiding me.

anandwana001 · 2020-09-02T12:06:46Z

I have committed changes according to the suggestions. Please have a look.

@rt4914 and @anandwana001 I really appreciate your time and support for guiding me.

You can comment on requested changes comment which helps me to understand things are solved and then I can resolve those comments.
You can say something like solved or done in comments.

The-Pascal · 2020-09-02T15:22:41Z

You can comment on requested changes comment which helps me to understand things are solved and then I can resolve those comments.
You can say something like solved or done in comments.

Thanks @anandwana001 I have done the same.

anandwana001

Looks good so far @The-Pascal
I found a couple of arguments like

private const val IS_MULTIPANE_KEY = "IS_MULTIPANE_KEY" in AdministratorControlsFragment,

const val KEY_NAVIGATION_PROFILE_ID = "KEY_NAVIGATION_PROFILE_ID" in NavigationDrawerFragmentPresenter,

intent.putExtra(KEY_NAVIGATION_PROFILE_ID, profileId) in HelpActivity (this can be same as
intent.putExtra(COMPLETED_STORY_LIST_ACTIVITY_PROFILE_ID_KEY, internalProfileId)) and for all others internalProfileId,

intent.putExtra(KEY_NAVIGATION_PROFILE_ID, profileId) in HomeActivity, MyDownloadsActivity, OptionsActivity, TopicActivity and other places too.

To keep the track and make this work easier, I suggest creating a list of packages in the PR descriptions and make changes.
Let me know for any other assistance.

anandwana001 · 2020-09-25T02:43:40Z

@The-Pascal You need to update the branch with develop before working more on this PR as we had updated the package names.

Fix oppia#1587 : Made intent/bundle/saved/tags instance keys consistent

7206d49

anandwana001 self-requested a review August 27, 2020 12:14

anandwana001 self-assigned this Aug 27, 2020

anandwana001 suggested changes Aug 27, 2020

View reviewed changes

anandwana001 assigned The-Pascal and unassigned anandwana001 Aug 27, 2020

rt4914 suggested changes Aug 27, 2020

View reviewed changes

The-Pascal added 3 commits August 27, 2020 20:01

Fix oppia#1587 : Resolve test cases error. Made intent keys consistent.

ad6738d

Fix oppia#1587 : Resolve test cases error. Made intent keys consistent.

e488310

Fix oppia#1587 : Tags related to fragment / activities made consistent.

0864cae

rt4914 assigned rt4914 and anandwana001 and unassigned The-Pascal Aug 28, 2020

rt4914 suggested changes Aug 28, 2020

View reviewed changes

rt4914 assigned The-Pascal and unassigned rt4914 and anandwana001 Aug 28, 2020

The-Pascal added 4 commits September 2, 2020 09:46

Fix oppia#1587 : Renamed and added TAGS in companion object.

866c292

Fix oppia#1587 : Renamed and added TAGS in companion object.

4e561f5

Fix oppia#1587 : Renamed and added TAGS in companion object.

b3969d7

Fix oppia#1587 : Renamed and added TAGS in companion object.

08be64d

The-Pascal requested review from rt4914 and anandwana001 September 2, 2020 05:22

anandwana001 suggested changes Sep 3, 2020

View reviewed changes

The-Pascal closed this Oct 3, 2020

The-Pascal removed their assignment Oct 3, 2020

		@@ -12,7 +12,8 @@ import org.oppia.app.settings.profile.ProfileListActivity
		import javax.inject.Inject

		const val SELECTED_CONTROLS_TITLE_KEY = "SELECTED_CONTROLS_TITLE_KEY"

Fix #1587 : Made intent/bundle/saved/tags instance keys consistent #1717

Fix #1587 : Made intent/bundle/saved/tags instance keys consistent #1717

Conversation

The-Pascal commented Aug 26, 2020 • edited Loading

Explanation

Checklist

anandwana001 commented Aug 27, 2020

anandwana001 commented Aug 27, 2020

anandwana001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 commented Aug 27, 2020

rt4914 left a comment

Choose a reason for hiding this comment

The-Pascal commented Aug 27, 2020

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandwana001 commented Sep 1, 2020

The-Pascal commented Sep 2, 2020

anandwana001 commented Sep 2, 2020

The-Pascal commented Sep 2, 2020

anandwana001 left a comment • edited Loading

Choose a reason for hiding this comment

anandwana001 commented Sep 25, 2020

The-Pascal commented Aug 26, 2020 •

edited

Loading

anandwana001 left a comment •

edited

Loading