Implementing Host List Context Menu and Delete Feature

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 dfd1e8e5a1016332051b6092e7cef20adcb8f651..360b0b35bc41f1f74f81a393f6931fe893f0a29c 100644
--- a/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
+++ b/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
@@ -20,6 +20,7 @@ import android.support.v7.app.ActionBarDrawerToggle;
 import android.support.v7.app.AlertDialog;
 import android.support.v7.app.AppCompatActivity;
 import android.support.v7.widget.Toolbar;
+import android.view.ContextMenu;
 import android.view.Gravity;
 import android.view.Menu;
 import android.view.MenuItem;
@@ -116,6 +117,9 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
      */
     private boolean mWaitingForAuthToken = false;
 
+    /** ID of the host in progress of deletion. **/
+    private String mHostIdBeingDeleted;
+
     private DrawerLayout mDrawerLayout;
 
     private ActionBarDrawerToggle mDrawerToggle;
@@ -196,6 +200,7 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
 
         // Get ahold of our view widgets.
         mHostListView = (ListView) findViewById(R.id.hostList_chooser);
+        registerForContextMenu(mHostListView);
         mEmptyView = findViewById(R.id.hostList_empty);
         mHostListView.setOnItemClickListener(
                 new AdapterView.OnItemClickListener() {
@@ -371,7 +376,7 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
             if (resultCode == RESULT_OK) {
                 // User gave OAuth permission to this app (or recovered from any OAuth failure),
                 // so retry fetching the token.
-                requestAuthToken(false);
+                refreshHostList(false);
             } else {
                 // User denied permission or cancelled the dialog, so cancel the request.
                 mWaitingForAuthToken = false;
@@ -404,7 +409,49 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
         mDrawerToggle.onConfigurationChanged(newConfig);
     }
 
-    /** Called to initialize the action bar. */
+    private int getHostIndexForMenu(ContextMenu.ContextMenuInfo menuInfo) {

[Lambros, 2016/05/12 23:42:13]

static

[Yuwei, 2016/05/13 19:09:32]

Done.

+        return ((AdapterView.AdapterContextMenuInfo) menuInfo).position;
+    }
+
+    @Override
+    public void onCreateContextMenu(ContextMenu menu, View v,
+                                    ContextMenu.ContextMenuInfo menuInfo) {

[Lambros, 2016/05/12 23:42:13]

nit: Java line-continuation is +8-space indent.

[Yuwei, 2016/05/13 19:09:32]

Done.

+        super.onCreateContextMenu(menu, v, menuInfo);
+        if (v.getId() == R.id.hostList_chooser) {
+            getMenuInflater().inflate(R.menu.options_actionbar, menu);
+            HostInfo info = mHosts[getHostIndexForMenu(menuInfo)];
+            menu.setHeaderTitle(info.name);
+        }
+    }
+
+    @Override
+    public boolean onContextItemSelected(MenuItem item) {
+        int i = item.getItemId();

[Lambros, 2016/05/12 23:42:13]

nit: 'id' instead of 'i'.

[Yuwei, 2016/05/13 19:09:32]

Done.

+        if (i == R.id.actionbar_connect) {
+            onHostClicked(getHostIndexForMenu(item.getMenuInfo()));

[Lambros, 2016/05/12 23:42:13]

Maybe pull out the index?
int hostIndex = getHostIndexForMenu(item.getMenuInfo());

[Yuwei, 2016/05/13 19:09:32]

Done.

+        } else if (i == R.id.actionbar_delete) {
+            HostInfo hostInfo = mHosts[getHostIndexForMenu(item.getMenuInfo())];
+            AlertDialog.Builder builder = new AlertDialog.Builder(this);
+            String message = getString(R.string.confirm_host_delete_android, hostInfo.name);
+            final String hostId = hostInfo.id;
+            builder.setMessage(message)
+                    .setPositiveButton(android.R.string.yes,
+                            new DialogInterface.OnClickListener() {
+                                @Override
+                                public void onClick(DialogInterface dialog, int which) {
+                                    deleteHost(false, hostId);
+                                    dialog.dismiss();
+                                }
+                            })
+                    .setNegativeButton(android.R.string.no, null)
+                    .create().show();
+        } else {
+            return false;
+        }
+        return true;
+    }
+
+        /** Called to initialize the action bar. */

[Lambros, 2016/05/12 23:42:13]

nit: indentation

[Yuwei, 2016/05/13 19:09:32]

Done.

     @Override
     public boolean onCreateOptionsMenu(Menu menu) {
         getMenuInflater().inflate(R.menu.chromoting_actionbar, menu);
@@ -429,7 +476,7 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
 
         int id = item.getItemId();
         if (id == R.id.actionbar_directoryrefresh) {
-            refreshHostList();
+            refreshHostList(false);
             return true;
         }
         return super.onOptionsItemSelected(item);
@@ -480,19 +527,58 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
                 getPreferences(MODE_PRIVATE).getString(PREFERENCE_EXPERIMENTAL_FLAGS, ""));
     }
 
-    private void refreshHostList() {
+    private void showAuthErrorMessage(OAuthTokenFetcher.Error error) {
+        String explanation = getString(error == OAuthTokenFetcher.Error.NETWORK
+                ? R.string.error_network_error : R.string.error_unexpected);
+        Toast.makeText(Chromoting.this, explanation, Toast.LENGTH_LONG).show();
+    }
+
+    private void refreshHostList(boolean expireCurrentToken) {
         if (mWaitingForAuthToken) {
             return;
         }
-
-        mTriedNewAuthToken = false;
         showHostListLoadingIndicator();
 
         // The refresh button simply makes use of the currently-chosen account.
-        requestAuthToken(false);
+        requestAuthToken(expireCurrentToken, new OAuthTokenFetcher.Callback() {
+            @Override
+            public void onTokenFetched(String token) {
+                mHostListManager.retrieveHostList(mToken, Chromoting.this);
+            }
+
+            @Override
+            public void onError(OAuthTokenFetcher.Error error) {
+                showAuthErrorMessage(error);
+                updateHostListView();
+            }
+        });
+    }
+
+    private void deleteHost(boolean expireCurrentToken, final String hostId) {
+        if (mWaitingForAuthToken) {
+            return;
+        }
+        showHostListLoadingIndicator();
+
+        mHostIdBeingDeleted = hostId;
+
+        requestAuthToken(expireCurrentToken, new OAuthTokenFetcher.Callback() {
+            @Override
+            public void onTokenFetched(String token) {
+                mHostListManager.deleteHost(token, hostId, Chromoting.this);
+            }
+
+            @Override
+            public void onError(OAuthTokenFetcher.Error error) {
+                showAuthErrorMessage(error);
+            }
+        });
     }
 
-    private void requestAuthToken(boolean expireCurrentToken) {
+    // TODO(yuweih): This implementation have undefined behavior if multiple auth token requests
+    // start at the same time. In that case we should consider queueing up the requests.
+    private void requestAuthToken(boolean expireCurrentToken,
+                                  final OAuthTokenFetcher.Callback callback) {

[Lambros, 2016/05/12 23:42:13]

nit: line-continuation is +8 space.

         mWaitingForAuthToken = true;
 
         OAuthTokenFetcher fetcher = new OAuthTokenFetcher(this, mAccount, TOKEN_SCOPE,
@@ -501,16 +587,13 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
                     public void onTokenFetched(String token) {
                         mWaitingForAuthToken = false;
                         mToken = token;
-                        mHostListManager.retrieveHostList(mToken, Chromoting.this);
+                        callback.onTokenFetched(token);
                     }
 
                     @Override
                     public void onError(OAuthTokenFetcher.Error error) {
                         mWaitingForAuthToken = false;
-                        updateHostListView();
-                        String explanation = getString(error == OAuthTokenFetcher.Error.NETWORK
-                                ? R.string.error_network_error : R.string.error_unexpected);
-                        Toast.makeText(Chromoting.this, explanation, Toast.LENGTH_LONG).show();
+                        callback.onError(error);
                     }
                 });
 
@@ -529,7 +612,7 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
         // The current host list is no longer valid for the new account, so clear the list.
         mHosts = new HostInfo[0];
         updateUi();
-        refreshHostList();
+        refreshHostList(false);
     }
 
     @Override
@@ -546,6 +629,7 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
     public void onHostListReceived(HostInfo[] hosts) {
         // Store a copy of the array, so that it can't be mutated by the HostListManager. HostInfo
         // is an immutable type, so a shallow copy of the array is sufficient here.
+        mTriedNewAuthToken = false;

[Lambros, 2016/05/12 23:42:13]

I think we can get rid of the retry-if-auth-token-expired logic. The old
AccountsManager API would often return invalid OAuth tokens which the
application needed to test and refresh explicitly. The new GoogleAuthUtil class
doesn't have this problem - it always hands us good tokens (it refreshes
automatically if expired).

Don't need to worry about it for this CL.

[Yuwei, 2016/05/13 19:09:32]

Done.

         mHosts = Arrays.copyOf(hosts, hosts.length);
         updateHostListView();
         updateUi();
@@ -558,11 +642,12 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
 
     @Override
     public void onHostDeleted() {
-        // Not implemented Yet.
+        mHostIdBeingDeleted = null;
+        mHostListManager.retrieveHostList(mToken, this);
     }
 
     @Override
-    public void onError(HostListManager.Error error) {
+    public void onError(HostListManager.RequestType type, HostListManager.Error error) {
         String explanation = null;
         switch (error) {
             case AUTH_FAILED:
@@ -591,12 +676,18 @@ public class Chromoting extends AppCompatActivity implements ConnectionListener,
         if (!mTriedNewAuthToken) {
             // This was our first connection attempt.
             mTriedNewAuthToken = true;
-            requestAuthToken(true);
-
+            switch (type) {
+                case RETRIEVE_HOST_LIST:
+                    refreshHostList(true);
+                    break;
+                case DELETE_HOST:
+                    deleteHost(true, mHostIdBeingDeleted);
+            }
             // We're not in an error state *yet*.
-            return;
         } else {
             // Authentication truly failed.
+            mTriedNewAuthToken = false;
+            mHostIdBeingDeleted = null;
             Log.e(TAG, "Fresh auth token was rejected.");
             explanation = getString(R.string.error_authentication_failed);
             Toast.makeText(this, explanation, Toast.LENGTH_LONG).show();