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
@@ skipping 41 matching lines @@
 public class ProxyService extends Service implements OnSharedPreferenceChangeListener
 {
   private static final String TAG = Utils.getTag(ProxyService.class);
 
   private static final String LOCALHOST = "127.0.0.1";
 
   private static final int[] PORT_VARIANTS = new int[] {-1, 2020, 3030, 4040, 5050, 6060, 7070, 9090, 1234, 12345, 4321, 0};
 
   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;
 
   /**
    * This flag indicates that this mobile device runs an Android version that allows the user to configure a http(s) proxy.
    */
   public static final boolean GLOBAL_PROXY_USER_CONFIGURABLE = Build.VERSION.SDK_INT >= 12; // Honeycomb 3.1
 
   /**
    * Broadcasted when service starts or stops.
    */
   public static final String BROADCAST_STATE_CHANGED = "org.adblockplus.android.SERVICE_STATE_CHANGED";
 
   /**
    * Broadcasted if proxy fails to start.
    */
   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";
 
   boolean hideIcon;
 
   protected ProxyServer proxy = null;
 
   protected int port;
 
   private final Properties proxyConfiguration = new Properties();
 
   private ProxyConfigurator proxyConfigurator = null;
 
+  private NotificationWatcher notificationWatcher = null;
+
   @SuppressLint("NewApi")
   @Override
   public void onCreate()
   {
     super.onCreate();
 
     StrictMode.setThreadPolicy(new StrictMode.ThreadPolicy.Builder()
         .permitAll()
         .penaltyLog()
         .build());
@@ skipping 107 matching lines @@
       proxy = new ProxyServer();
       proxy.logLevel = Server.LOG_LOG;
       proxy.setup(listen, proxyConfiguration.getProperty("handler"), proxyConfiguration);
       proxy.start();
     }
 
     prefs.registerOnSharedPreferenceChangeListener(this);
 
     // 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");
   }
 
   @Override
   public int onStartCommand(final Intent intent, final int flags, final int startId)
   {
     return START_STICKY;
   }
 
   @Override
   public void onDestroy()
   {
     super.onDestroy();
 
+    this.stopNotificationWatcher();
+
     unregisterReceiver(this.proxyReceiver);
     unregisterReceiver(this.proxyStateChangedReceiver);
 
     if (this.proxyConfigurator != null)
     {
       this.proxyConfigurator.unregisterProxy();
       this.proxyConfigurator.shutdown();
     }
 
     sendBroadcast(new Intent(BROADCAST_STATE_CHANGED).putExtra("enabled", false));
@@ skipping 243 matching lines @@
     builder.setContentText(getString(msgId, port));
     builder.setOngoing(true);
 
     return builder.getNotification();
   }
 
   public void setEmptyIcon(final boolean hide)
   {
     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()
   {
     final boolean manual = isManual();
     final Intent stateIntent = new Intent(BROADCAST_STATE_CHANGED)
         .putExtra("enabled", true)
         .putExtra("port", port)
         .putExtra("manual", manual);
 
@@ skipping 42 matching lines @@
    * </p>
    */
   private final BroadcastReceiver proxyStateChangedReceiver = new BroadcastReceiver()
   {
     @Override
     public void onReceive(final Context context, final Intent intent)
     {
       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();
       }
     }
   };
 
   final class ProxyServer extends Server
   {
     @Override
     public void close()
     {
@@ skipping 12 matching lines @@
 
     @Override
     public void log(final int level, final Object obj, final String message)
     {
       if (level <= logLevel)
       {
         Log.println(7 - level, obj != null ? obj.toString() : TAG, message);
       }
     }
   }
+
+  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;
+          }
+        }
+      }
+    }
+  }
 }