ABP/Android Manual proxy user notifications

ABP/Android Manual proxy user notifications

[Felix Dahlke, 2013-02-22 09:11:14]

I get the impression there's either some changes missing or some unrelated
changes have been included here, some things don't add up. Like where is the
nativeProxy field being renamed to hasNativeProxy?

Also, I don't understand what's the gist of this change, is it just refactoring?

http://codereview.adblockplus.org/9347203/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/9347203/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/ProxyService.java:151: // not used but left for
future reference
Why would we need that in the future? Even if, we could still get it from source
control.

[Andrey Novikov, 2013-02-22 09:24:57]

That's why I like visual diffs. :) Use side-by-side view and you will find
hasNativeProxy. It's not renamed, it was introduced long time before. In fact it
was introduced based on your review. :)

This is not the refactoring. the purpose of this change is to show user
notification that he needs to configure proxy manually and to make service
remove that notification as soon as proxy will be correctly set.

Before that we were "waiting for traffic" to remove this notification, because
we were unable to detect proxy settings change (this refers to unrooted 2.x
devices).

[Andrey Novikov, 2013-02-22 09:25:05]

http://codereview.adblockplus.org/9347203/diff/1/src/org/adblockplus/android/...
File src/org/adblockplus/android/ProxyService.java (right):

http://codereview.adblockplus.org/9347203/diff/1/src/org/adblockplus/android/...
src/org/adblockplus/android/ProxyService.java:151: // not used but left for
future reference
You asked me this already in previous reviews. :) There are two methods to read
current proxy settings, unused on looks more convenient but I am afraid to
switch to it because everything works now with the first method.

[Felix Dahlke, 2013-02-22 09:35:00]

On 2013/02/22 09:24:57, Andrey Novikov wrote:
> That's why I like visual diffs. :) Use side-by-side view and you will find
> hasNativeProxy. It's not renamed, it was introduced long time before. In fact
it
> was introduced based on your review. :)

I know that hasNativeProxy exists, I've just noted that the renaming from
nativeProxy to hasNativeProxy is not part of the review, so there's either
something missing or some unrelated parts in here. Or the state before this
review wouldn't have compiled.
 
> This is not the refactoring. the purpose of this change is to show user
> notification that he needs to configure proxy manually and to make service
> remove that notification as soon as proxy will be correctly set.
> 
> Before that we were "waiting for traffic" to remove this notification, because
> we were unable to detect proxy settings change (this refers to unrooted 2.x
> devices).

So it's a different method of discovering whether manual configuration is
necessary? Will this still work on unrooted 2.x devices?

[Felix Dahlke, 2013-02-22 10:30:58]

LGTM