Issue 5444834596749312: Update the checkbox when enabling Acceptable Ads

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

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

Created: Nov. 27, 2013, 9:02 p.m.

Download [raw] [tar.bz2]

		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

Expand All Messages | Collapse All Messages

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

Expand All Messages | Collapse All Messages