Issue 5235 - Refactoring on SharedPrefs

adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/util/SharedPrefsUtils.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 package org.adblockplus.sbrowser.contentblocker.util;
 
 import android.content.Context;
 import android.content.SharedPreferences;
 import android.preference.PreferenceManager;
 
 import java.util.Set;
 
 public class SharedPrefsUtils
 {
 
   public static void putBoolean(Context context, int keyResId, boolean value)

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

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.

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

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.

   {
     final SharedPreferences.Editor editor = getDefaultSharedPreferences(context).edit();
     editor.putBoolean(context.getString(keyResId), value).apply();
   }
 
   public static boolean getBoolean(Context context, int keyResId, boolean defValue)
   {
     final SharedPreferences preferences = getDefaultSharedPreferences(context);
     try
     {
@@ skipping 67 matching lines @@
   {
     getDefaultSharedPreferences(context).registerOnSharedPreferenceChangeListener(
         new OnSharedPreferenceChangeListenerWrapper(listener)
     );
   }
 
   public static void unregisterOnSharedPreferenceChangeListener(Context context,
       OnSharedPreferenceChangeListener listener)
   {
     getDefaultSharedPreferences(context).unregisterOnSharedPreferenceChangeListener(
         new OnSharedPreferenceChangeListenerWrapper(listener)

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

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]

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

     );
   }
 
   private static SharedPreferences getDefaultSharedPreferences(Context context)
   {
     return PreferenceManager.getDefaultSharedPreferences(context.getApplicationContext());
   }
 
   private static class OnSharedPreferenceChangeListenerWrapper
       implements SharedPreferences.OnSharedPreferenceChangeListener
   {
 
     private final OnSharedPreferenceChangeListener listener;
 
     OnSharedPreferenceChangeListenerWrapper(OnSharedPreferenceChangeListener listener) {
       this.listener = listener;
     }
 
     @Override
     public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key)
     {
       this.listener.onSharedPreferenceChanged(key);
     }
+
+    @Override
+    public boolean equals(Object obj)
+    {
+      if (this == obj)
+      {
+        return true;
+      }
+      else if (obj instanceof OnSharedPreferenceChangeListenerWrapper)
+      {
+        return this.listener.equals(((OnSharedPreferenceChangeListenerWrapper) obj).listener);
+      }
+      return false;
+    }
+
+    @Override
+    public int hashCode()
+    {
+      return this.listener.hashCode();
+    }
   }
 
   public interface OnSharedPreferenceChangeListener
   {
     void onSharedPreferenceChanged(String key);
   }
 }