Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(136)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 9 months ago by Felix Dahlke
Modified:
5 years, 9 months ago
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
5 years, 9 months ago (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 ...
5 years, 9 months ago (2013-11-28 08:59:42 UTC) #2
Felix Dahlke
5 years, 9 months ago (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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5