Issue 5235 - Refactoring on SharedPrefs

Issue 5235 - Refactoring on SharedPrefs

[anton, 2017-05-29 05:33:04]

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
(right):

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:229:
final boolean wifiOnly = "1".equals(SharedPrefsUtils.getString(
why boolean variable is stored as string? wouldn't it be better to declare final
String wifiOnlyDefault="1"

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java
(right):

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:29:
public static void putBoolean(Context context, int keyResId, boolean value)
general suggest for all similar methods.

I's assumed only resource id to be used with this helper.
However we can have some `static final String KEY=".."` to be used (now or
later) somewhere.

So the suggestion is to provide methods that accept `String key` instead of `int
resourceId` and create convenience methods with similar signatures but accepting
`int resourceId` that just forward to according method with `String key`. Unless
we're absolutely sure we don't need and will never need `String key` methods
(which is less likely). 

Not insisting on that though.

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:117:
new OnSharedPreferenceChangeListenerWrapper(listener)
Creating new wrapper while unregistering looks strange. While trying to
unregister it tries to find already registered listener but we're creating new
one instead.
And it depends on android implementation if it's searched with `==` or
`equals()`. In the first case we will just leak registered listener (wrapper) as
it will be not found.
In the second case it depends on wrapper `equals()`impl - if it's written as
actually comparing wrapped interface then it will work.

Can you check it?

[diegocarloslima, 2017-05-31 14:32:09]

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
(right):

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:229:
final boolean wifiOnly = "1".equals(SharedPrefsUtils.getString(
On 2017/05/29 05:33:03, anton wrote:
> why boolean variable is stored as string? wouldn't it be better to declare
final
> String wifiOnlyDefault="1"

This is because the preference value is stored by a ListPreference in
preferences_main.xml which entryValues are numeric. I think that it was
implemented this way so it would support adding a 3rd option in the future. But
even if we change this, it wouldn't be in the scope of this issue.

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java
(right):

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:29:
public static void putBoolean(Context context, int keyResId, boolean value)
On 2017/05/29 05:33:04, anton wrote:
> general suggest for all similar methods.
> 
> I's assumed only resource id to be used with this helper.
> However we can have some `static final String KEY=".."` to be used (now or
> later) somewhere.
> 
> So the suggestion is to provide methods that accept `String key` instead of
`int
> resourceId` and create convenience methods with similar signatures but
accepting
> `int resourceId` that just forward to according method with `String key`.
Unless
> we're absolutely sure we don't need and will never need `String key` methods
> (which is less likely). 
> 
> Not insisting on that though.

Well for instance, the goal was to standardize the storage and retrieval of
preferences and for now, I think is better to only accept string resources so we
would avoid boilerplate code for retrieving the key from a resource. Also, all
keys are conveniently stored in the same file, avoiding key duplication. But if
we ever feel the need to add a method that accepts a `String key` as parameter,
we could do so... I just think that we shouldn't add unused method signatures
for now.

https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:117:
new OnSharedPreferenceChangeListenerWrapper(listener)
On 2017/05/29 05:33:04, anton wrote:
> Creating new wrapper while unregistering looks strange. While trying to
> unregister it tries to find already registered listener but we're creating new
> one instead.
> And it depends on android implementation if it's searched with `==` or
> `equals()`. In the first case we will just leak registered listener (wrapper)
as
> it will be not found.
> In the second case it depends on wrapper `equals()`impl - if it's written as
> actually comparing wrapped interface then it will work.
> 
> Can you check it?

Really nice catch! I totally overlooked this. I created this wrapper to avoid
exposing the SharedPreferences object in the callback, so the storage and
retrieval of preferences would always be handled by the utility class. I have
checked and internally is used a WeakHashMap, so overriding the wrapper's equals
and hashcode will suffice. I'll fix this and submit a patch

[anton, 2017-06-02 07:21:44]

On 2017/05/31 14:32:09, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
> (right):
> 
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:229:
> final boolean wifiOnly = "1".equals(SharedPrefsUtils.getString(
> On 2017/05/29 05:33:03, anton wrote:
> > why boolean variable is stored as string? wouldn't it be better to declare
> final
> > String wifiOnlyDefault="1"
> 
> This is because the preference value is stored by a ListPreference in
> preferences_main.xml which entryValues are numeric. I think that it was
> implemented this way so it would support adding a 3rd option in the future.
But
> even if we change this, it wouldn't be in the scope of this issue.
> 
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java
> (right):
> 
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:29:
> public static void putBoolean(Context context, int keyResId, boolean value)
> On 2017/05/29 05:33:04, anton wrote:
> > general suggest for all similar methods.
> > 
> > I's assumed only resource id to be used with this helper.
> > However we can have some `static final String KEY=".."` to be used (now or
> > later) somewhere.
> > 
> > So the suggestion is to provide methods that accept `String key` instead of
> `int
> > resourceId` and create convenience methods with similar signatures but
> accepting
> > `int resourceId` that just forward to according method with `String key`.
> Unless
> > we're absolutely sure we don't need and will never need `String key` methods
> > (which is less likely). 
> > 
> > Not insisting on that though.
> 
> Well for instance, the goal was to standardize the storage and retrieval of
> preferences and for now, I think is better to only accept string resources so
we
> would avoid boilerplate code for retrieving the key from a resource. Also, all
> keys are conveniently stored in the same file, avoiding key duplication. But
if
> we ever feel the need to add a method that accepts a `String key` as
parameter,
> we could do so... I just think that we shouldn't add unused method signatures
> for now.
> 
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:117:
> new OnSharedPreferenceChangeListenerWrapper(listener)
> On 2017/05/29 05:33:04, anton wrote:
> > Creating new wrapper while unregistering looks strange. While trying to
> > unregister it tries to find already registered listener but we're creating
new
> > one instead.
> > And it depends on android implementation if it's searched with `==` or
> > `equals()`. In the first case we will just leak registered listener
(wrapper)
> as
> > it will be not found.
> > In the second case it depends on wrapper `equals()`impl - if it's written as
> > actually comparing wrapped interface then it will work.
> > 
> > Can you check it?
> 
> Really nice catch! I totally overlooked this. I created this wrapper to avoid
> exposing the SharedPreferences object in the callback, so the storage and
> retrieval of preferences would always be handled by the utility class. I have
> checked and internally is used a WeakHashMap, so overriding the wrapper's
equals
> and hashcode will suffice. I'll fix this and submit a patch

LGTM

[jens, 2017-07-06 12:29:12]

On 2017/06/02 07:21:44, anton wrote:
> On 2017/05/31 14:32:09, diegocarloslima wrote:
> >
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> > File
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:229:
> > final boolean wifiOnly = "1".equals(SharedPrefsUtils.getString(
> > On 2017/05/29 05:33:03, anton wrote:
> > > why boolean variable is stored as string? wouldn't it be better to declare
> > final
> > > String wifiOnlyDefault="1"
> > 
> > This is because the preference value is stored by a ListPreference in
> > preferences_main.xml which entryValues are numeric. I think that it was
> > implemented this way so it would support adding a 3rd option in the future.
> But
> > even if we change this, it wouldn't be in the scope of this issue.
> > 
> >
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> > File
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:29:
> > public static void putBoolean(Context context, int keyResId, boolean value)
> > On 2017/05/29 05:33:04, anton wrote:
> > > general suggest for all similar methods.
> > > 
> > > I's assumed only resource id to be used with this helper.
> > > However we can have some `static final String KEY=".."` to be used (now or
> > > later) somewhere.
> > > 
> > > So the suggestion is to provide methods that accept `String key` instead
of
> > `int
> > > resourceId` and create convenience methods with similar signatures but
> > accepting
> > > `int resourceId` that just forward to according method with `String key`.
> > Unless
> > > we're absolutely sure we don't need and will never need `String key`
methods
> > > (which is less likely). 
> > > 
> > > Not insisting on that though.
> > 
> > Well for instance, the goal was to standardize the storage and retrieval of
> > preferences and for now, I think is better to only accept string resources
so
> we
> > would avoid boilerplate code for retrieving the key from a resource. Also,
all
> > keys are conveniently stored in the same file, avoiding key duplication. But
> if
> > we ever feel the need to add a method that accepts a `String key` as
> parameter,
> > we could do so... I just think that we shouldn't add unused method
signatures
> > for now.
> > 
> >
>
https://codereview.adblockplus.org/29449601/diff/29449602/adblockplussbrowser...
> >
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java:117:
> > new OnSharedPreferenceChangeListenerWrapper(listener)
> > On 2017/05/29 05:33:04, anton wrote:
> > > Creating new wrapper while unregistering looks strange. While trying to
> > > unregister it tries to find already registered listener but we're creating
> new
> > > one instead.
> > > And it depends on android implementation if it's searched with `==` or
> > > `equals()`. In the first case we will just leak registered listener
> (wrapper)
> > as
> > > it will be not found.
> > > In the second case it depends on wrapper `equals()`impl - if it's written
as
> > > actually comparing wrapped interface then it will work.
> > > 
> > > Can you check it?
> > 
> > Really nice catch! I totally overlooked this. I created this wrapper to
avoid
> > exposing the SharedPreferences object in the callback, so the storage and
> > retrieval of preferences would always be handled by the utility class. I
have
> > checked and internally is used a WeakHashMap, so overriding the wrapper's
> equals
> > and hashcode will suffice. I'll fix this and submit a patch
> 
> LGTM

LGTM