Refactor Chromoting.java to pull out some nested classes.

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chromoting;
 
 import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.accounts.AccountManagerCallback;
 import android.accounts.AccountManagerFuture;
 import android.accounts.AuthenticatorException;
 import android.accounts.OperationCanceledException;
 import android.app.Activity;
-import android.content.Context;
 import android.content.Intent;
 import android.content.SharedPreferences;
 import android.os.Bundle;
 import android.os.Handler;
 import android.os.HandlerThread;
 import android.text.Html;
 import android.util.Log;
 import android.view.Menu;
 import android.view.MenuItem;
 import android.view.View;
@@ skipping 22 matching lines @@
     private static final String ACCOUNT_TYPE = "com.google";
 
     /** Scopes at which the authentication token we request will be valid. */
     private static final String TOKEN_SCOPE = "oauth2:https://www.googleapis.com/auth/chromoting " +
             "https://www.googleapis.com/auth/googletalk";
 
     /** Path from which to download a user's host list JSON object. */
     private static final String HOST_LIST_PATH =
             "https://www.googleapis.com/chromoting/v1/@me/hosts?key=";
 
-    /** Color to use for hosts that are online. */
-    private static final String HOST_COLOR_ONLINE = "green";
-
-    /** Color to use for hosts that are offline. */
-    private static final String HOST_COLOR_OFFLINE = "red";
+    /** Lock to protect |mAccount| and |mToken|. */
+    // TODO(lambroslambrou): |mHosts| needs to be protected as well.
+    private Object mLock = new Object();
 
     /** User's account details. */
     private Account mAccount;
 
     /** Account auth token. */
     private String mToken;
 
     /** List of hosts. */
     private JSONArray mHosts;
 
@@ skipping 109 matching lines @@
         }
         else {
             // The refresh button simply makes use of the currently-chosen account.
             AccountManager.get(this).getAuthToken(mAccount, TOKEN_SCOPE, null, this,
                     new HostListDirectoryGrabber(this), mNetwork);
         }
 
         return true;
     }
 
+    /** Called when the user taps on a host entry. */
+    public void connectToHost(JSONObject host) {
+        try {
+            synchronized (mLock) {
+                JniInterface.connectToHost(mAccount.name, mToken, host.getString("jabberId"),
+                        host.getString("hostId"), host.getString("publicKey"),
+                        new Runnable() {
+                                @Override
+                                public void run() {
+                                    startActivity(new Intent(Chromoting.this, Desktop.class));
+                                }
+                        });
+            }
+        } catch (JSONException ex) {
+            Log.w("host", ex);
+            Toast.makeText(this, getString(R.string.error_reading_host),
+                    Toast.LENGTH_LONG).show();
+            // Close the application.
+            finish();
+        }
+    }
+
     /**
      * Processes the authentication token once the system provides it. Once in possession of such a
      * token, attempts to request a host list from the directory server. In case of a bad response,
      * this is retried once in case the system's cached auth token had expired.
      */
     private class HostListDirectoryGrabber implements AccountManagerCallback<Bundle> {
         /** Whether authentication has already been attempted. */
         private boolean mAlreadyTried;
 
         /** Communication with the screen. */
@@ skipping 13 matching lines @@
         public void run(AccountManagerFuture<Bundle> future) {
             Log.i("auth", "User finished with auth dialogs");
             try {
                 // Here comes our auth token from the Android system.
                 Bundle result = future.getResult();
                 String accountName = result.getString(AccountManager.KEY_ACCOUNT_NAME);
                 String accountType = result.getString(AccountManager.KEY_ACCOUNT_TYPE);
                 String authToken = result.getString(AccountManager.KEY_AUTHTOKEN);
                 Log.i("auth", "Received an auth token from system");
 
-                synchronized (mUi) {
+                synchronized (mLock) {
                     mAccount = new Account(accountName, accountType);
                     mToken = authToken;
                     getPreferences(MODE_PRIVATE).edit().putString("account_name", accountName).
                             putString("account_type", accountType).apply();
                 }
 
                 // Send our HTTP request to the directory server.
                 URLConnection link =
                         new URL(HOST_LIST_PATH + JniInterface.nativeGetApiKey()).openConnection();
                 link.addRequestProperty("client_id", JniInterface.nativeGetClientId());
@@ skipping 20 matching lines @@
                 // Assemble error message to display to the user.
                 String explanation = getString(R.string.error_unknown);
                 if (ex instanceof OperationCanceledException) {
                     explanation = getString(R.string.error_auth_canceled);
                 } else if (ex instanceof AuthenticatorException) {
                     explanation = getString(R.string.error_no_accounts);
                 } else if (ex instanceof IOException) {
                     if (!mAlreadyTried) {
                         // This was our first connection attempt.
 
-                        synchronized (mUi) {
+                        synchronized (mLock) {
                             if (mAccount != null) {
                                 // We got an account, but couldn't log into it. We'll retry in case
                                 // the system's cached authentication token had already expired.
                                 AccountManager authenticator = AccountManager.get(mUi);
                                 mAlreadyTried = true;
 
                                 Log.w("auth", "Requesting renewal of rejected auth token");
                                 authenticator.invalidateAuthToken(mAccount.type, mToken);
                                 mToken = null;
                                 authenticator.getAuthToken(
                                         mAccount, TOKEN_SCOPE, null, mUi, this, mNetwork);
 
                                 // We're not in an error state *yet*.
                                 return;
                             }
                         }
 
                         // We didn't even get an account, so the auth server is likely unreachable.
                         explanation = getString(R.string.error_bad_connection);
                     } else {
                         // Authentication truly failed.
                         Log.e("auth", "Fresh auth token was also rejected");
                         explanation = getString(R.string.error_auth_failed);
                     }
                 } else if (ex instanceof JSONException) {
                     explanation = getString(R.string.error_unexpected_response);
-                    runOnUiThread(new HostListDisplayer(mUi));
                 }
 
                 mHosts = null;
                 Log.w("auth", ex);
                 Toast.makeText(mUi, explanation, Toast.LENGTH_LONG).show();
             }
 
             // Share our findings with the user.
-            runOnUiThread(new HostListDisplayer(mUi));
+            runOnUiThread(new Runnable() {
+                    @Override
+                    public void run() {
+                        updateUi();
+                    }
+            });
         }
     }
 
-    /** Formats the host list and offers it to the user. */
-    private class HostListDisplayer implements Runnable {
-        /** Communication with the screen. */
-        private Activity mUi;
-
-        /** Constructor. */
-        public HostListDisplayer(Activity ui) {
-            mUi = ui;
+    /**
+     * Updates the infotext and host list display.
+     * This method affects the UI and must be run on the main thread.
+     */
+    private void updateUi() {
+        synchronized (mLock) {
+            mRefreshButton.setEnabled(mAccount != null);
+            if (mAccount != null) {
+                mAccountSwitcher.setTitle(mAccount.name);
+            }
         }
 
-        /**
-         * Updates the infotext and host list display.
-         * This method affects the UI and must be run on its same thread.
-         */
-        @Override
-        public void run() {
-            synchronized (mUi) {
-                mRefreshButton.setEnabled(mAccount != null);
-                if (mAccount != null) {
-                    mAccountSwitcher.setTitle(mAccount.name);
-                }
-            }
-
-            if (mHosts == null) {
-                mGreeting.setText(getString(R.string.inst_empty_list));
-                mList.setAdapter(null);
-                return;
-            }
-
-            mGreeting.setText(getString(R.string.inst_host_list));
-
-            ArrayAdapter<JSONObject> displayer = new HostListAdapter(mUi, R.layout.host);
-            Log.i("hostlist", "About to populate host list display");
-            try {
-                int index = 0;
-                while (!mHosts.isNull(index)) {
-                    displayer.add(mHosts.getJSONObject(index));
-                    ++index;
-                }
-                mList.setAdapter(displayer);
-            }
-            catch(JSONException ex) {
-                Log.w("hostlist", ex);
-                Toast.makeText(
-                        mUi, getString(R.string.error_cataloging_hosts), Toast.LENGTH_LONG).show();
-
-                // Close the application.
-                finish();
-            }
-        }
-    }
-
-    /** Describes the appearance and behavior of each host list entry. */
-    private class HostListAdapter extends ArrayAdapter<JSONObject> {
-        /** Constructor. */
-        public HostListAdapter(Context context, int textViewResourceId) {
-            super(context, textViewResourceId);
+        if (mHosts == null) {
+            mGreeting.setText(getString(R.string.inst_empty_list));
+            mList.setAdapter(null);
+            return;
         }
 
-        /** Generates a View corresponding to this particular host. */
-        @Override
-        public View getView(int position, View convertView, ViewGroup parent) {
-            TextView target = (TextView)super.getView(position, convertView, parent);
+        mGreeting.setText(getString(R.string.inst_host_list));
 
-            try {
-                final JSONObject host = getItem(position);
-                target.setText(Html.fromHtml(host.getString("hostName") + " (<font color = \"" +
-                        (host.getString("status").equals("ONLINE") ? HOST_COLOR_ONLINE :
-                        HOST_COLOR_OFFLINE) + "\">" + host.getString("status") + "</font>)"));
+        ArrayAdapter<JSONObject> displayer = new HostListAdapter(this, R.layout.host);
+        Log.i("hostlist", "About to populate host list display");
+        try {
+            int index = 0;
+            while (!mHosts.isNull(index)) {
+                displayer.add(mHosts.getJSONObject(index));
+                ++index;
+            }
+            mList.setAdapter(displayer);
+        } catch (JSONException ex) {
+            Log.w("hostlist", ex);
+            Toast.makeText(this, getString(R.string.error_cataloging_hosts),
+                    Toast.LENGTH_LONG).show();
 
-                if (host.getString("status").equals("ONLINE")) {  // Host is online.
-                    target.setOnClickListener(new View.OnClickListener() {
-                            @Override
-                            public void onClick(View v) {
-                                try {
-                                    synchronized (getContext()) {
-                                        JniInterface.connectToHost(mAccount.name, mToken,
-                                                host.getString("jabberId"),
-                                                host.getString("hostId"),
-                                                host.getString("publicKey"),
-                                                new Runnable() {
-                                            @Override
-                                            public void run() {
-                                                startActivity(
-                                                        new Intent(getContext(), Desktop.class));
-                                            }
-                                        });
-                                    }
-                                }
-                                catch(JSONException ex) {
-                                    Log.w("host", ex);
-                                    Toast.makeText(getContext(),
-                                            getString(R.string.error_reading_host),
-                                            Toast.LENGTH_LONG).show();
-
-                                    // Close the application.
-                                    finish();
-                                }
-                            }
-                        });
-                } else {  // Host is offline.
-                    // Disallow interaction with this entry.
-                    target.setEnabled(false);
-                }
-            }
-            catch(JSONException ex) {
-                Log.w("hostlist", ex);
-                Toast.makeText(getContext(),
-                        getString(R.string.error_displaying_host),
-                        Toast.LENGTH_LONG).show();
-
-                // Close the application.
-                finish();
-            }
-
-            return target;
+            // Close the application.
+            finish();
         }
     }
 }
+
+/** Describes the appearance and behavior of each host list entry. */
+class HostListAdapter extends ArrayAdapter<JSONObject> {

[Sergey Ulanov, 2013/12/21 02:15:28]

Does this compile? I think you need to put it to a separate file.

[Lambros, 2013/12/26 19:29:46]

It's legal according to:
http://docs.oracle.com/javase/specs/jls/se7/html/jls-7.html#jls-7.6

You can have multiple classes defined in a .java file, with either public or
unspecified (package) access. But at most one of them can be public.

The compiler may enforce the restriction that the extra non-public classes can
only be used from the same source file. So the non-public class is effectively a
private implementation detail of the public class.

If we think the extra class is truly a Pimpl, it makes sense to keep it in the
same file (otherwise we should move it to another file as you suggested).

And then, if we keep it in the same file, it can either be a separate class, or
a static nested class. I prefer a separate class in this case, as it's better
encapsulated than a nested class, which could access private members of its
outer class.
http://stackoverflow.com/questions/2148686/non-public-top-level-class-vs-stat...

[Sergey Ulanov, 2013/12/26 20:03:48]

This isn't really a Pimple (pimple = Private IMPLEmentation). Anyway, even if
it's allowed by the language, it still makes this code less readable. I think it
would be better to move it to a separate file. Google Java style explicitly
prohibits putting more than one top-level class in a single file
(http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s3-source-fi...)
:
  A source file consists of, in order:
    - License or copyright information, if present
    - Package statement
    - Import statements
    - Exactly one top-level class

On 2013/12/26 19:29:46, Lambros wrote:

[Lambros, 2013/12/27 01:56:54]

OK, done!

+    /** Color to use for hosts that are online. */
+    private static final String HOST_COLOR_ONLINE = "green";
+
+    /** Color to use for hosts that are offline. */
+    private static final String HOST_COLOR_OFFLINE = "red";
+
+    private Chromoting mChromoting;
+
+    /** Constructor. */
+    public HostListAdapter(Chromoting chromoting, int textViewResourceId) {
+        super(chromoting, textViewResourceId);
+        mChromoting = chromoting;
+    }
+
+    /** Generates a View corresponding to this particular host. */
+    @Override
+    public View getView(int position, View convertView, ViewGroup parent) {
+        TextView target = (TextView)super.getView(position, convertView, parent);
+
+        try {
+            final JSONObject host = getItem(position);
+            String status = host.getString("status");
+            boolean online = status.equals("ONLINE");
+            target.setText(Html.fromHtml(host.getString("hostName") + " (<font color = \"" +
+                    (online ? HOST_COLOR_ONLINE : HOST_COLOR_OFFLINE) + "\">" + status +
+                    "</font>)"));
+
+            if (online) {
+                target.setOnClickListener(new View.OnClickListener() {
+                        @Override
+                        public void onClick(View v) {
+                            mChromoting.connectToHost(host);
+                        }
+                });
+            } else {
+                // Disallow interaction with this entry.
+                target.setEnabled(false);
+            }
+        } catch (JSONException ex) {
+            Log.w("hostlist", ex);
+            Toast.makeText(mChromoting, mChromoting.getString(R.string.error_displaying_host),
+                    Toast.LENGTH_LONG).show();
+
+            // Close the application.
+            mChromoting.finish();
+        }
+
+        return target;
+    }
+}