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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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 :)
Sign in to reply to this message.
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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
