Issue 29674689: Issue 6289 - Fix OnSharedPreferenceChangeListener (Closed)
DescriptionIssue 6289 - Fix OnSharedPreferenceChangeListener
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 2
MessagesTotal messages: 9
https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:217: private SharedPrefsUtils.OnSharedPreferenceChangeListener listener = It's better to be final https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:26: import static android.content.SharedPreferences.*; We usually try to avoid the wildcard import
https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:217: private SharedPrefsUtils.OnSharedPreferenceChangeListener listener = On 2018/01/19 14:26:10, diegocarloslima wrote: > It's better to be final Acknowledged. https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java (right): https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:26: import static android.content.SharedPreferences.*; On 2018/01/19 14:26:11, diegocarloslima wrote: > We usually try to avoid the wildcard import Acknowledged.
On 2018/01/19 14:36:07, jens wrote: > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java > (right): > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:217: > private SharedPrefsUtils.OnSharedPreferenceChangeListener listener = > On 2018/01/19 14:26:10, diegocarloslima wrote: > > It's better to be final > > Acknowledged. > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > (right): > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:26: > import static android.content.SharedPreferences.*; > On 2018/01/19 14:26:11, diegocarloslima wrote: > > We usually try to avoid the wildcard import > > Acknowledged. Uploaded a new patch set
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... > > File > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java > > (right): > > > > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:217: > > private SharedPrefsUtils.OnSharedPreferenceChangeListener listener = > > On 2018/01/19 14:26:10, diegocarloslima wrote: > > > It's better to be final > > > > Acknowledged. > > > > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > > File > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java > > (right): > > > > > https://codereview.adblockplus.org/29674689/diff/29674693/adblockplussbrowser... > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:26: > > import static android.content.SharedPreferences.*; > > On 2018/01/19 14:26:11, diegocarloslima wrote: > > > We usually try to avoid the wildcard import > > > > Acknowledged. > > Uploaded a new patch set LGTM
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 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.
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
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 Yeah, nothing to add to that :)
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 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
