[remoting android] Close navigation drawer before opening Help/Feedback.

remoting/android/java/src/org/chromium/chromoting/Chromoting.java

Index: remoting/android/java/src/org/chromium/chromoting/Chromoting.java
diff --git a/remoting/android/java/src/org/chromium/chromoting/Chromoting.java b/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
index 6cfd884171550c02d3043f7bd5212d8ee15caedd..45c4627c53f1f00d36ff81930aa99ee2a79764f9 100644
--- a/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
+++ b/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
@@ -108,6 +108,12 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
 
     private ActionBarDrawerToggle mDrawerToggle;
 
+    /**
+     * Task to be run after the navigation drawer is closed. Can be null. This is used to run
+     * Help/Feedback tasks which require a screenshot with the drawer closed.
+     */
+    private Runnable mPendingDrawerCloseTask;
+
     private AccountSwitcher mAccountSwitcher;
 
     /** The currently-connected Client, if any. */
@@ -167,6 +173,44 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
         mProgressView.setVisibility(View.GONE);
     }
 
+    private void runPendingDrawerCloseTask() {
+        // Avoid potential recursion problems by null-ing the task first.
+        Runnable task = mPendingDrawerCloseTask;

[Yuwei, 2016/10/04 23:41:14]

Optionally you can have a queue of pending tasks rather than just one task so
that closeDrawerThenRun() can be run multiple times in a task.

[Lambros, 2016/10/05 00:03:15]

Acknowledged. No need for a queue just yet.

+        mPendingDrawerCloseTask = null;
+        if (task != null) {
+            task.run();
+        }
+    }
+
+    private void closeDrawerThenRun(Runnable task) {

[Yuwei, 2016/10/04 23:41:14]

Instead of having extra launch* methods here, could it be better to make
closeDrawerThenRun() public then create the anonymous class in
NavigationMenuAdapter? By doing so we don't need to go back and forth between
Chromoting and the Adapter when we later change the list items.

[Lambros, 2016/10/05 00:03:15]

If I'm not mistaken, this would mean doubly-nested anonymous Runnable classes in
NavigationMenuAdapter which would look really ugly.
That could be avoided by pulling out the Runnable-creation into new methods, but
that doesn't seem any better or worse than what I've got now.

[Yuwei, 2016/10/05 00:27:02]

Yep. Callback hell :) Though it can be simplified to:

()->{closeDrawerThenRun(()->{HelpSingleton.get().launchFeedback();});}

once the N SDK get rolled in.
Or you can just create them at the beginning of createNavigationMenu().

Maybe we can later implement the Promise and have something like this:

item.onClick().then(()->{
  return chromoting.closeDrawer();
}).then(()->{
  launchHelp(...);
});
Since Chromoting.java is already too messy to read, I would try not to introduce
more unrelated logic to this class. I don't really have a strong opinion for
which one is better, so you can go ahead if you like.

+        mPendingDrawerCloseTask = task;
+        if (mDrawerLayout.isDrawerOpen(Gravity.START)) {
+            mDrawerLayout.closeDrawer(Gravity.START);
+        } else {
+            runPendingDrawerCloseTask();
+        }
+    }
+
+    /** Closes any navigation drawer, then shows the Help screen. */
+    public void launchHelp(final HelpContext helpContext) {
+        closeDrawerThenRun(new Runnable() {
+            @Override
+            public void run() {
+                HelpSingleton.getInstance().launchHelp(Chromoting.this, helpContext);
+            }
+        });
+    }
+
+    /** Closes any navigation drawer, then shows the Feedback screen. */
+    public void launchFeedback() {
+        closeDrawerThenRun(new Runnable() {
+            @Override
+            public void run() {
+                HelpSingleton.getInstance().launchFeedback(Chromoting.this);
+            }
+        });
+    }
+
     /**
      * Called when the activity is first created. Loads the native library and requests an
      * authentication token from the system.
@@ -200,7 +244,13 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
 
         mDrawerLayout = (DrawerLayout) findViewById(R.id.drawer_layout);
         mDrawerToggle = new ActionBarDrawerToggle(this, mDrawerLayout, toolbar,
-                R.string.open_navigation_drawer, R.string.close_navigation_drawer);
+                R.string.open_navigation_drawer, R.string.close_navigation_drawer) {
+            @Override
+            public void onDrawerClosed(View drawerView) {
+                super.onDrawerClosed(drawerView);
+                runPendingDrawerCloseTask();
+            }
+        };
         mDrawerLayout.addDrawerListener(mDrawerToggle);
 
         // Disable the hamburger icon animation. This is more complex than it ought to be.
@@ -440,7 +490,7 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
     /** Called when the user touches hyperlinked text. */
     @Override
     public void onClick(View view) {
-        HelpSingleton.getInstance().launchHelp(this, HelpContext.HOST_SETUP);
+        launchHelp(HelpContext.HOST_SETUP);
     }
 
     private void onDeleteHostClicked(int hostIndex) {