ABP/Android UI

src/org/adblockplus/android/Preferences.java

 package org.adblockplus.android;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.List;
 
 import android.app.ActivityManager;
@@ skipping 20 matching lines @@
 import android.view.MenuItem;
 import android.view.View;
 import android.view.Window;
 import android.widget.TextView;
 
 /**
  * Main settings UI.
  */
 public class Preferences extends SummarizedPreferences
 {
-  private final static String TAG = "Preferences";

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

As in AdvancedPreferences, "static final"?

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

Done.

+  private static final String TAG = "Preferences";
 
   private AboutDialog aboutDialog;
   private boolean showAbout = false;
   private String configurationMsg;
   private String subscriptionSummary;
 
   @Override
   public void onCreate(Bundle savedInstanceState)
   {
     requestWindowFeature(Window.FEATURE_NO_TITLE);
@@ skipping 15 matching lines @@
       if (lastVersion != thisVersion)
       {
         copyAssets();
         SharedPreferences.Editor editor = prefs.edit();
         editor.putInt(getString(R.string.pref_version), thisVersion);
         editor.commit();
       }
     }
     catch (NameNotFoundException e)
     {
       copyAssets();

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

Why can this getPackageInfo fail while those in AboutDialog/AdvancedPreferences
can't?

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

It can not. But I prefer to be reinsured in this case:
AboutDialog/AdvancedPreferences just show a number to a user, she wouldn't die
if she will see -1 instead of 123, but here application fill break if it will
not get up to date assets.

     }
   }
 
   @Override
   protected void onStart()
   {
     super.onStart();
     AdblockPlus application = AdblockPlus.getApplication();
     application.startEngine();
     application.startInteractive();
   }
 
   @Override
   public void onResume()

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

Another long method, same comment as for AdvancedPreferences.onCreateDialog().

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

Same comment :) See code comments - they are for you.

   {
     super.onResume();
     SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this);
 
     final AdblockPlus application = AdblockPlus.getApplication();
 
     // Construct subscription list
     RefreshableListPreference subscriptionList = (RefreshableListPreference) findPreference(getString(R.string.pref_subscription));
     List<Subscription> subscriptions = application.getSubscriptions();
     String[] entries = new String[subscriptions.size()];
@@ skipping 20 matching lines @@
       if (offer != null)
       {
         subscriptionList.setValue(offer.url);
         application.setSubscription(offer);
         new AlertDialog.Builder(this).setTitle(R.string.app_name).setMessage(String.format(getString(R.string.msg_subscription_offer, offer.title))).setIcon(android.R.drawable.ic_dialog_info)
             .setPositiveButton(R.string.ok, null).create().show();
       }
     }
 
     // Enable manual subscription refresh
-    subscriptionList.setOnRefreshClickListener(new View.OnClickListener() {

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

Brace on its own line please.

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

Done.

+    subscriptionList.setOnRefreshClickListener(new View.OnClickListener()
+    {
       @Override
       public void onClick(View v)
       {
         application.refreshSubscription();
       }
     });
 
     // Set subscription status message
     if (subscriptionSummary != null)
       subscriptionList.setSummary(subscriptionSummary);
     else
       setPrefSummary(subscriptionList);
 
     // Time to start listening for events
     registerReceiver(receiver, new IntentFilter(AdblockPlus.BROADCAST_SUBSCRIPTION_STATUS));
     registerReceiver(receiver, new IntentFilter(AdblockPlus.BROADCAST_FILTER_MATCHES));
     registerReceiver(receiver, new IntentFilter(ProxyService.BROADCAST_STATE_CHANGED));
     registerReceiver(receiver, new IntentFilter(ProxyService.BROADCAST_PROXY_FAILED));
 
     final String url = current;
 
     // Initialize subscription verification
-    (new Thread() {

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

Brace on its own line please.

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

Done.

+    (new Thread()
+    {
       @Override
       public void run()
       {
         if (!application.verifySubscriptions())
         {
           Subscription subscription = application.getSubscription(url);
           application.setSubscription(subscription);
         }
       }
     }).start();
 
     // Check if service is running and update UI accordingly
     boolean enabled = prefs.getBoolean(getString(R.string.pref_enabled), false);
     if (enabled && !isServiceRunning())
     {
       setEnabled(false);
-      enabled = false;

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

enabled isn't read past this point, Setting it to false here is not necessary

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

Done.

     }
     // Run service if this is first application run
     else if (!enabled && firstRun)
     {
       startService(new Intent(this, ProxyService.class));
       setEnabled(true);
     }
 
     // Process screen rotation
     if (configurationMsg != null)
@@ skipping 77 matching lines @@
   private void copyAssets()
   {
     AssetManager assetManager = getAssets();
     String[] files = null;
     try
     {
       files = assetManager.list("install");
     }
     catch (IOException e)
     {
-      Log.e(TAG, e.getMessage());

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

Why not pass e as the third parameter, like below?

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

Done.

+      Log.e(TAG, "Failed to get assets list", e);
     }
     for (int i = 0; i < files.length; i++)
     {
-      InputStream in = null;

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

in and out can be declared in the try block, they are not referenced outside it.

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

Done.

-      OutputStream out = null;
       try
       {
         Log.d(TAG, "Copy: install/" + files[i]);
-        in = assetManager.open("install/" + files[i]);
-        out = openFileOutput(files[i], MODE_PRIVATE);
+        InputStream in = assetManager.open("install/" + files[i]);
+        OutputStream out = openFileOutput(files[i], MODE_PRIVATE);
         byte[] buffer = new byte[1024];
         int read;
         while ((read = in.read(buffer)) != -1)
         {
           out.write(buffer, 0, read);
         }
         in.close();
-        in = null;

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

You don't need to set in/out to null, they aren't used anymore and will go out
of scope very soon.

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

Done.

         out.flush();
         out.close();
-        out = null;
-
       }
       catch (Exception e)
       {
         Log.e(TAG, "Asset copy error", e);
       }
     }
   }
 
   /**
    * Redirects user to configuration URL.
    */
   public void onHelp(View view)
   {
     Uri uri = Uri.parse(getString(R.string.configuring_url));
-    final Intent intent = new Intent(Intent.ACTION_VIEW, uri);

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

Why make intent final?

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

No reason, just copied url opening code from sample.

+    Intent intent = new Intent(Intent.ACTION_VIEW, uri);
     startActivity(intent);
   }
 
   /**
    * Shows about dialog.
    */
   public void onAbout(View view)
   {
     aboutDialog = new AboutDialog(this);
-    aboutDialog.setOnDismissListener(new OnDismissListener() {
+    aboutDialog.setOnDismissListener(new OnDismissListener()
+    {
 
       @Override
       public void onDismiss(DialogInterface dialog)
       {
         showAbout = false;

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

Naming nit pick: This sounds imperative, how about "aboutShown"?

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

For me name is correct - it flags if have to show about dialog on the next run.

         aboutDialog = null;
       }
     });
     showAbout = true;
     aboutDialog.show();
   }
 
   @Override
   public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key)
   {
     if (getString(R.string.pref_enabled).equals(key))
     {
       boolean enabled = sharedPreferences.getBoolean(key, false);
-      if (enabled && !isServiceRunning())
+      boolean serviceRunning = isServiceRunning();
+      if (enabled && !serviceRunning)
         startService(new Intent(this, ProxyService.class));
-      else if (!enabled && isServiceRunning())

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

I'd feel safer is isServiceRunning() was called just once.

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

Done.

+      else if (!enabled && serviceRunning)
         stopService(new Intent(this, ProxyService.class));
     }
     if (getString(R.string.pref_subscription).equals(key))
     {
       String current = sharedPreferences.getString(key, null);
       AdblockPlus application = AdblockPlus.getApplication();
       Subscription subscription = application.getSubscription(current);
       application.setSubscription(subscription);
     }
     super.onSharedPreferenceChanged(sharedPreferences, key);
   }
 
   private void showConfigurationMsg(String message)
   {
     TextView msg = (TextView) findViewById(R.id.txt_configuration);
     msg.setText(message);
     msg.setVisibility(View.VISIBLE);
     configurationMsg = message;
   }
 
   private void hideConfigurationMsg()
   {
     if (configurationMsg == null)
       return;
     TextView msg = (TextView) findViewById(R.id.txt_configuration);
     msg.setVisibility(View.GONE);
     configurationMsg = null;
   }
 
-  private BroadcastReceiver receiver = new BroadcastReceiver() {

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

Brace on a new line please.

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

Done.

+  private BroadcastReceiver receiver = new BroadcastReceiver()
+  {
     @Override
-    public void onReceive(final Context context, Intent intent)

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

Why make context final?

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

It was previously used in a new thread, fixed.

+    public void onReceive(Context context, Intent intent)
     {
       String action = intent.getAction();
       Bundle extra = intent.getExtras();
       if (action.equals(ProxyService.BROADCAST_STATE_CHANGED))
       {
         if (extra.getBoolean("enabled"))
         {
           // If service is enabled in manual mode, show configuration message
           if (extra.getBoolean("manual"))
           {
@@ skipping 14 matching lines @@
       if (action.equals(ProxyService.BROADCAST_PROXY_FAILED))
       {
         String msg = extra.getString("msg");
         new AlertDialog.Builder(Preferences.this).setTitle(R.string.error).setMessage(msg).setIcon(android.R.drawable.ic_dialog_alert).setPositiveButton(R.string.ok, null).create().show();
         setEnabled(false);
       }
       if (action.equals(AdblockPlus.BROADCAST_SUBSCRIPTION_STATUS))
       {
         final String text = extra.getString("text");
         final long time = extra.getLong("time");
-        runOnUiThread(new Runnable() {
+        runOnUiThread(new Runnable()
+        {
           public void run()
           {
             setSubscriptionStatus(text, time);
           }
         });
       }
     }
   };
 
   /**
@@ skipping 48 matching lines @@
 
   @Override
   protected void onSaveInstanceState(Bundle outState)
   {
     outState.putString("subscriptionSummary", subscriptionSummary);
     outState.putString("configurationMsg", configurationMsg);
     outState.putBoolean("showAbout", showAbout);
     super.onSaveInstanceState(outState);
   }
 }