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

Issue 29674689: Issue 6289 - Fix OnSharedPreferenceChangeListener (Closed)

Created:
Jan. 19, 2018, 2:04 p.m. by jens
Modified:
Jan. 23, 2018, 3:44 p.m.
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
Jan. 19, 2018, 2:14 p.m. (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 ...
Jan. 19, 2018, 2:26 p.m. (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: ...
Jan. 19, 2018, 2:36 p.m. (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): > > ...
Jan. 19, 2018, 2:36 p.m. (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 ...
Jan. 19, 2018, 2:42 p.m. (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 ...
Jan. 22, 2018, 5:41 a.m. (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 ...
Jan. 22, 2018, 11:09 a.m. (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): > > ...
Jan. 23, 2018, 3:20 p.m. (2018-01-23 15:20:00 UTC) #8
anton
Jan. 23, 2018, 3:39 p.m. (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

Powered by Google App Engine
This is Rietveld