Refactor Chromoting.java to pull out some nested classes.

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 cd258d2532bb4fafa6be1cd6545eba976d1e6725..6668583b59e0bd48acaef710814842afe6f7cdad 100644
--- a/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
+++ b/remoting/android/java/src/org/chromium/chromoting/Chromoting.java
@@ -11,7 +11,6 @@ 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;
@@ -54,11 +53,9 @@ public class Chromoting extends Activity {
     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;
@@ -188,6 +185,28 @@ public class Chromoting extends Activity {
         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,
@@ -221,7 +240,7 @@ public class Chromoting extends Activity {
                 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).
@@ -262,7 +281,7 @@ public class Chromoting extends Activity {
                     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.
@@ -289,7 +308,6 @@ public class Chromoting extends Activity {
                     }
                 } else if (ex instanceof JSONException) {
                     explanation = getString(R.string.error_unexpected_response);
-                    runOnUiThread(new HostListDisplayer(mUi));
                 }
 
                 mHosts = null;
@@ -298,126 +316,104 @@ public class Chromoting extends Activity {
             }
 
             // 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 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;
+    /**
+     * 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);
             }
+        }
 
-            mGreeting.setText(getString(R.string.inst_host_list));
+        if (mHosts == null) {
+            mGreeting.setText(getString(R.string.inst_empty_list));
+            mList.setAdapter(null);
+            return;
+        }
 
-            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();
+        mGreeting.setText(getString(R.string.inst_host_list));
 
-                // Close the application.
-                finish();
+        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();
+
+            // 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);
-        }
+/** 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";
 
-        /** 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);
+    /** Color to use for hosts that are offline. */
+    private static final String HOST_COLOR_OFFLINE = "red";
 
-            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>)"));
-
-                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();
+    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();
 
-            return target;
+            // Close the application.
+            mChromoting.finish();
         }
+
+        return target;
     }
 }