Issue 2513 - Adblock Browser does not display chosen filter lists as active right away

Issue 2513 - Adblock Browser does not display chosen filter lists as active right away

[anton, 2016-09-30 07:12:54]

https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:100:
if (preference instanceof CheckBoxPreference && newValue instanceof Boolean)
i'd prefer here to add spaces for each condition for better readability like
this:
`((preference instanceof CheckBoxPreference) && (newValue instanceof Boolean))`

not sure we have codestyle rule for it so it's just my suggestion

https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:192:
if(this.progressDialog != null)
space here?

[diegocarloslima, 2016-10-26 13:04:33]

https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:100:
if (preference instanceof CheckBoxPreference && newValue instanceof Boolean)
On 2016/09/30 07:12:53, anton wrote:
> i'd prefer here to add spaces for each condition for better readability like
> this:
> `((preference instanceof CheckBoxPreference) && (newValue instanceof
Boolean))`
> 
> not sure we have codestyle rule for it so it's just my suggestion

Actually, if you look below you will notice that this statement was already
present on this class just like this. Personally, I have no strong opinion about
adding parenthesis to this statement, but since it was already like this, I
think we should just keep it without them.

https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:192:
if(this.progressDialog != null)
On 2016/09/30 07:12:53, anton wrote:
> space here?

Acknowledged.

[anton, 2016-11-02 13:58:05]

On 2016/10/26 13:04:33, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
> File
>
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java
> (right):
> 
>
https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
>
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:100:
> if (preference instanceof CheckBoxPreference && newValue instanceof Boolean)
> On 2016/09/30 07:12:53, anton wrote:
> > i'd prefer here to add spaces for each condition for better readability like
> > this:
> > `((preference instanceof CheckBoxPreference) && (newValue instanceof
> Boolean))`
> > 
> > not sure we have codestyle rule for it so it's just my suggestion
> 
> Actually, if you look below you will notice that this statement was already
> present on this class just like this. Personally, I have no strong opinion
about
> adding parenthesis to this statement, but since it was already like this, I
> think we should just keep it without them.
> 
>
https://codereview.adblockplus.org/29345373/diff/29345374/mobile/android/thir...
>
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:192:
> if(this.progressDialog != null)
> On 2016/09/30 07:12:53, anton wrote:
> > space here?
> 
> Acknowledged.

LGTM

[Felix Dahlke, 2016-12-13 08:59:20]

Looks good, just one comment.

https://codereview.adblockplus.org/29345373/diff/29361550/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29345373/diff/29361550/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:33:
Preference.OnPreferenceChangeListener,
SubscriptionContainer.SubscriptionListener
FYI: We have so far (how I remember it) avoided to have classes implement
listener interfaces, and added a private field with a separate listener object
for that instead. IIRC that was because I found it harder to read the code this
way - it's just not as obvious which method implements which listener. I don't
feel that strong about it though, so if you do, I'm OK with it if you keep it
as-is.

[diegocarloslima, 2016-12-13 13:31:05]

https://codereview.adblockplus.org/29345373/diff/29361550/mobile/android/thir...
File
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java
(right):

https://codereview.adblockplus.org/29345373/diff/29361550/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/SubscriptionPreferenceCategory.java:33:
Preference.OnPreferenceChangeListener,
SubscriptionContainer.SubscriptionListener
On 2016/12/13 08:59:20, Felix Dahlke wrote:
> FYI: We have so far (how I remember it) avoided to have classes implement
> listener interfaces, and added a private field with a separate listener object
> for that instead. IIRC that was because I found it harder to read the code
this
> way - it's just not as obvious which method implements which listener. I don't
> feel that strong about it though, so if you do, I'm OK with it if you keep it
> as-is.

There are a few classes in ABB which implement listeners such as StartPane and
UrlInputDialog, but I agree that it's more readable if we define them through
inner classes +  class members

[Felix Dahlke, 2016-12-13 16:16:58]

LGTM!