Issue 3846 - Add notification for users using an old SBrowser version

src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java

Index: src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
diff --git a/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java b/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
index a2ef1491936baba1da434497142bc654cb80bddd..a7ca55b0fd0d0a9da503dbfb77a9eb9f2d394a19 100644
--- a/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
+++ b/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
@@ -29,7 +29,9 @@ import android.app.AlertDialog;
 import android.app.ProgressDialog;
 import android.content.DialogInterface;
 import android.content.DialogInterface.OnClickListener;
+import android.content.Intent;
 import android.content.SharedPreferences;
+import android.net.Uri;
 import android.os.Bundle;
 import android.preference.PreferenceActivity;
 import android.preference.PreferenceManager;
@@ -40,6 +42,7 @@ public class MainPreferences extends PreferenceActivity implements
     EngineService.OnEngineCreatedCallback, SharedPreferences.OnSharedPreferenceChangeListener
 {
   private static final String TAG = MainPreferences.class.getSimpleName();
+  private static final String SBROWSER_APP_ID = "com.sec.android.app.sbrowser";
   private ProgressDialog progressDialog = null;
   private Engine engine = null;
   private AlertDialog setupDialog = null;
@@ -75,6 +78,69 @@ public class MainPreferences extends PreferenceActivity implements
     super.onStop();
   }
 
+  private void checkForCompatibleSBrowser()
+  {
+    if (!Engine.hasCompatibleSBrowserInstalled(this.getApplicationContext()))
+    {
+      final AlertDialog d = new AlertDialog.Builder(this)
+          .setCancelable(false)
+          .setTitle(R.string.sbrowser_dialog_title)
+          .setMessage(Html.fromHtml(this.readTextFile(R.raw.sbrowser_dialog)))
+          .setNeutralButton(R.string.sbrowser_dialog_button, new OnClickListener()
+          {
+            @Override
+            public void onClick(DialogInterface dialog, int which)
+            {
+              try
+              {
+                startActivity(new Intent(Intent.ACTION_VIEW, Uri.parse("market://details?id="
+                    + SBROWSER_APP_ID)));
+              }
+              catch (final Throwable t)
+              {
+                startActivity(new Intent(Intent.ACTION_VIEW, Uri
+                    .parse("https://play.google.com/store/apps/details?id=" + SBROWSER_APP_ID)));
+              }
+            }
+          }).create();
+      d.show();
+    }
+    else
+    {
+      this.checkAAStatus();

[Felix Dahlke, 2016/04/01 08:54:53]

Unless I'm missing something, this has nothing to do with checking for the
SBrowser version. One simple change I would find better would be to return a
boolean here and then decide in the caller whether to call `checkAAStatus()`.

[René Jeschke, 2016/04/10 22:03:25]

Well, yeah, those methods aren't really named correctly. It's not only a check,
but also displaying of a dialogue and an ansynchronous chain (the dialogue
button calls the next 'checkXxx' method).

Thought about naming this something like 'stepXxx' or the like, but you know
that I suck at naming.

Returning a boolean would mean that I would need to duplicate the 'if' logic in
at least two places, and would result in ugly nested ifs. Would also, IMHO, be
more sensible to errors in the end.

So, to sum it up: those methods are a chain, that can be continued either via
the 'else' clause (if a check fails) or via a dialogue button.

I would love to hear any naming suggestions :D

[Felix Dahlke, 2016/04/11 13:53:48]

Well, all the logic would be in the caller and none in the `check*` methods. How
about adding a new function that performs all the checks? Then you can extract
the logic out of the caller but still have it all in one place and each function
has just one purpose. You could call that one `performStartupChecks` or
something.
I think if you leave the methods as they are it's hard, because both
`checkForCompatibleSBrowser` and `checkAAStatus` do more than one thing. Maybe
`checkForCompatibleSBrowserOrProceed` and `checkAAStatusOrProceed` respectively?
Sure, those names suck, but at least they sort of imply what's happening here.
But I'd be a bigger fan of making the methods do just one thing.

[René Jeschke, 2016/04/12 09:10:04]

Na, the thing is ... the method need to _exit_ to let the dialogue do its thing.
So you can not have all three checks in the same method, as you need to exit the
method when displaying a dialogue.

That's why those methods call the next method either when no dialogue needs to
be displayed or via the dialogue-onPress-callback.

[Felix Dahlke, 2016/04/12 11:44:46]

OK I see, didn't realise that `checkAAStatus` is calling `checkSetupStatus`,
that complicates things... I'm fine with keeping it as is with a slightly better
name. `check*AndProceed` is my best idea.

[René Jeschke, 2016/04/12 13:32:39]

Done.

+    }
+  }
+
+  private void checkAAStatus()
+  {
+    final SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this);
+    final String keyAaInfoShown = this.getString(R.string.key_aa_info_shown);
+    final boolean aaInfoShown = prefs.getBoolean(keyAaInfoShown, false);
+    if (!aaInfoShown)
+    {
+      final AlertDialog d = new AlertDialog.Builder(this)
+          .setCancelable(false)
+          .setTitle(R.string.aa_dialog_title)
+          .setMessage(Html.fromHtml(this.readTextFile(R.raw.aa_dialog)))
+          .setNeutralButton(R.string.aa_dialog_button, new OnClickListener()
+          {
+            @Override
+            public void onClick(DialogInterface dialog, int which)
+            {
+              prefs.edit()
+                  .putBoolean(keyAaInfoShown, true)
+                  .commit();
+              MainPreferences.this.checkSetupStatus();
+            }
+          }).create();
+      d.show();
+    }
+    else
+    {
+      this.checkSetupStatus();

[Felix Dahlke, 2016/04/01 08:54:53]

Same as above.

[René Jeschke, 2016/04/10 22:03:25]

See above.

+    }
+  }
+
   private void checkSetupStatus()
   {
     final boolean applicationActivated = PreferenceManager.getDefaultSharedPreferences(this)
@@ -110,32 +176,7 @@ public class MainPreferences extends PreferenceActivity implements
       this.progressDialog.dismiss();
       this.progressDialog = null;
 
-      final SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this);
-      final String keyAaInfoShown = this.getString(R.string.key_aa_info_shown);
-      final boolean aaInfoShown = prefs.getBoolean(keyAaInfoShown, false);
-      if (!aaInfoShown)
-      {
-        AlertDialog d = new AlertDialog.Builder(this)
-            .setCancelable(false)
-            .setTitle(R.string.aa_dialog_title)
-            .setMessage(Html.fromHtml(this.readTextFile(R.raw.aa_dialog)))
-            .setNeutralButton(R.string.aa_dialog_button, new OnClickListener()
-            {
-              @Override
-              public void onClick(DialogInterface dialog, int which)
-              {
-                prefs.edit()
-                    .putBoolean(keyAaInfoShown, true)
-                    .commit();
-                MainPreferences.this.checkSetupStatus();
-              }
-            }).create();
-        d.show();
-      }
-      else
-      {
-        this.checkSetupStatus();
-      }
+      this.checkForCompatibleSBrowser();
     }
   }