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

Fix #1762: NavigationDrawer cancel switch profile always mark home #1816

Closed
wants to merge 57 commits into from
Closed

Fix #1762: NavigationDrawer cancel switch profile always mark home #1816

Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
d415788
fixed major bugs
prayutsu Sep 5, 2020
519c36a
fixed marking correct last item in navigation drawer
prayutsu Sep 5, 2020
803ce83
fixed nits
prayutsu Sep 5, 2020
e3f336e
fixed lint errors
prayutsu Sep 5, 2020
1931f22
test without any marking
prayutsu Sep 6, 2020
c56d9c4
fixed almost all the bugs
prayutsu Sep 19, 2020
5098c10
tried fixing last bug
prayutsu Sep 21, 2020
0799f75
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Sep 26, 2020
245014b
fixed all the bugs
prayutsu Sep 27, 2020
80a5a8f
removed extra code
prayutsu Sep 28, 2020
f9a8600
Nit change
prayutsu Sep 28, 2020
2f9d2b7
test for reason of admin controls bug
prayutsu Sep 28, 2020
d271dfe
Optimized code
prayutsu Sep 28, 2020
99215f8
fixed lint error
prayutsu Sep 28, 2020
cf488c5
fixed lint error
prayutsu Sep 28, 2020
f5d2fcb
fixed reviewed changes
prayutsu Oct 9, 2020
b071e2b
fixed ktlint failures
prayutsu Oct 9, 2020
56d5250
fixed suggested changes
prayutsu Oct 9, 2020
78e2d62
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Dec 20, 2020
7eccac7
add few test cases
prayutsu Dec 24, 2020
8e4fdc0
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Dec 24, 2020
0ef2f5a
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 6, 2021
bfe6192
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 11, 2021
5d4f8ee
Add more test cases
prayutsu Jan 11, 2021
6639420
Fix imports
prayutsu Jan 11, 2021
0c8b43d
Fix Administrator Controls test cases
prayutsu Jan 12, 2021
007407d
Uncomment All other test cases
prayutsu Jan 12, 2021
05f48f0
Make a function to uncheck all items when Administrator Controls is c…
prayutsu Jan 13, 2021
a1777e5
Fix lint
prayutsu Jan 13, 2021
f9f086c
Fix lint
prayutsu Jan 13, 2021
6c477fc
Improve code
prayutsu Jan 15, 2021
0733811
Replace .isCheckable with .isChecked
prayutsu Jan 16, 2021
72a3fe2
Improve quality of the code by using function
prayutsu Jan 19, 2021
544c814
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 19, 2021
b23869f
Add test cases related to color for all menu items
prayutsu Jan 19, 2021
b03b57e
Restore deleted line
prayutsu Jan 19, 2021
38c09c8
Add @RunOn(TestPlatform.ESPRESSO) notation to added test cases
prayutsu Jan 22, 2021
52ae75d
Fix lint errors
prayutsu Jan 22, 2021
de003c6
Fix suggested changes
prayutsu Jan 22, 2021
08f59d3
Fix lint errors
prayutsu Jan 22, 2021
6fba236
Fix the names of the new test cases
prayutsu Jan 22, 2021
cc45211
Fix suggested changes
prayutsu Jan 22, 2021
72d243c
Fix nits
prayutsu Jan 26, 2021
3774af2
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 26, 2021
4c7f798
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 28, 2021
08deb4b
Correct dialog box dependency
prayutsu Jan 29, 2021
91d75b1
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Feb 2, 2021
6ffd6ed
Replace different keys with a single proto object.
prayutsu Feb 3, 2021
caf0f48
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Feb 3, 2021
93828e2
Fix lint and bazel build errors.
prayutsu Feb 3, 2021
4ff38db
Add ":arguments_java_proto_lite" in the android_library in BUILD.bazel
prayutsu Feb 3, 2021
d65710b
Improve comments' explanation and make the variable names consistent.
prayutsu Feb 3, 2021
381509d
Try implementing suggested changes.
prayutsu Feb 5, 2021
8ec8a2d
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Feb 5, 2021
d9a4608
Fix lint errors.
prayutsu Feb 5, 2021
fe5191f
Make use of enum defined in arguments.proto
prayutsu Feb 5, 2021
b29f8e5
Fix protobuf error.
prayutsu Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions app/src/main/java/org/oppia/android/app/drawer/ExitProfileDialogFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,32 @@ class ExitProfileDialogFragment : DialogFragment() {
// 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"
const val BOOL_IS_ADMINISTRATOR_CONTROLS_SELECTED_KEY =
"BOOL_IS_ADMINISTRATOR_CONTROLS_SELECTED_KEY"
const val INT_LAST_CHECKED_ITEM_KEY =
"INT_LAST_CHECKED_ITEM_KEY"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: move to a single line if it fits (per 100 character limit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/**
* This function is responsible for displaying content in DialogFragment.
*
* @return [ExitProfileDialogFragment]: DialogFragment
*/
fun newInstance(isFromNavigationDrawer: Boolean): ExitProfileDialogFragment {
fun newInstance(
isFromNavigationDrawer: Boolean,
isAdministratorControlsSelected: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming these both can't be true, so given that they're mutually exclusive I think we should model the API (both the function & the argument keys) to be based on an enum rather than booleans.

This might actually be a good opportunity to start implementing support for protos stored in bundles. I'll try to get a PR out later today that introduces a utility to do this easily. The idea would be to introduce a proto file + proto that represents the intent arguments for this fragment and then just populate a new object & save/retrieve it from the bundle. Will try to make the API work well for both intents & arguments.

This will be establishing a new pattern, but I think it's long-term the better way to go since it provides us with better flexibility & API design for fragments & activities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BenHenning I think they both can be simultaneously true, the first one is true when we open Navigation Drawer and click Switch Profile (isFromNavigationDrawer = true) since it denotes that the dialogFragment created is initialized from the Navigation Drawer or not; in our case, it will always be true.
The second denotes that Administrator Controls is checked or not, and it may be true or false independent of isFromNavigationDrawer.
So, we might not be able to use enum here.

This might actually be a good opportunity to start implementing support for protos stored in bundles. I'll try to get a PR out later today that introduces a utility to do this easily. The idea would be to introduce a proto file + proto that represents the intent arguments for this fragment and then just populate a new object & save/retrieve it from the bundle. Will try to make the API work well for both intents & arguments.

This will be establishing a new pattern, but I think it's long-term the better way to go since it provides us with better flexibility & API design for fragments & activities.

If it is a better way to design the API for intents and arguments, I'll definitely try to implement it, so will wait till you make a PR for this.

lastCheckedItemId: Int
): ExitProfileDialogFragment {
val exitProfileDialogFragment = ExitProfileDialogFragment()
val args = Bundle()
args.putBoolean(BOOL_IS_FROM_NAVIGATION_DRAWER_EXTRA_KEY, isFromNavigationDrawer)
args.putBoolean(BOOL_IS_ADMINISTRATOR_CONTROLS_SELECTED_KEY, isAdministratorControlsSelected)
args.putInt(INT_LAST_CHECKED_ITEM_KEY, lastCheckedItemId)
exitProfileDialogFragment.arguments = args
return exitProfileDialogFragment
}
}

lateinit var exitProfileDialogInterface: ExitProfileDialogInterface
private lateinit var exitProfileDialogInterface: ExitProfileDialogInterface

override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
val args =
Expand All @@ -43,6 +53,13 @@ class ExitProfileDialogFragment : DialogFragment() {
false
)

val isAdministratorControlsSelected = args.getBoolean(
BOOL_IS_ADMINISTRATOR_CONTROLS_SELECTED_KEY,
false
)

val lastCheckedItemId = args.getInt(INT_LAST_CHECKED_ITEM_KEY)

if (isFromNavigationDrawer) {
exitProfileDialogInterface =
parentFragment as ExitProfileDialogInterface
Expand All @@ -53,7 +70,11 @@ class ExitProfileDialogFragment : DialogFragment() {
.setMessage(R.string.home_activity_back_dialog_message)
.setNegativeButton(R.string.home_activity_back_dialog_cancel) { dialog, _ ->
if (isFromNavigationDrawer) {
exitProfileDialogInterface.markHomeMenuCloseDrawer()
exitProfileDialogInterface.markLastCheckedItemCloseDrawer(
lastCheckedItemId,
isAdministratorControlsSelected
)
exitProfileDialogInterface.unmarkSwitchProfileItemCloseDrawer()
}
dialog.dismiss()
}
Expand Down
7 changes: 6 additions & 1 deletion app/src/main/java/org/oppia/android/app/drawer/ExitProfileDialogInterface.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,10 @@ package org.oppia.android.app.drawer

/** Interface to handle option selection in [ExitProfileDialogFragment]. */
interface ExitProfileDialogInterface {
fun markHomeMenuCloseDrawer()
fun markLastCheckedItemCloseDrawer(
lastCheckedItemId: Int,
isAdministratorControlsSelected: Boolean
)

fun unmarkSwitchProfileItemCloseDrawer()
}
14 changes: 12 additions & 2 deletions app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,17 @@ class NavigationDrawerFragment :
navigationDrawerFragmentPresenter.openProfileProgress(profileId)
}

override fun markHomeMenuCloseDrawer() {
navigationDrawerFragmentPresenter.markHomeMenuCloseDrawer()
override fun markLastCheckedItemCloseDrawer(
lastCheckedItemId: Int,
isAdministratorControlsSelected: Boolean
) {
navigationDrawerFragmentPresenter.checkLastCheckedItemCloseDrawer(
lastCheckedItemId,
isAdministratorControlsSelected
)
}

override fun unmarkSwitchProfileItemCloseDrawer() {
navigationDrawerFragmentPresenter.uncheckSwitchProfileItemCloseDrawer()
}
}
62 changes: 53 additions & 9 deletions app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,7 @@ class NavigationDrawerFragmentPresenter @Inject constructor(
return@setOnClickListener
}

binding.fragmentDrawerNavView.menu.forEach { menuItem ->
menuItem.isCheckable = false
}
uncheckAllMenuItemsWhenAdministratorControlsIsChecked()

drawerLayout.closeDrawers()
getFooterViewModel().isAdministratorControlsSelected.set(true)
Expand Down Expand Up @@ -199,7 +197,6 @@ class NavigationDrawerFragmentPresenter @Inject constructor(
}

private fun openActivityByMenuItemId(menuItemId: Int) {
getFooterViewModel().isAdministratorControlsSelected.set(false)
if (previousMenuItemId != menuItemId) {
when (NavigationDrawerItem.valueFromNavId(menuItemId)) {
NavigationDrawerItem.HOME -> {
Expand Down Expand Up @@ -244,8 +241,25 @@ class NavigationDrawerFragmentPresenter @Inject constructor(
if (previousFragment != null) {
fragment.childFragmentManager.beginTransaction().remove(previousFragment).commitNow()
}
val lastCheckedItemId: Int
val isAdministratorControlsSelected: Boolean
if (getFooterViewModel().isAdministratorControlsSelected.get() == true) {
getFooterViewModel().isAdministratorControlsSelected.set(false)
isAdministratorControlsSelected = true
lastCheckedItemId = -1
} else {
isAdministratorControlsSelected = false
lastCheckedItemId = previousMenuItemId ?: -1
}
binding.fragmentDrawerNavView.menu.getItem(
NavigationDrawerItem.SWITCH_PROFILE.ordinal
).isChecked = true
val dialogFragment = ExitProfileDialogFragment
.newInstance(isFromNavigationDrawer = true)
.newInstance(
isFromNavigationDrawer = true,
isAdministratorControlsSelected = isAdministratorControlsSelected,
lastCheckedItemId = lastCheckedItemId
)
dialogFragment.showNow(fragment.childFragmentManager, TAG_SWITCH_PROFILE_DIALOG)
}
}
Expand All @@ -263,12 +277,41 @@ class NavigationDrawerFragmentPresenter @Inject constructor(
)
}

fun markHomeMenuCloseDrawer() {
fun checkLastCheckedItemCloseDrawer(
lastCheckedItemId: Int,
isAdministratorControlsSelected: Boolean
) {
if (isAdministratorControlsSelected) {
getFooterViewModel().isAdministratorControlsSelected.set(true)
uncheckAllMenuItemsWhenAdministratorControlsIsChecked()
} else if (lastCheckedItemId != -1) {
getFooterViewModel().isAdministratorControlsSelected.set(false)
val checkedItemPosition =
when (lastCheckedItemId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

keep when outside of getItem() and use a var to input in getItem(), this way it will be more clean and readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

NavigationDrawerItem.HOME.value -> 0
NavigationDrawerItem.OPTIONS.value -> 1
NavigationDrawerItem.HELP.value -> 2
NavigationDrawerItem.DOWNLOADS.value -> 3
else -> 4
}
if (checkedItemPosition != 4) {
binding.fragmentDrawerNavView.menu.getItem(checkedItemPosition).isChecked = true
}
}
drawerLayout.closeDrawers()
}

fun uncheckSwitchProfileItemCloseDrawer() {
binding.fragmentDrawerNavView.menu.getItem(
NavigationDrawerItem.HOME.ordinal
NavigationDrawerItem.SWITCH_PROFILE.ordinal
).isChecked =
true
drawerLayout.closeDrawers()
false
}

private fun uncheckAllMenuItemsWhenAdministratorControlsIsChecked() {
binding.fragmentDrawerNavView.menu.forEach { item ->
item.isCheckable = false
}
}

/**
Expand Down Expand Up @@ -345,6 +388,7 @@ class NavigationDrawerFragmentPresenter @Inject constructor(
} else {
// For showing navigation drawer in AdministratorControlsActivity
getFooterViewModel().isAdministratorControlsSelected.set(true)
uncheckAllMenuItemsWhenAdministratorControlsIsChecked()
this.drawerLayout = drawerLayout
drawerToggle = object : ActionBarDrawerToggle(
fragment.activity,
Expand Down
6 changes: 5 additions & 1 deletion app/src/main/java/org/oppia/android/app/home/HomeActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ class HomeActivity :
supportFragmentManager.beginTransaction().remove(previousFragment).commitNow()
}
val dialogFragment = ExitProfileDialogFragment
.newInstance(isFromNavigationDrawer = false)
.newInstance(
isFromNavigationDrawer = false,
isAdministratorControlsSelected = false,
Copy link
Member

Choose a reason for hiding this comment

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

Rather than storing the "from" might it be cleaner for us to represent the 'behavior' that we desire from ExitProfileDialogFragment? Specifically, I think it's a dependency inversion for something like ExitProfileDialogFragment to specialize behavior based on certain origins. It may be the case that certain origins should drive certain reset behavior (e.g. "set selected tab to X" or "reset to current selection" which is a generic concept rather than tying to specific flows). Does such a general case exist here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really a good point, rather than storing isFromNavigationDrawer we could store something like restoreLastCheckedItem is true or not.
@BenHenning Can you please confirm if you meant the same?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that sounds much cleaner @prayutsu. Good name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lastCheckedItemId = -1
)
dialogFragment.showNow(supportFragmentManager, TAG_SWITCH_PROFILE_DIALOG)
}

Expand Down
Loading