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

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

[Felix Dahlke, 2016-04-01 08:54:54]

Looks good all in all, just a few comments.

https://codereview.adblockplus.org/29339199/diff/29339200/AndroidManifest.xml
File AndroidManifest.xml (right):

https://codereview.adblockplus.org/29339199/diff/29339200/AndroidManifest.xml...
AndroidManifest.xml:5: android:versionName="1.0.2" >
Version bumps should go into a separate Noissue commit.

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right):

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:110:
this.checkAAStatus();
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()`.

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:140:
this.checkSetupStatus();
Same as above.

[René Jeschke, 2016-04-10 22:03:26]

https://codereview.adblockplus.org/29339199/diff/29339200/AndroidManifest.xml
File AndroidManifest.xml (right):

https://codereview.adblockplus.org/29339199/diff/29339200/AndroidManifest.xml...
AndroidManifest.xml:5: android:versionName="1.0.2" >
On 2016/04/01 08:54:53, Felix Dahlke wrote:
> Version bumps should go into a separate Noissue commit.

Done.

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right):

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:110:
this.checkAAStatus();
On 2016/04/01 08:54:53, Felix Dahlke wrote:
> 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()`.

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

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:140:
this.checkSetupStatus();
On 2016/04/01 08:54:53, Felix Dahlke wrote:
> Same as above.

See above.

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

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right):

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:110:
this.checkAAStatus();
On 2016/04/10 22:03:25, René Jeschke wrote:
> 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.

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.

> 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

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:05]

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right):

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:110:
this.checkAAStatus();
On 2016/04/11 13:53:48, Felix Dahlke wrote:
> On 2016/04/10 22:03:25, René Jeschke wrote:
> > 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.
> 
> 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.
> 
> > 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
> 
> 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.

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]

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right):

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:110:
this.checkAAStatus();
On 2016/04/12 09:10:04, René Jeschke wrote:
> On 2016/04/11 13:53:48, Felix Dahlke wrote:
> > On 2016/04/10 22:03:25, René Jeschke wrote:
> > > 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.
> > 
> > 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.
> > 
> > > 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
> > 
> > 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.
> 
> 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.

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]

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
File src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right):

https://codereview.adblockplus.org/29339199/diff/29339200/src/org/adblockplus...
src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:110:
this.checkAAStatus();
On 2016/04/12 11:44:46, Felix Dahlke wrote:
> On 2016/04/12 09:10:04, René Jeschke wrote:
> > On 2016/04/11 13:53:48, Felix Dahlke wrote:
> > > On 2016/04/10 22:03:25, René Jeschke wrote:
> > > > 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.
> > > 
> > > 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.
> > > 
> > > > 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
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

Done.

[Felix Dahlke, 2016-04-12 15:03:16]

Beautiful, LGTM.