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

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

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 60 matching lines @@
     super.onStart();
     EngineService.startService(this, this);
   }
 
   @Override
   protected void onStop()
   {
     super.onStop();
   }
 
-  private void checkForCompatibleSBrowser()
+  private void checkForCompatibleSBrowserAndProceed()
   {
     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()
+      this.checkAAStatusAndProceed();
+    }
+  }
+
+  private void checkAAStatusAndProceed()
   {
     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)
         .getBoolean(this.getString(R.string.key_application_activated), false);
 
     if (!applicationActivated)
     {
@@ skipping 18 matching lines @@
   @Override
   public void onEngineCreated(Engine engine, boolean success)
   {
     Log.d(TAG, "onEngineCreated: " + success);
     this.engine = success ? engine : null;
     if (this.progressDialog != null)
     {
       this.progressDialog.dismiss();
       this.progressDialog = null;
 
-      this.checkForCompatibleSBrowser();
+      this.checkForCompatibleSBrowserAndProceed();
     }
   }
 
   @Override
   public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key)
   {
     if (this.getString(R.string.key_automatic_updates).equals(key) && this.engine != null)
     {
       this.engine.connectivityChanged();
     }
@@ skipping 35 matching lines @@
         r.close();
       }
     }
     catch (IOException e)
     {
       Log.e(TAG, "Resource reading failed for: " + id, e);
       return "...";
     }
   }
 }