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

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

Created:
Nov. 27, 2013, 9:02 p.m. by Felix Dahlke
Modified:
Nov. 28, 2013, 2:22 p.m.
Reviewers:
Wladimir Palant
CC:
Andrey Novikov
Visibility:
Public.

Description

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

Messages

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,
either
> as part of this review or separately.

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

> However, if I understand the code here correctly then the underlying issue
here
> 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:
https://trello.com/c/ckQs5RPz/345-don-t-keep-duplicate-state-in-the-preferences

Powered by Google App Engine
This is Rietveld