Issue 2720 - [Adblocking settings] Add the other filter lists category

Issue 2720 - [Adblocking settings] Add the other filter lists category

[René Jeschke, 2015-07-17 19:10:28]

This looks heavier than it really is.

I included a few 'unrelated' changes because I had to touch most of the XMLs
anyway again.
 Those unrelated changes include the removal of 'android:enabled="false"'
attributes for our own PreferenceScreens, which speeds up the display of those
screens.

Those noise/unrelated changes should be few, though.

[Felix Dahlke, 2015-07-26 17:06:23]

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
File mobile/android/base/locales/en-US/android_strings.dtd (right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:651: <!ENTITY
abb_pref_category_further_blocking "More blocking options">
The term we sort of ended up agreeing on for this is "More blocking options".

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:651: <!ENTITY
abb_pref_category_further_blocking "More blocking options">
"further_blocking" disappeared as a term and presumably won't come back, so we
shouldn't use it in the code IMO. The most robust thing I can come up with would
be `more_subscriptions` :)

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:667: <!ENTITY
abb_pref_category_add_other_list "Add other blocking list">
We're not using the term "blocking list" at this point, so that should be
"filter list" for now.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:669: <!ENTITY
abb_disable_tracking "Disable Tracking">
Nit: I'd prefer to prefix these to make things more clear, e.g.
abb_list_disable_tracking

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:674: <!ENTITY
abb_block_list_url "www.yourURL.com/blockinglist.txt">
Nit: yoururl.com actually exists and is a questionable thing, how about using
example.org instead?

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:674: <!ENTITY
abb_block_list_url "www.yourURL.com/blockinglist.txt">
Nit: Call these keys "abb_filter_list_*"? We could also play it safe and always
call filter lists "subscriptions" in the code. It'd be consistent with the rest
of the ABP code base and would be resistant to naming changes.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:676: <!ENTITY
abb_block_list_adding "Adding new subscription...">
"Add new filter list" for consistency.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
File mobile/android/base/resources/layout/abb_custom_prefscreen.xml (right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/resources/layout/abb_custom_prefscreen.xml:12:
android:layout_marginStart="6dip"
Does marginStart/End actually work here? How I understood it,
android:supportsRtl needs to be enabled for this, but how I see it, it's not in
our case.

All that said, seeing how the margin in every direction is the same, we could
just use `android:layout_margin="6dip"`.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:64: public
static void handlerPost(Runnable runnable)
Nit: I always thought starting functions with side effects with a verb improves
readability, so I'd vote for `postToHandler` (or even just `post` if that's
clear enough). But that's just my two cents, up to you.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:25:
import
org.adblockplus.browser.SubscriptionPreferenceCategory.SubscriptionContainer;
May be non-trivial, but I think it'd make more sense to move this logic out of
SubscriptionPreferenceCategory, if we use it in a second place now. Could be a
follow-up.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:41:
public class OtherPreferenceGroup extends PreferenceGroup implements
Nit: `OtherPreferenceGroup` sounds pretty generic, but this is actually just for
"More blocking options", right? How about `MoreSubscriptionsPreferenceGroup` (in
the vein of another comment regarding this - the wording is not too stable at
this point and we should be careful when putting it in the code.)

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:73:
private static final String[] OTHER_BUILTINS =
Nit: Considering how they're used, `IGNORED_BUILTINS` maybe? That said, I think
we don't need to be this generic and can just specifically add the AA list to
IGNORED_URLS. The only reason why the AA list is not here is so it's easier for
users to disable the feature, even if they don't get "filter lists" at all -
don't think we will have any other such cases.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:193:
for (Entry<String, String> e :
this.activeSubscriptions.enabledSubscriptions.entrySet())
What about disabled subscriptions? Would it make sense to show them, but checked
off?

I know this is more of an academic point because I don't see how a user would
manage to disable a subscription with the current UI. Unchecking them will
remove them.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:40:
private static final int BUTTON_ENABLED_COLOR = 0xff0099cc;
As you know, I don't think we should have hard coded (i.e., in Java code) style
values - how about moving this to XML? Apart from all the mechanisms Android
provides to customise this for other environments and the benefits of having all
this stuff in one place, it's a much less risky change to modify it in an XML
file than to mess with the Java code, and such meddling won't distract from
actual behaviour changes when looking for regressions etc.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:55:
ADD_WHITELIST
It seems to me we're not using ADD_WHITELIST, nor do I understand what the
distinction is supposed to be. Technically they're the same thing, after all,
just subscriptions.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:146: url =
"http://" + url;
IMHO we should just consider URLs without the protocol invalid. I'm saying that
partly because SSL should be used where anyhow possible, but we have no idea if
it's going to work or not, and there won't be a guaranteed fallback to HTTP if
the target doesn't support HTTPS. So I'd just leave this out.

[René Jeschke, 2015-07-28 10:55:27]

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
File mobile/android/base/locales/en-US/android_strings.dtd (right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:651: <!ENTITY
abb_pref_category_further_blocking "More blocking options">
On 2015/07/26 17:06:21, Felix Dahlke wrote:
> "further_blocking" disappeared as a term and presumably won't come back, so we
> shouldn't use it in the code IMO. The most robust thing I can come up with
would
> be `more_subscriptions` :)

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:651: <!ENTITY
abb_pref_category_further_blocking "More blocking options">
On 2015/07/26 17:06:21, Felix Dahlke wrote:
> The term we sort of ended up agreeing on for this is "More blocking options".

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:667: <!ENTITY
abb_pref_category_add_other_list "Add other blocking list">
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> We're not using the term "blocking list" at this point, so that should be
> "filter list" for now.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:669: <!ENTITY
abb_disable_tracking "Disable Tracking">
On 2015/07/26 17:06:21, Felix Dahlke wrote:
> Nit: I'd prefer to prefix these to make things more clear, e.g.
> abb_list_disable_tracking

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:674: <!ENTITY
abb_block_list_url "www.yourURL.com/blockinglist.txt">
On 2015/07/26 17:06:21, Felix Dahlke wrote:
> Nit: Call these keys "abb_filter_list_*"? We could also play it safe and
always
> call filter lists "subscriptions" in the code. It'd be consistent with the
rest
> of the ABP code base and would be resistant to naming changes.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:674: <!ENTITY
abb_block_list_url "www.yourURL.com/blockinglist.txt">
On 2015/07/26 17:06:21, Felix Dahlke wrote:
> Nit: http://yoururl.com actually exists and is a questionable thing, how about
using
> http://example.org instead?

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/locales/en-US/android_strings.dtd:676: <!ENTITY
abb_block_list_adding "Adding new subscription...">
On 2015/07/26 17:06:21, Felix Dahlke wrote:
> "Add new filter list" for consistency.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
File mobile/android/base/resources/layout/abb_custom_prefscreen.xml (right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/base...
mobile/android/base/resources/layout/abb_custom_prefscreen.xml:12:
android:layout_marginStart="6dip"
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> Does marginStart/End actually work here? How I understood it,
> android:supportsRtl needs to be enabled for this, but how I see it, it's not
in
> our case.
> 
> All that said, seeing how the margin in every direction is the same, we could
> just use `android:layout_margin="6dip"`.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java (right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/AddOnBridge.java:64: public
static void handlerPost(Runnable runnable)
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> Nit: I always thought starting functions with side effects with a verb
improves
> readability, so I'd vote for `postToHandler` (or even just `post` if that's
> clear enough). But that's just my two cents, up to you.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:25:
import
org.adblockplus.browser.SubscriptionPreferenceCategory.SubscriptionContainer;
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> May be non-trivial, but I think it'd make more sense to move this logic out of
> SubscriptionPreferenceCategory, if we use it in a second place now. Could be a
> follow-up.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:41:
public class OtherPreferenceGroup extends PreferenceGroup implements
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> Nit: `OtherPreferenceGroup` sounds pretty generic, but this is actually just
for
> "More blocking options", right? How about `MoreSubscriptionsPreferenceGroup`
(in
> the vein of another comment regarding this - the wording is not too stable at
> this point and we should be careful when putting it in the code.)

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:73:
private static final String[] OTHER_BUILTINS =
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> Nit: Considering how they're used, `IGNORED_BUILTINS` maybe? That said, I
think
> we don't need to be this generic and can just specifically add the AA list to
> IGNORED_URLS. The only reason why the AA list is not here is so it's easier
for
> users to disable the feature, even if they don't get "filter lists" at all -
> don't think we will have any other such cases.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/OtherPreferenceGroup.java:193:
for (Entry<String, String> e :
this.activeSubscriptions.enabledSubscriptions.entrySet())
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> What about disabled subscriptions? Would it make sense to show them, but
checked
> off?
> 
> I know this is more of an academic point because I don't see how a user would
> manage to disable a subscription with the current UI. Unchecking them will
> remove them.

As ABP does not store custom, removed subscriptions, there's no 'easy' way to
achieve this. We would need to implement our on persistence layer on the Android
side, which I think is a bit overkill.

Also, if a user accidentially disables the list, and did not yet leave the
dialogue, it can be re-enabled. Otherwise it just gets lost.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:40:
private static final int BUTTON_ENABLED_COLOR = 0xff0099cc;
On 2015/07/26 17:06:23, Felix Dahlke wrote:
> As you know, I don't think we should have hard coded (i.e., in Java code)
style
> values - how about moving this to XML? Apart from all the mechanisms Android
> provides to customise this for other environments and the benefits of having
all
> this stuff in one place, it's a much less risky change to modify it in an XML
> file than to mess with the Java code, and such meddling won't distract from
> actual behaviour changes when looking for regressions etc.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:55:
ADD_WHITELIST
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> It seems to me we're not using ADD_WHITELIST, nor do I understand what the
> distinction is supposed to be. Technically they're the same thing, after all,
> just subscriptions.

Because the same dialogue will be needed to implement the whitelisting settings
page and I do not see a point in _not_ preparing for this case now.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:146: url =
"http://" + url;
On 2015/07/26 17:06:22, Felix Dahlke wrote:
> IMHO we should just consider URLs without the protocol invalid. I'm saying
that
> partly because SSL should be used where anyhow possible, but we have no idea
if
> it's going to work or not, and there won't be a guaranteed fallback to HTTP if
> the target doesn't support HTTPS. So I'd just leave this out.

Do you really want to force all Android users, to type the _whole_ link if it's
just plain http? It's already complicated enough to enter URLs, and enforcing
specifying a scheme, that is the default for URLs doesn't make sense to me.

Also, the hint text from the mockups does also not include 'http://' but starts
directly with the domain.

[Felix Dahlke, 2015-07-29 20:32:08]

Almost there, just a few more naming concerns.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:54:
ADD_BLOCKING_LIST,
Nit: As said elsewhere, we should avoid hard coding the term blocking list since
we don't actually use it (yet), nor are we sure we will do that.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:55:
ADD_WHITELIST
On 2015/07/28 10:55:27, René Jeschke wrote:
> On 2015/07/26 17:06:22, Felix Dahlke wrote:
> > It seems to me we're not using ADD_WHITELIST, nor do I understand what the
> > distinction is supposed to be. Technically they're the same thing, after
all,
> > just subscriptions.
> 
> Because the same dialogue will be needed to implement the whitelisting
settings
> page and I do not see a point in _not_ preparing for this case now.

Acknowledged. Nit then though: Maybe call it ADD_WHITELISTED_SITE for clarity's
sake.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:146: url =
"http://" + url;
On 2015/07/28 10:55:26, René Jeschke wrote:
> On 2015/07/26 17:06:22, Felix Dahlke wrote:
> > IMHO we should just consider URLs without the protocol invalid. I'm saying
> that
> > partly because SSL should be used where anyhow possible, but we have no idea
> if
> > it's going to work or not, and there won't be a guaranteed fallback to HTTP
if
> > the target doesn't support HTTPS. So I'd just leave this out.
> 
> Do you really want to force all Android users, to type the _whole_ link if
it's
> just plain http? It's already complicated enough to enter URLs, and enforcing
> specifying a scheme, that is the default for URLs doesn't make sense to me.
> 
> Also, the hint text from the mockups does also not include 'http://' but
starts
> directly with the domain. 

Hm, fair enough with the typing... Maybe it'd be slightly better to have a
selection of HTTP or HTTPS and then just enter the URL without protocol here.
Anyway, for the sake of progress, I think it's fine to do it like this for now.
If HTTP doesn't work, it normally gets redirected to HTTPS, so that should be
rather robust.

https://codereview.adblockplus.org/29322610/diff/29322805/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322805/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java:40:
public class MoreSubscriptionsPreferenceGroup extends PreferenceGroup implements
Naming nit again: Seems you're still mostly calling subcriptions "filter_list"
in the code. I think we should settle one one term here to avoid confusion. I'd
lean towards subscription because that's the technical term used under the hood,
regardless of what we call them on the UI.

[René Jeschke, 2015-07-30 09:26:44]

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:54:
ADD_BLOCKING_LIST,
On 2015/07/29 20:32:07, Felix Dahlke wrote:
> Nit: As said elsewhere, we should avoid hard coding the term blocking list
since
> we don't actually use it (yet), nor are we sure we will do that.

Done.

https://codereview.adblockplus.org/29322610/diff/29322701/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java:55:
ADD_WHITELIST
On 2015/07/29 20:32:07, Felix Dahlke wrote:
> On 2015/07/28 10:55:27, René Jeschke wrote:
> > On 2015/07/26 17:06:22, Felix Dahlke wrote:
> > > It seems to me we're not using ADD_WHITELIST, nor do I understand what the
> > > distinction is supposed to be. Technically they're the same thing, after
> all,
> > > just subscriptions.
> > 
> > Because the same dialogue will be needed to implement the whitelisting
> settings
> > page and I do not see a point in _not_ preparing for this case now.
> 
> Acknowledged. Nit then though: Maybe call it ADD_WHITELISTED_SITE for
clarity's
> sake.

Done.

https://codereview.adblockplus.org/29322610/diff/29322805/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java
(right):

https://codereview.adblockplus.org/29322610/diff/29322805/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java:40:
public class MoreSubscriptionsPreferenceGroup extends PreferenceGroup implements
On 2015/07/29 20:32:07, Felix Dahlke wrote:
> Naming nit again: Seems you're still mostly calling subcriptions "filter_list"
> in the code. I think we should settle one one term here to avoid confusion.
I'd
> lean towards subscription because that's the technical term used under the
hood,
> regardless of what we call them on the UI.

Done.

[Felix Dahlke, 2015-07-30 17:38:00]

LGTM, with the final nit fixed

https://codereview.adblockplus.org/29322610/diff/29323038/mobile/android/base...
File mobile/android/base/strings.xml.in (right):

https://codereview.adblockplus.org/29322610/diff/29323038/mobile/android/base...
mobile/android/base/strings.xml.in:555: <string
name="abb_list_disable_tracking">&abb_list_disable_tracking;</string>
Nit: I know I know, I suggested "list" myself here, but for consistency's sake,
this should IMO also be called "subscription" now :)