Issue 98 - Use the libadblockplus update mechanism

src/org/adblockplus/android/Utils.java

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 org.adblockplus.android.updater.UpdaterActivity;
 import org.adblockplus.libadblockplus.JsValue;
 import org.adblockplus.libadblockplus.Subscription;
 import org.apache.commons.lang.StringUtils;
 
 import android.app.Notification;
+import android.app.NotificationManager;
 import android.app.PendingIntent;
 import android.content.Context;
 import android.content.Intent;
 import android.support.v4.app.NotificationCompat;
+import android.util.Log;
 
 public final class Utils
 {
+  private static final int UPDATE_NOTIFICATION_ID = R.string.app_name + 1;

[René Jeschke, 2014/09/26 10:24:05]

I'm not happy with having the ID here. This is a utilities class and it doesn't
feel right to have this ID here.

We could move this to AdblockPlus or ProxyService ... would make more sense
there IMO.

[Felix Dahlke, 2014/09/26 11:20:24]

It's a local constant - why make it a global one if it's only used here? We
generally try to declare variables as close to their use as possible.

[René Jeschke, 2014/09/26 11:31:50]

Well, then you have to move it into the method itself, as this value isn't even
used in the rest of the class.

[Felix Dahlke, 2014/09/26 11:39:05]

Yeah, I was pondering that, but a local final instead of a static final seems
rather unusual for constants in Java. No strong preference here, would you
prefer that?

[René Jeschke, 2014/09/26 11:59:17]

Would make more sense than declaring it class-wide. Also, as this is only used
in one location we wouldn't even need a variable here, but a final local
variable should be ok.

[Felix Dahlke, 2014/09/26 13:05:13]

Alright, made it a local constant. I disagree about inlining it, makes the code
harder to understand IMO.

[René Jeschke, 2014/09/26 13:10:52]

Righty right, thanks.

+
   private Utils()
   {
     //
   }
 
   public static String getTag(final Class<?> clazz)
   {
     return clazz.getSimpleName();
   }
 
   public static String capitalizeString(final String s)
   {
     if (s == null || s.length() == 0)
     {
       return "";
     }
 
     final char first = s.charAt(0);
 
     return Character.isUpperCase(first) ? s : Character.toUpperCase(first) + s.substring(1);
   }
 
-  protected static Notification createUpdateNotification(final Context context, final String url, final String error)
+  protected static void showUpdateNotification(final Context context, final String url,
+                                               final String error)
   {
-    final PendingIntent emptyIntent = PendingIntent.getActivity(context, 0, new Intent(), 0);
-
     final NotificationCompat.Builder builder = new NotificationCompat.Builder(context);
     builder.setContentTitle(context.getText(R.string.app_name));
     builder.setSmallIcon(R.drawable.ic_stat_warning);
-    builder.setWhen(System.currentTimeMillis());
     builder.setAutoCancel(true);
     builder.setOnlyAlertOnce(true);
+    final PendingIntent emptyIntent = PendingIntent.getActivity(context, 0, new Intent(), 0);
     builder.setContentIntent(emptyIntent);
 
-    if (url != null)
+    if (StringUtils.isNotEmpty(error))
+    {
+      Log.e(getTag(Utils.class), "Failed to check for updates: " + error);

[René Jeschke, 2014/09/26 10:24:05]

1. Please define this as a static variable as this call is costly [getTag(...)].
2. Feels wrong that 'Utils.class' reports update errors.

Maybe we can just remove the logging here? Or do we really gain anything from
logging the error?

[Felix Dahlke, 2014/09/26 11:20:24]

How else would we debug this in practice? The alternative would be to include
the message into the notification message, but I'm not sure we want to bother
the user with it. What do you think?

[René Jeschke, 2014/09/26 11:31:50]

Point is: the tag does not match the functionality. IMO a multi-purpose class
should not log stuff using it's class name. And to be more specific: such a
class should not log any errors.

We need to rework the logging nevertheless. We are spamming way to much crap
into the logfiles, too much noise.

So, why not just disable this message for now until we have some method for
better logging control? (As the whole updater also currently does nothing
anyways.)

[Felix Dahlke, 2014/09/26 11:39:05]

I would really like to be able to debug network problems we have in production
_somehow_, rather than swallowing the message entirely.

But you're right about this probably not really belonging into Utils. How about
I move the Log.e to AndroidUpdateCheckDoneCallback?

[René Jeschke, 2014/09/26 11:59:17]

Yes, that would make more sense IMO. And I will need to open an issue for
logging-refactoring.

[Felix Dahlke, 2014/09/26 13:05:13]

Done.

+      builder.setContentText(context.getString(R.string.msg_update_fail));
+    }
+    else if (StringUtils.isNotEmpty(url))
     {
       builder.setSmallIcon(R.drawable.ic_stat_download);
-
-      final Intent intent = new Intent(context, UpdaterActivity.class).addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+      final Intent intent = new Intent(context, UpdaterActivity.class)
+          .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
       intent.setAction("download");
       intent.putExtra("url", url);
-      final PendingIntent updateIntent = PendingIntent.getActivity(context, 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);
+      final PendingIntent updateIntent =
+          PendingIntent.getActivity(context, 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);
       builder.setContentIntent(updateIntent);
       builder.setContentText(context.getString(R.string.msg_update_available));
     }
-    else if (error != null)
-    {
-      // TODO Should we show error message to the user?
-      builder.setContentText(context.getString(R.string.msg_update_fail));
-    }
     else
     {
       builder.setContentText(context.getString(R.string.msg_update_missing));
     }
 
     final Notification notification = builder.getNotification();
-    return notification;
+    final NotificationManager notificationManager =
+        (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE);
+    notificationManager.notify(UPDATE_NOTIFICATION_ID, notification);
   }
 
   protected static void updateSubscriptionStatus(final Context context, final Subscription sub)
   {
     final JsValue jsDownloadStatus = sub.getProperty("downloadStatus");
     final String downloadStatus = jsDownloadStatus.isNull() ? "" : jsDownloadStatus.toString();
     final long lastDownload = sub.getProperty("lastDownload").asLong();
 
     String status = "synchronize_never";
     long time = 0;
@@ skipping 11 matching lines @@
       time = lastDownload;
       status = "synchronize_last_at";
     }
 
     context.sendBroadcast(new Intent(AdblockPlus.BROADCAST_SUBSCRIPTION_STATUS)
         .putExtra("url", sub.getProperty("url").toString())
         .putExtra("status", status)
         .putExtra("time", time * 1000L));
   }
 }