Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5444834596749312: Update the checkbox when enabling Acceptable Ads (Closed)

Nov. 27, 2013, 9:02 p.m. by Felix Dahlke
Nov. 28, 2013, 2:22 p.m.
Wladimir Palant
Andrey Novikov


I missed this earlier for some reason, but when testing with a fresh installation, I noticed that the Acceptable Ads checkbox is not actually ticked even though AAs get enabled. This fixes it.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -2 lines) Patch
M src/org/adblockplus/android/Preferences.java View 3 chunks +12 lines, -2 lines 1 comment Download


Total messages: 3
Felix Dahlke
Nov. 27, 2013, 9:04 p.m. (2013-11-27 21:04:08 UTC) #1
Wladimir Palant
As a short-term fix this is LGTM. Feel free to address the comment below, either ...
Nov. 28, 2013, 8:59 a.m. (2013-11-28 08:59:42 UTC) #2
Felix Dahlke
Nov. 28, 2013, 9:20 a.m. (2013-11-28 09:20:30 UTC) #3
On 2013/11/28 08:59:42, Wladimir Palant wrote:
> As a short-term fix this is LGTM. Feel free to address the comment below,
> as part of this review or separately.

Noticed that as well while working on this, was planning to do it in a separate

> However, if I understand the code here correctly then the underlying issue
> is state duplication. Preferences.onResume() should always read out acceptable
> ads state from libadblockplus and set the checkbox state accordingly.

Well, I agree that it's not great, but it's how the other preferences are
handled as well. I'd rather fix this for all of them at once and stick to the
pattern for now.

Here's a follow-up:

Powered by Google App Engine
This is Rietveld