Issue 1108 - Support notifications

src/org/adblockplus/android/ProxyService.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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.android;
 
 import java.io.IOException;
 import java.net.InetAddress;
 import java.net.ServerSocket;
 import java.net.UnknownHostException;
 import java.util.Properties;
 
 import org.adblockplus.android.configurators.ProxyConfigurator;
 import org.adblockplus.android.configurators.ProxyConfigurators;
 import org.adblockplus.android.configurators.ProxyRegistrationType;
+import org.adblockplus.libadblockplus.Notification.Type;
 import org.apache.commons.lang.StringUtils;
 
 import sunlabs.brazil.server.Server;
 import sunlabs.brazil.util.Base64;
 import android.annotation.SuppressLint;
 import android.app.Notification;
 import android.app.NotificationManager;
 import android.app.PendingIntent;
 import android.app.Service;
 import android.content.BroadcastReceiver;
@@ skipping 576 matching lines @@
 
   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 */
+    public static final int DEFAULT_POLL_DELAY = 3 * 60; /* seconds */
+    public static final int DEFAULT_POLL_INTERVAL = 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.

+    private final int pollDelay;
+    private final int pollInterval;
 
     volatile boolean running = true;
 
-    public NotificationWatcher(final ProxyService proxyService, final int firstPollInterval,
-        final int nextPollIntervals)
+    public NotificationWatcher(final ProxyService proxyService, final int pollDelay,
+        final int pollInterval)
     {
       this.proxyService = proxyService;
-      this.firstPollInterval = Math.max(0, firstPollInterval);
-      this.nextPollIntervals = Math.max(0, nextPollIntervals);
+      this.pollDelay = Math.max(0, pollDelay);
+      this.pollInterval = Math.max(0, pollInterval);
     }
 
     public NotificationWatcher(final ProxyService proxyService)
     {
-      this(proxyService, DEFAULT_FIRST_POLL_INTERVAL, DEFAULT_NEXT_POLL_INTERVALS);
+      this(proxyService, DEFAULT_POLL_DELAY, DEFAULT_POLL_INTERVAL);
     }
 
     protected void stop()
     {
       this.running = false;
     }
 
     @Override
     public void run()
     {
-      if (this.firstPollInterval == 0)
+      if (this.pollDelay == 0)
       {
         this.running = false;
         return;
       }
 
-      long nextPoll = System.currentTimeMillis() + 1000L * this.firstPollInterval;
+      long nextPoll = System.currentTimeMillis() + 1000L * this.pollDelay;
 
       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).

+            org.adblockplus.libadblockplus.Notification notification =
+                AdblockPlus.getApplication().getNextNotificationToShow();
+
+            while (notification != null
+                && (notification.getType() == Type.INVALID || notification.getType() == Type.QUESTION))
             {
-              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();
-
+              notification = AdblockPlus.getApplication().getNextNotificationToShow();
+            }
+
+            if (notification != null)
+            {
               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();
+
+              notificationManager.notify(AdblockPlus.SERVER_NOTIFICATION_ID,
+                  new NotificationCompat.Builder(this.proxyService.getApplicationContext())
+                      .setSmallIcon(R.drawable.ic_stat_blocking)
+                      .setContentTitle(notification.getTitle())
+                      .setContentText(notification.getMessageString())
+                      .getNotification());
             }
-
-            Log.d(TAG, "Received " + idx + " new notifications");
           }
           catch (Exception ex)
           {
             Log.e(TAG, "Polling for notifications failed: " + ex.getMessage(), ex);
           }
 
-          if (this.nextPollIntervals == 0)
+          if (this.pollInterval == 0)
           {
             this.running = false;
           }
           else
           {
-            nextPoll = System.currentTimeMillis() + 1000L * this.nextPollIntervals;
+            nextPoll = System.currentTimeMillis() + 1000L * this.pollInterval;
           }
         }
       }
     }
   }
 }