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

prayutsu · 2020-09-05T22:23:15Z

Explanation

Fixes #1762
Earlier, always home menu item got marked in the nav menu after canceling the switch profile dialog box. This is now fixed to show the correct corresponding menu item.

Quick Updates-

All the newly added test cases are passing on Espresso -
All the newly added test cases need to be fixed for robolectric.

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.

prayutsu · 2020-09-05T22:31:06Z

@anandwana001 PTAL

anandwana001

@prayutsu we need to take a deep look into this why we are having this issue, what sort of things causing this failure and also need to write proper test cases which fails without fix and passes with the fix.

app/src/main/java/org/oppia/app/drawer/ExitProfileDialogInterface.kt

app/src/main/java/org/oppia/app/drawer/ExitProfileDialogFragment.kt

anandwana001 · 2020-09-10T17:22:01Z

@rt4914 Currently, we are facing issues on all test cases regarding NavigationDrawerTestActivityTest and this PR will require to add test cases especially for the navigation drawer.

I think we need to block this PR till we solve #1968 for NavigationDrawerTestActivityTest

prayutsu · 2020-09-10T18:31:08Z

@rt4914 Currently, we are facing issues on all test cases regarding NavigationDrawerTestActivityTest and this PR will require to add test cases especially for the navigation drawer.

I think we need to block this PR till we solve #973 for NavigationDrawerTestActivityTest

Okay, so I'll work on other pending issues then.

anandwana001 · 2020-09-19T18:37:22Z

@prayutsu By the time the test file is getting fixed, you can work on this issue other than writing test cases.
Let me know if you need any help.

prayutsu · 2020-09-19T21:15:28Z

@anandwana001 Everything seems to work fine. The only problem is in the last case (please see the video in description) that is Administrator Controls->Switch Profile->Cancel->Rotate Device.
If we don't rotate the device everything seems but just after rotating the phone, switch profile is marked.

prayutsu · 2020-09-19T21:16:14Z

@anandwana001 I have updated the video in the description of the PR, please have a look at that.

anandwana001 · 2020-09-21T05:28:07Z

@anandwana001 I have updated the video in the description of the PR, please have a look at that.

That's an issue we have to solve.
Could you please mention what should be the approaches you think we can solve this issue? We can pick a the best one from that. I will also look into this issue today and will update you.

prayutsu · 2020-09-21T05:51:29Z

@anandwana001 I have updated the video in the description of the PR, please have a look at that.

That's an issue we have to solve.
Could you please mention what should be the approaches you think we can solve this issue? We can pick a the best one from that. I will also look into this issue today and will update you.

Sure

anandwana001 · 2020-09-25T02:38:10Z

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

… navigation-drawer-switch-profile-fix

app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt

prayutsu · 2020-09-27T20:07:37Z

@anandwana001 @rt4914 Please have a look.

BenHenning

Thanks @prayutsu! I really like the new proto usage. Left some comments to see if we can leverage it to make some potential improvements that would have been harder before.

BenHenning · 2021-02-04T01:15:15Z

app/src/main/java/org/oppia/android/app/drawer/ExitProfileDialogFragment.kt

+        ExitProfileDialogArguments.newBuilder()
+          .setRestoreLastCheckedMenuItem(restoreLastCheckedMenuItem)
+          .setIsAdministratorControlsSelected(isAdministratorControlsSelected)
+          .setLastCheckedMenuItemId(lastCheckedMenuItemId).build()


Suggested change

.setLastCheckedMenuItemId(lastCheckedMenuItemId).build()

.setLastCheckedMenuItemId(lastCheckedMenuItemId)

.build()

ktlint doesn't enforce this correctly, but continuation for call chains like this should be broken up such that each operation at the same logical "level" should also be at the same syntactical "level" (meaning on its own line in this case).

BenHenning · 2021-02-04T01:16:43Z

app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt

          NavigationDrawerItem.HOME.value -> 0
          NavigationDrawerItem.OPTIONS.value -> 1
          NavigationDrawerItem.HELP.value -> 2
          NavigationDrawerItem.DOWNLOADS.value -> 3


So now that we're using a proto, could we also use an enum to represent the last checked menu item? That provides type safety vs. passing an integer around.

We may be able to share the draw item enum & the menu item enum as well (e.g. just have 1 proto enum to define both so that we can include it in argument protos).

BenHenning · 2021-02-04T01:17:42Z

model/src/main/proto/arguments.proto

+
+  // The id of the menu item that was checked before the Exit Profile Dialog appears.
+  int32 last_checked_menu_item_id = 1;
+}


Add EOF newline.

BenHenning · 2021-02-04T01:18:43Z

model/src/main/proto/arguments.proto

+  bool is_administrator_controls_selected = 2;
+
+  // The id of the menu item that was checked before the Exit Profile Dialog appears.
+  int32 last_checked_menu_item_id = 1;


Is it possible to consolidate these into a single value the represents the place from which we opened the exit profile dialog?

BenHenning · 2021-02-04T01:19:18Z

model/src/main/proto/arguments.proto

+
+// Represents the arguments associated with ExitProfileDialogFragment.
+message ExitProfileDialogArguments {
+  // The last checked menu item needs to be restored or not.


Suggested change

// The last checked menu item needs to be restored or not.

// Whether the last checked menu item needs to be restored or not.

Ditto elsewhere when documenting booleans.

BenHenning · 2021-02-04T01:19:48Z

model/src/main/proto/arguments.proto

+  // Fragment/Activity or not.
+  bool is_administrator_controls_selected = 2;
+
+  // The id of the menu item that was checked before the Exit Profile Dialog appears.


Menu item from where? Suggest being specific here.

mentioned menu item from Navigation Drawer.

BenHenning · 2021-02-04T01:20:06Z

model/src/main/proto/arguments.proto

+
+// Represents the arguments associated with ExitProfileDialogFragment.
+message ExitProfileDialogArguments {
+  // The last checked menu item needs to be restored or not.


This documentation comment overall may benefit from more context: what does 'restore' mean & what menu is being described here?

Provided more context on what does restore actually mean.

BenHenning · 2021-02-04T01:20:25Z

model/src/main/proto/arguments.proto

+option java_package = "org.oppia.android.app.model";
+option java_multiple_files = true;
+
+// Represents the arguments associated with ExitProfileDialogFragment.


Suggest instead: The fragment arguments passed to instances of ExitProfileDialogFragment.

… navigation-drawer-switch-profile-fix

prayutsu

PTAL @BenHenning, I have tried making some changes, and I need some good name suggestions as mentioned below.

prayutsu · 2021-02-05T10:39:42Z

app/src/main/java/org/oppia/android/app/drawer/Argument.kt

+package org.oppia.android.app.drawer
+
+sealed class Argument {
+  class LastCheckedMenuItem(val navigationDrawerItem: NavigationDrawerItem) : Argument()


Need suggestion for a better name.

Also, please suggest a good KDoc.

prayutsu · 2021-02-05T10:41:58Z

app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt

+                  3 -> NavigationDrawerItem.DOWNLOADS
+                  4 -> NavigationDrawerItem.SWITCH_PROFILE
+                  else -> NavigationDrawerItem.HOME
+                }


How should we handle else branch here?

prayutsu · 2021-02-05T10:48:02Z

model/src/main/proto/arguments.proto

+}
+
+enum LastCheckedMenuItem {
+  HOME_MENU_ITEM_UNSPECIFIED = 0;


Need suggestion for the documentation comment.

prayutsu · 2021-02-05T11:37:03Z

app/src/main/java/org/oppia/android/app/drawer/ExitProfileDialogFragment.kt

+      3 -> NavigationDrawerItem.DOWNLOADS
+      4 -> NavigationDrawerItem.SWITCH_PROFILE
+      else -> NavigationDrawerItem.HOME
+    }


How should we handle else branch here?

prayutsu · 2021-02-05T11:38:01Z

app/src/main/java/org/oppia/android/app/drawer/ExitProfileDialogFragment.kt

+              )
+              else -> Argument.LastCheckedMenuItem(
+                getNavigationDrawerItem(
+                  exitProfileDialogArguments.lastCheckedMenuItemValue


Should we create a constant with value - 2, so that it looks cleaner and expressive?
Or do we have a better way to handle it?

chrk2205 · 2021-02-07T16:29:54Z

@prayutsu @BenHenning @rt4914 @anandwana001
For this bug, we can check the previous marked item and mark it whenever the cancel is clicked

I tried to solve this bug,
In the markMenuCloseDrawer() we can simply mark the previously selected item by using this function
if (previousMenuItemId != null) { val k: Int = previousMenuItemId!! if (k == 0) { getFooterViewModel().isAdministratorControlsSelected.set(true) binding.fragmentDrawerNavView.menu.getItem( NavigationDrawerItem.SWITCH_PROFILE.ordinal ).isChecked = false drawerLayout.closeDrawers() } else { binding.fragmentDrawerNavView.menu.getItem( NavigationDrawerItem.valueFromNavId(k).ordinal ).isChecked = true drawerLayout.closeDrawers() } }

with this solution issue #2650 will not arise
thanks

prayutsu · 2021-02-07T20:31:22Z

@chrk2205 I think this works really well, and now I am thinking that we were passing arguments from the NavigationDrawerFragmentPresenter to the ExitProfileDialogFragment and then again passing it to NavigationDrawerFragmentPresenter, which actually doesn't make sense as we already have our information in the NavigationDrawerFragmentPresenter. We should just invoke the function and give it a more meaningful name.
However, this doesn't solve the issue completely and we'll still need to make some changes to avoid some more bugs.

Screenrecorder-2021-02-08-00-14-47-901.mp4

When we click outside the dialog box, then the dialog box disappears and Switch Profile remains checked. And now there are 2 possibilities -
- We may not want the dialog box to disappear on clicking outside, then this can be easily handled.
- If we want the dialog box to disappear on clicking outside the dialog box, then we may want to trigger the same behavior as the cancel button.
  Note that this issue is present in the current implementation also.
If we go to Administrator Controls, the open NavDrawer, then click on Switch Profile and then change the configuration, then Administrator Controls also get checked. If we click the cancel button, then also it remains checked.
- We'll need to figure out a good strategy to do this, as this can be a little tricky.
  Note that this issue is not present in the current implementation.
We also want to make sure that whenever we isAdministratorControlsSelected is set to true, we should make all the other menuItems uncheckable.
We'll also be required to give the variables good names.

@BenHenning WDYT?

chrk2205 · 2021-02-08T09:48:28Z

@prayutsu
for Bug 1
you can use setCancelable=false to stop the alert dialog from closing on the background click
If this doesn't work the alternative is to use the onDestroy function to mark the previous item instead of onNegativButtonClicked

prayutsu · 2021-02-08T12:43:53Z

@chrk2205 I guess we'll have to use setCanceledOnTouchOutside(false), I guess this is not hard, but the second bug is a bit tricky.

rt4914 · 2021-02-08T14:33:12Z

@chrk2205 Your suggestions does sound correct too. @prayutsu Can you try to optimise as per his suggestions?

BenHenning

Took a high-level pass @prayutsu. I think I need some clarification on the desired behavior before I can go further thoughts on the specifics.

BenHenning · 2021-02-08T20:49:05Z

model/src/main/proto/arguments.proto

+message ExitProfileDialogArguments {
+  // Whether the last checked navigation drawer menu item needs to be restored(highlighted) or
+  // not after cancelling the Exit Profile Dialog.
+  bool restore_last_checked_menu_item = 1;


Can this be true if admin_controls_or_nav_drawer_items is not of type last_checked_menu_item? Can you enumerate via a reply all the different behaviors we expect to be supported by this proto? I think that will better help me understand if it's structured in a way that can satisfy those requirements.

BenHenning · 2021-02-08T20:52:13Z

app/src/main/java/org/oppia/android/app/drawer/Argument.kt

@@ -0,0 +1,6 @@
+package org.oppia.android.app.drawer
+
+sealed class Argument {


Why do we need a sealed class if we have a proto with a oneof? That should satisfy the same need.

BenHenning · 2021-02-08T20:56:46Z

Re: the discussion, I think it's preferable to keep the tab state management in the navigation drawer itself as much as possible. However given the issues around the drawer not knowing if navigation succeeded, I think we need some way to confirm the navigation before changing the status.

prayutsu · 2021-02-09T13:25:35Z

@rt4914 As there are a lot of changes that need to be reverted, should I consider making a new PR?

rt4914 · 2021-02-09T13:26:20Z

@rt4914 As there are a lot of changes that need to be reverted, should I consider making a new PR?

@prayutsu Yeah sure.

prayutsu · 2021-02-09T14:10:38Z

Closing this PR as created a new PR simplifying the implementation #2671

prayutsu added 3 commits September 6, 2020 01:26

fixed major bugs

d415788

fixed marking correct last item in navigation drawer

519c36a

fixed nits

803ce83

prayutsu mentioned this pull request Sep 5, 2020

Fix #1762 : Navigation drawer cancel switch profile always mark home #1773

Closed

8 tasks

fixed lint errors

e3f336e

prayutsu requested review from anandwana001 and rt4914 September 5, 2020 22:30

prayutsu assigned anandwana001 Sep 5, 2020

anandwana001 suggested changes Sep 6, 2020

View reviewed changes

app/src/main/java/org/oppia/app/drawer/ExitProfileDialogInterface.kt Outdated Show resolved Hide resolved

anandwana001 assigned prayutsu and unassigned anandwana001 Sep 6, 2020

prayutsu commented Sep 6, 2020

View reviewed changes

app/src/main/java/org/oppia/app/drawer/ExitProfileDialogFragment.kt Outdated Show resolved Hide resolved

test without any marking

1931f22

fixed almost all the bugs

c56d9c4

tried fixing last bug

5098c10

Merge branch 'develop' of https://github.com/oppia/oppia-android into…

0799f75

… navigation-drawer-switch-profile-fix

prayutsu commented Sep 26, 2020

View reviewed changes

app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt Outdated Show resolved Hide resolved

fixed all the bugs

245014b

prayutsu added 2 commits September 28, 2020 15:13

removed extra code

80a5a8f

Nit change

f9a8600

BenHenning requested changes Feb 4, 2021

View reviewed changes

BenHenning assigned prayutsu and unassigned BenHenning Feb 4, 2021

prayutsu added 5 commits February 5, 2021 15:10

Try implementing suggested changes.

381509d

Merge branch 'develop' of https://github.com/oppia/oppia-android into…

8ec8a2d

… navigation-drawer-switch-profile-fix

Fix lint errors.

d9a4608

Make use of enum defined in arguments.proto

fe5191f

Fix protobuf error.

b29f8e5

prayutsu commented Feb 5, 2021

View reviewed changes

prayutsu assigned BenHenning and unassigned prayutsu Feb 5, 2021

prayutsu commented Feb 5, 2021

View reviewed changes

prayutsu mentioned this pull request Feb 7, 2021

Switch Profile NavDrawer Menu Item gets unchecked on changing Configuration #2650

Closed

rt4914 self-assigned this Feb 8, 2021

rt4914 removed their assignment Feb 8, 2021

BenHenning reviewed Feb 8, 2021

View reviewed changes

BenHenning assigned prayutsu and unassigned BenHenning Feb 8, 2021

prayutsu mentioned this pull request Feb 9, 2021

Fix #1762, #2704: Highlight Correct MenuItem after cancelling ExitProfileDialog #2671

Merged

8 tasks

prayutsu closed this Feb 9, 2021

	.setLastCheckedMenuItemId(lastCheckedMenuItemId).build()
	.setLastCheckedMenuItemId(lastCheckedMenuItemId)
	.build()

	// The last checked menu item needs to be restored or not.
	// Whether the last checked menu item needs to be restored or not.

		@@ -0,0 +1,6 @@
		package org.oppia.android.app.drawer

		sealed class Argument {

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

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

Conversation

prayutsu commented Sep 5, 2020 • edited Loading

Explanation

Checklist

prayutsu commented Sep 5, 2020

anandwana001 left a comment

Choose a reason for hiding this comment

anandwana001 commented Sep 10, 2020 • edited Loading

prayutsu commented Sep 10, 2020

anandwana001 commented Sep 19, 2020

prayutsu commented Sep 19, 2020 • edited Loading

prayutsu commented Sep 19, 2020 • edited Loading

anandwana001 commented Sep 21, 2020

prayutsu commented Sep 21, 2020

anandwana001 commented Sep 25, 2020

prayutsu commented Sep 27, 2020

BenHenning 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

Choose a reason for hiding this comment

prayutsu 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

prayutsu Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

chrk2205 commented Feb 7, 2021 • edited Loading

prayutsu commented Feb 7, 2021 • edited Loading

chrk2205 commented Feb 8, 2021

prayutsu commented Feb 8, 2021

rt4914 commented Feb 8, 2021

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning commented Feb 8, 2021

prayutsu commented Feb 9, 2021

rt4914 commented Feb 9, 2021

prayutsu commented Feb 9, 2021

prayutsu commented Sep 5, 2020 •

edited

Loading

anandwana001 commented Sep 10, 2020 •

edited

Loading

prayutsu commented Sep 19, 2020 •

edited

Loading

prayutsu commented Sep 19, 2020 •

edited

Loading

prayutsu Feb 5, 2021 •

edited

Loading

chrk2205 commented Feb 7, 2021 •

edited

Loading

prayutsu commented Feb 7, 2021 •

edited

Loading