Issue 1108 - Support notifications

src/org/adblockplus/android/ProxyService.java

Index: src/org/adblockplus/android/ProxyService.java
diff --git a/src/org/adblockplus/android/ProxyService.java b/src/org/adblockplus/android/ProxyService.java
index 0d136f69f0305ae9ab5b2e997677612457ff0120..f70ee4ec0d3da747706fdbb7bc9b169880c00e04 100755
--- a/src/org/adblockplus/android/ProxyService.java
+++ b/src/org/adblockplus/android/ProxyService.java
@@ -59,8 +59,6 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
 
   private static final boolean LOG_REQUESTS = false;
 
-  static final int ONGOING_NOTIFICATION_ID = R.string.app_name;
-
   private static final long POSITION_RIGHT = Build.VERSION.SDK_INT >= Build.VERSION_CODES.GINGERBREAD ? Long.MIN_VALUE : Long.MAX_VALUE;
 
   /**
@@ -79,7 +77,7 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
   public static final String BROADCAST_PROXY_FAILED = "org.adblockplus.android.PROXY_FAILURE";
 
   /**
-   * Common command bridge
+   * Proxy state changed
    */
   public static final String PROXY_STATE_CHANGED_ACTION = "org.adblockplus.android.PROXY_STATE_CHANGED";
 
@@ -93,6 +91,8 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
 
   private ProxyConfigurator proxyConfigurator = null;
 
+  private NotificationWatcher notificationWatcher = null;
+
   @SuppressLint("NewApi")
   @Override
   public void onCreate()
@@ -220,9 +220,12 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
 
     // Lock service
     hideIcon = prefs.getBoolean(getString(R.string.pref_hideicon), getResources().getBoolean(R.bool.def_hideicon));
-    startForeground(ONGOING_NOTIFICATION_ID, getNotification());
+    startForeground(AdblockPlus.ONGOING_NOTIFICATION_ID, getNotification());
 
     sendStateChangedBroadcast();
+
+    this.startNotificationWatcher();
+
     Log.i(TAG, "Service started");
   }
 
@@ -237,6 +240,8 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
   {
     super.onDestroy();
 
+    this.stopNotificationWatcher();
+
     unregisterReceiver(this.proxyReceiver);
     unregisterReceiver(this.proxyStateChangedReceiver);
 
@@ -500,7 +505,7 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
   {
     hideIcon = hide;
     final NotificationManager notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE);
-    notificationManager.notify(ONGOING_NOTIFICATION_ID, getNotification());
+    notificationManager.notify(AdblockPlus.ONGOING_NOTIFICATION_ID, getNotification());
   }
 
   public void sendStateChangedBroadcast()
@@ -563,7 +568,7 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
       if (intent != null && PROXY_STATE_CHANGED_ACTION.equals(intent.getAction()))
       {
         final NotificationManager notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE);
-        notificationManager.notify(ONGOING_NOTIFICATION_ID, getNotification());
+        notificationManager.notify(AdblockPlus.ONGOING_NOTIFICATION_ID, getNotification());
         ProxyService.this.sendStateChangedBroadcast();
       }
     }
@@ -596,4 +601,123 @@ public class ProxyService extends Service implements OnSharedPreferenceChangeLis
       }
     }
   }
+
+  private void startNotificationWatcher()
+  {
+    this.stopNotificationWatcher();
+
+    this.notificationWatcher = new NotificationWatcher(this);
+
+    final Thread thread = new Thread(this.notificationWatcher);
+    thread.setDaemon(true);
+    thread.start();
+  }
+
+  private void stopNotificationWatcher()
+  {
+    if (this.notificationWatcher != null)
+    {
+      this.notificationWatcher.stop();
+      this.notificationWatcher = null;
+    }
+  }
+
+  private static class NotificationWatcher implements Runnable

[Felix Dahlke, 2015/02/18 12:49:56]

This seems to be way too much code for what we want to do here - can't we just
use Handler.postDelayed? I'm pretty sure that should work even if the app is
killed.

If not, we could use AlarmManager.

[René Jeschke, 2015/02/18 13:13:02]

AlarmManager: Why use a system-wide, scheduling service for a simple delay?

Handler: This uses message passing to ensure things get executed on the EDT. We
used Handler for the no-traffic checks ... which never really worked (and I do
not want to handle this on the EDT).

I would really like to keep the Thread. And the LOC saving by using a Handler
isn't that high.

[Felix Dahlke, 2015/02/18 14:10:07]

Well, if this doesn't work reliably on the EDT, fair enough. Surprises me
frankly.

That leaves AlarmManager. That's indeed not much less code, and it _might_ get
fiddly to have an alarm that just runs code and doesn't bring any activity to
front.

Then I'll review the polling thread below.

+  {
+    public static final int DEFAULT_FIRST_POLL_INTERVAL = 3 * 60; /* seconds */
+    public static final int DEFAULT_NEXT_POLL_INTERVALS = 0; /* seconds */
+
+    private final ProxyService proxyService;
+    private final int firstPollInterval;

[Felix Dahlke, 2015/02/18 14:10:07]

How about initialDelay? It's not really an interval if it's just once :P

[René Jeschke, 2015/02/18 14:45:20]

Done.

+    private final int nextPollIntervals;

[Felix Dahlke, 2015/02/18 14:10:07]

Assuming firstPollInterval is renamed, this could just be pollingInterval.

[René Jeschke, 2015/02/18 14:45:20]

Done.

+
+    volatile boolean running = true;
+
+    public NotificationWatcher(final ProxyService proxyService, final int firstPollInterval,
+        final int nextPollIntervals)
+    {
+      this.proxyService = proxyService;
+      this.firstPollInterval = Math.max(0, firstPollInterval);
+      this.nextPollIntervals = Math.max(0, nextPollIntervals);
+    }
+
+    public NotificationWatcher(final ProxyService proxyService)
+    {
+      this(proxyService, DEFAULT_FIRST_POLL_INTERVAL, DEFAULT_NEXT_POLL_INTERVALS);
+    }
+
+    protected void stop()
+    {
+      this.running = false;
+    }
+
+    @Override
+    public void run()
+    {
+      if (this.firstPollInterval == 0)
+      {
+        this.running = false;
+        return;
+      }
+
+      long nextPoll = System.currentTimeMillis() + 1000L * this.firstPollInterval;
+
+      while (this.running)
+      {
+        try
+        {
+          Thread.sleep(250);
+        }
+        catch (InterruptedException ex)
+        {
+          break;
+        }
+
+        if (System.currentTimeMillis() >= nextPoll && this.running)
+        {
+          try
+          {
+            Log.d(TAG, "Polling for notifications");
+            org.adblockplus.libadblockplus.Notification notification = AdblockPlus.getApplication()
+                .getNotificationToShow();
+
+            int idx = 0;
+            while (notification != null)

[Felix Dahlke, 2015/02/18 14:10:07]

We should actually just show a single notification here, not all in a row.
GetNextNotificationToShow is going to return only one notification - the one
we're supposed to show. If there's a critical one but also some information
ones, we'll never see the latter really.

[René Jeschke, 2015/02/18 14:45:20]

It is guaranteed that there aren't any two notifications waiting? Why is the
method then called 'get_NEXT_NotificationToShow'?

But if you can guarantee that there will never be two notifications pending,
then I'll remove this. Can you? :-)

[Felix Dahlke, 2015/02/18 14:55:04]

There can be an arbitrary number of notifications waiting, but we always only
deliberately show one. Some notifications (e.g. emergency) always have
precedence and will keep showing up forever (even when internally marked as
shown). If there's just information notifications, we'll show one on each
startup, until there aren't any anymore.

Once we do this periodically rather then on startup, it's gonna get a bit more
complicated. We don't want to have emergency notifications show up at fixed
intervals e.g. We'll also have notifications that will be triggered by certain
filters matching (e.g. by visiting a site).

+            {
+              Notification notif = new NotificationCompat.Builder(
+                  this.proxyService.getApplicationContext())

[Felix Dahlke, 2015/02/18 14:10:07]

Shouldn't we pass the app's context here rather than the service's?

[René Jeschke, 2015/02/18 14:45:20]

I still am working on the service separation. The 'application' will then no
longer be available, and the Service already has a Context, so why use a foreign
context when we have a native one?

+                  .setSmallIcon(R.drawable.ic_stat_blocking)
+                  .setContentTitle(notification.getTitle())
+                  .setContentText(notification.getMessageString())
+                  .getNotification();
+
+              final NotificationManager notificationManager = (NotificationManager) this.proxyService
+                  .getSystemService(NOTIFICATION_SERVICE);
+              notificationManager.notify(AdblockPlus.GROUPED_NOTIFICATION_ID + idx, notif);
+
+              notification.markAsShown();

[Felix Dahlke, 2015/02/18 14:10:07]

We actually only need to call this for question notifications - which aren't
supported yet. Note that it's currently not technically possible to actually GET
a question notification from the server, they're only created internally on ABP
for Firefox/Chrome/Safari/Opera.

[René Jeschke, 2015/02/18 14:45:20]

Ah, Ok. I thought this has something to do with 'feedback'. Will remove this
then.

+
+              idx++;
+              notification = AdblockPlus.getApplication().getNotificationToShow();
+            }
+
+            Log.d(TAG, "Received " + idx + " new notifications");
+          }
+          catch (Exception ex)
+          {
+            Log.e(TAG, "Polling for notifications failed: " + ex.getMessage(), ex);
+          }
+
+          if (this.nextPollIntervals == 0)
+          {
+            this.running = false;
+          }
+          else
+          {
+            nextPoll = System.currentTimeMillis() + 1000L * this.nextPollIntervals;
+          }
+        }
+      }
+    }
+  }
 }