ABP/Android UI

src/org/adblockplus/android/AdvancedPreferences.java

+package org.adblockplus.android;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.adblockplus.android.updater.AlarmReceiver;
+
+import android.app.AlertDialog;
+import android.app.Dialog;
+import android.content.ComponentName;
+import android.content.DialogInterface;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.content.SharedPreferences;
+import android.content.pm.PackageInfo;
+import android.content.pm.PackageManager.NameNotFoundException;
+import android.os.Build;
+import android.os.Bundle;
+import android.os.IBinder;
+import android.preference.Preference;
+import android.preference.PreferenceManager;
+import android.preference.PreferenceScreen;
+import android.preference.Preference.OnPreferenceClickListener;
+import android.text.ClipboardManager;
+import android.text.TextUtils;
+import android.util.Log;
+import android.view.View;
+import android.widget.TextView;
+import android.widget.Toast;
+
+/**
+ * Advanced settings UI.
+ */
+public class AdvancedPreferences extends SummarizedPreferences
+{
+  private final static String TAG = "AdvancedPreferences";

[Felix Dahlke, 2012/10/09 14:27:29]

How about "static final" instead of "final static"? That'd be consistent with
the SUN/Oracle conventions and the line below.

[Andrey Novikov, 2012/10/12 13:19:14]

Done.

+
+  private static final int CONFIGURATION_DIALOG = 1;
+
+  private static ProxyService proxyService = null;
+
+  @Override
+  public void onCreate(Bundle savedInstanceState)
+  {
+    super.onCreate(savedInstanceState);
+
+    addPreferencesFromResource(R.xml.preferences_advanced);
+
+    PreferenceScreen screen = getPreferenceScreen();
+    if (Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1
+    {
+      screen.removePreference(findPreference(getString(R.string.pref_proxy)));
+    }
+    if (getResources().getBoolean(R.bool.def_release))
+    {
+      screen.removePreference(findPreference(getString(R.string.pref_support)));
+    }
+    else
+    {
+      Preference prefUpdate = findPreference(getString(R.string.pref_checkupdate));
+      prefUpdate.setOnPreferenceClickListener(new OnPreferenceClickListener() {

[Felix Dahlke, 2012/10/09 14:27:29]

How about putting this opening brace on its own line?

[Andrey Novikov, 2012/10/12 13:19:14]

Done.

+        public boolean onPreferenceClick(Preference preference)
+        {
+          Intent updater = new Intent(getApplicationContext(), AlarmReceiver.class).putExtra("notifynoupdate", true);
+          sendBroadcast(updater);
+          return true;
+        }
+      });
+
+      Preference prefConfiguration = findPreference(getString(R.string.pref_configuration));
+      prefConfiguration.setOnPreferenceClickListener(new OnPreferenceClickListener() {

[Felix Dahlke, 2012/10/09 14:27:29]

Same as above, brace should be on its own line.

[Andrey Novikov, 2012/10/12 13:19:14]

Done.

+        public boolean onPreferenceClick(Preference preference)
+        {
+          showDialog(CONFIGURATION_DIALOG);
+          return true;
+        }
+      });
+    }
+  }
+
+  @Override
+  public void onResume()
+  {
+    super.onResume();
+    SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this);
+    int refresh = Integer.valueOf(prefs.getString(getString(R.string.pref_refresh), "0"));
+    findPreference(getString(R.string.pref_wifirefresh)).setEnabled(refresh > 0);
+    connect();
+  }
+
+  @Override
+  public void onPause()
+  {
+    super.onPause();
+    disconnect();
+  }
+
+  @Override
+  public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key)
+  {
+    if (getString(R.string.pref_refresh).equals(key))
+    {
+      int refresh = Integer.valueOf(sharedPreferences.getString(getString(R.string.pref_refresh), "0"));
+      findPreference(getString(R.string.pref_wifirefresh)).setEnabled(refresh > 0);
+    }
+    if (getString(R.string.pref_crashreport).equals(key))
+    {
+      AdblockPlus application = AdblockPlus.getApplication();
+      application.updateCrashReportStatus();
+    }
+    super.onSharedPreferenceChanged(sharedPreferences, key);
+  }
+
+  @Override
+  protected Dialog onCreateDialog(int id)
+  {

[Felix Dahlke, 2012/10/09 14:27:29]

You know I don't like long methods, this is one :) I don't insist on this being
split up, but it would be easier for me to understand that way.

[Andrey Novikov, 2012/10/12 13:19:14]

We worked out a compromise in adding more comments.

+    Dialog dialog = null;
+    switch (id)

[Felix Dahlke, 2012/10/09 14:27:29]

Why use a switch case if there's only one case handled? I'd prefer an if.

[Andrey Novikov, 2012/10/12 13:19:14]

This is a well known Android code pattern, there can be more dialogs in future.

+    {
+      case CONFIGURATION_DIALOG:
+        List<String> items = new ArrayList<String>();
+        int versionCode = -1;
+        try

[Felix Dahlke, 2012/10/09 14:27:29]

The same code for getting the package info is in AboutDialog. Maybe this
warrants a utility method that ignores the exception? In this case that would
simplify the following lines to:

PackageInfo pi = Utils.getPackageInfo(this);
versionCode = pi.versionCode;

But if it becomes utility code, we can't really ignore the exception safely
anymore, we should throw a RuntimeException or log an error.

[Andrey Novikov, 2012/10/12 13:19:14]

NameNotFoundException is thrown when a given package, application, or component
name can not be found. It definitely finds caller package info.

AboutDialog also reads versionName which is not needed here, that's why I do not
put it into utility function.

[Felix Dahlke, 2012/10/12 13:29:47]

That's why I suggested to return a PackageInfo object instead of just the
versionCode.

+        {
+          PackageInfo pi = getPackageManager().getPackageInfo(getPackageName(), 0);
+          versionCode = pi.versionCode;
+        }
+        catch (NameNotFoundException e)
+        {
+          // ignore - this shouldn't happen
+        }
+        items.add(String.format("API: %d Build: %d", Build.VERSION.SDK_INT, versionCode));
+        if (proxyService != null)
+        {
+          items.add(String.format("Local port: %d", proxyService.port));
+          if (proxyService.isTransparent())
+          {
+            items.add("Running in root mode");
+          }
+          if (proxyService.isNativeProxy())
+          {
+            items.add("Uses native proxy");
+          }
+          if (Build.VERSION.SDK_INT >= 12) // Honeycomb 3.1

[Felix Dahlke, 2012/10/09 14:27:29]

I think this should be a constant, since it's also used above. Or better yet,
set a boolean field in the constructor.

[Andrey Novikov, 2012/10/12 13:19:14]

Done.

+          {
+            String[] px = proxyService.getUserProxy();
+            if (px != null)
+            {
+              items.add("System settings:");
+              items.add(String.format("Host: [%s] Port: [%s] Excl: [%s]", px[0], px[1], px[2]));
+            }
+          }
+          items.add("Proxy settings:");
+          items.add(String.format("Host: [%s] Port: [%s] Excl: [%s]", proxyService.proxy.props.getProperty("adblock.proxyHost"), proxyService.proxy.props.getProperty("adblock.proxyPort"),
+              proxyService.proxy.props.getProperty("adblock.proxyExcl")));
+          if (proxyService.proxy.props.getProperty("adblock.auth") != null)
+            items.add("Auth: yes");
+        }
+        else
+        {
+          items.add("Service not running");
+        }
+
+        TextView messageText = new TextView(this);
+        messageText.setPadding(12, 6, 12, 6);
+        messageText.setText(TextUtils.join("\n", items));
+        messageText.setOnClickListener(new View.OnClickListener() {

[Felix Dahlke, 2012/10/09 14:27:29]

Brace on a new line please.

[Andrey Novikov, 2012/10/12 13:19:14]

Done.

+
+          @Override
+          public void onClick(View v)
+          {
+            ClipboardManager manager = (ClipboardManager) getSystemService(CLIPBOARD_SERVICE);
+            TextView showTextParam = (TextView) v;
+            manager.setText(showTextParam.getText());
+            Toast.makeText(v.getContext(), R.string.msg_clipboard, Toast.LENGTH_SHORT).show();
+          }
+        });
+
+        AlertDialog.Builder builder = new AlertDialog.Builder(this);
+        builder.setView(messageText).setTitle(R.string.configuration_name).setIcon(android.R.drawable.ic_dialog_info).setCancelable(false)
+            .setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() {
+              public void onClick(DialogInterface dialog, int id)
+              {
+                dialog.cancel();
+              }
+            });
+        dialog = builder.create();
+        break;
+    }
+    return dialog;
+  }
+
+  private void connect()
+  {
+    bindService(new Intent(this, ProxyService.class), proxyServiceConnection, 0);
+  }
+
+  private void disconnect()
+  {
+    unbindService(proxyServiceConnection);
+    proxyService = null;
+  }
+
+  private ServiceConnection proxyServiceConnection = new ServiceConnection() {
+    public void onServiceConnected(ComponentName className, IBinder service)
+    {
+      proxyService = ((ProxyService.LocalBinder) service).getService();
+      Log.d(TAG, "Proxy service connected");
+    }
+
+    public void onServiceDisconnected(ComponentName className)
+    {
+      proxyService = null;
+      Log.d(TAG, "Proxy service disconnected");
+    }
+  };
+}