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

Issue 29674689: Issue 6289 - Fix OnSharedPreferenceChangeListener (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by jens
Modified:
1 year, 5 months ago
Reviewers:
diegocarloslima, anton
Visibility:
Public.

Description

Issue 6289 - Fix OnSharedPreferenceChangeListener

Patch Set 1 : #

Total comments: 4

Patch Set 2 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -66 lines) Patch
M adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java View 1 3 chunks +31 lines, -27 lines 0 comments Download
M adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java View 1 1 chunk +6 lines, -39 lines 2 comments Download

Messages

Total messages: 9
jens
1 year, 6 months ago (2018-01-19 14:14:14 UTC) #1
diegocarloslima
https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java#newcode217 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:217: private SharedPrefsUtils.OnSharedPreferenceChangeListener listener = It's better to be final ...
1 year, 6 months ago (2018-01-19 14:26:11 UTC) #2
jens
https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java#newcode217 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:217: private SharedPrefsUtils.OnSharedPreferenceChangeListener listener = On 2018/01/19 14:26:10, diegocarloslima wrote: ...
1 year, 6 months ago (2018-01-19 14:36:07 UTC) #3
jens
On 2018/01/19 14:36:07, jens wrote: > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java > (right): > > ...
1 year, 6 months ago (2018-01-19 14:36:47 UTC) #4
diegocarloslima
On 2018/01/19 14:36:47, jens wrote: > On 2018/01/19 14:36:07, jens wrote: > > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java ...
1 year, 6 months ago (2018-01-19 14:42:50 UTC) #5
anton
https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java#newcode122 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:122: public abstract static class OnSharedPreferenceChangeListener are you sure that ...
1 year, 5 months ago (2018-01-22 05:41:13 UTC) #6
diegocarloslima
https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java#newcode122 adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:122: public abstract static class OnSharedPreferenceChangeListener On 2018/01/22 05:41:13, anton ...
1 year, 5 months ago (2018-01-22 11:09:26 UTC) #7
jens
On 2018/01/22 11:09:26, diegocarloslima wrote: > https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > (right): > > ...
1 year, 5 months ago (2018-01-23 15:20:00 UTC) #8
anton
1 year, 5 months ago (2018-01-23 15:39:50 UTC) #9
On 2018/01/22 11:09:26, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser...
> File
>
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java
> (right):
> 
>
https://codereview.adblockplus.org/29674689/diff/29674725/adblockplussbrowser...
>
adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:122:
> public abstract static class OnSharedPreferenceChangeListener
> On 2018/01/22 05:41:13, anton wrote:
> > are you sure that it's better to use class hierarchy with just 1 method body
> > definition instead of just interface? i think interface is preferred in this
> > case.
> 
> With an interface, we cannot make onSharedPreferenceChanged(SharedPreferences
> sharedPreferences, String key) final. The whole goal for this abstract class
is
> to hide the SharedPreferences instance through the callback. We want to hide
the
> SharedPreferences, to better indicate that the SharePreferences should be
> modified by using the SharedPrefsUtils methods

LGTM
Sign in to reply to this message.

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