Fix #1762: NavigationDrawer cancel switch profile always mark home #1816
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,16 +17,26 @@ 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" | ||
|
|
||
| /** | ||
| * This function is responsible for displaying content in DialogFragment. | ||
| * | ||
| * @return [ExitProfileDialogFragment]: DialogFragment | ||
| */ | ||
| fun newInstance( | ||
| isFromNavigationDrawer: Boolean, | ||
| isAdministratorControlsSelected: Boolean, | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. @BenHenning I think they both can be simultaneously true, the first one is true when we open Navigation Drawer and click
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 | ||
| } | ||
|
|
@@ -43,6 +53,15 @@ class ExitProfileDialogFragment : DialogFragment() { | |
| false | ||
| ) | ||
|
|
||
| val isAdminSelected = args.getBoolean( | ||
|
There was a problem hiding this comment. we can use |
||
| BOOL_IS_ADMINISTRATOR_CONTROLS_SELECTED_KEY, | ||
| false | ||
| ) | ||
|
|
||
| val lastCheckedItemId = args.getInt( | ||
| INT_LAST_CHECKED_ITEM_KEY | ||
| ) | ||
|
There was a problem hiding this comment. Convert this to single line: |
||
|
|
||
| if (isFromNavigationDrawer) { | ||
| exitProfileDialogInterface = | ||
| parentFragment as ExitProfileDialogInterface | ||
|
|
@@ -53,7 +72,11 @@ class ExitProfileDialogFragment : DialogFragment() { | |
| .setMessage(R.string.home_activity_back_dialog_message) | ||
| .setNegativeButton(R.string.home_activity_back_dialog_cancel) { dialog, _ -> | ||
| if (isFromNavigationDrawer) { | ||
prayutsu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| exitProfileDialogInterface.markLastCheckedItemCloseDrawer( | ||
rt4914 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| lastCheckedItemId, | ||
| isAdminSelected | ||
| ) | ||
| exitProfileDialogInterface.unmarkSwitchProfileItemCloseDrawer() | ||
| } | ||
| dialog.dismiss() | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,11 @@ class HomeActivity : InjectableAppCompatActivity(), RouteToTopicListener { | |
| supportFragmentManager.beginTransaction().remove(previousFragment).commitNow() | ||
| } | ||
| val dialogFragment = ExitProfileDialogFragment | ||
| .newInstance( | ||
| isFromNavigationDrawer = false, | ||
| isAdministratorControlsSelected = false, | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. This is really a good point, rather than storing There was a problem hiding this comment. Yes, that sounds much cleaner @prayutsu. Good name. |
||
| lastCheckedItemId = -1 | ||
| ) | ||
| dialogFragment.showNow(supportFragmentManager, TAG_SWITCH_PROFILE_DIALOG) | ||
| } | ||
| } | ||
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: move to a single line if it fits (per 100 character limit).
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.
Done