Code review

Code Review

CHANGES.rst

@@ -41,7 +41,7 @@ Improvements:
  - Better wording in notification for video call (#1421)
  - Improve widget banner (#2129)
  - Icon for Oreo (#2169)
- - Notification reliability and Messaging Style.
+ - Notification reliability and Messaging Style, with inlined reply (#2823, #1016).
  - Notification settings re-organization, added bing rule troubleshoot
  - Kotlin Code Improvement in VectorSettingsPreferencesFragment.kt
  - Remove redundant !! , Replace it with null safe operators in VectorSettingsPreferencesFragment.kt

vector/build.gradle

@@ -83,6 +83,7 @@ android {
             resValue "string", "git_revision_date", "\"${gitRevisionDate()}\""
             resValue "string", "git_branch_name", "\"${gitBranchName()}\""
             resValue "string", "build_number", rootProject.ext.buildNumberProp
+            buildConfigField "boolean", "LOW_PRIVACY_LOG_ENABLE", "false"
             minifyEnabled false
         }
 
@@ -91,6 +92,7 @@ android {
             resValue "string", "git_revision_date", "\"${gitRevisionDate()}\""
             resValue "string", "git_branch_name", "\"${gitBranchName()}\""
             resValue "string", "build_number", rootProject.ext.buildNumberProp
+            buildConfigField "boolean", "LOW_PRIVACY_LOG_ENABLE", "false"
             minifyEnabled false
             proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro'
         }

vector/src/app/java/im/vector/push/fcm/VectorFirebaseMessagingService.kt

@@ -1,4 +1,4 @@
-/**
+/*
  * Copyright 2019 New Vector Ltd
  *
  *
@@ -27,6 +27,7 @@ import com.google.firebase.messaging.RemoteMessage
 import com.google.gson.JsonParser
 import im.vector.BuildConfig
 import im.vector.Matrix
+import im.vector.R
 import im.vector.VectorApp
 import im.vector.activity.CommonActivityUtils
 import im.vector.notifications.NotifiableEventResolver
@@ -35,7 +36,6 @@ import im.vector.notifications.SimpleNotifiableEvent
 import im.vector.push.PushManager
 import im.vector.services.EventStreamService
 import org.matrix.androidsdk.MXSession
-import org.matrix.androidsdk.data.store.IMXStore
 import org.matrix.androidsdk.rest.model.Event
 import org.matrix.androidsdk.rest.model.bingrules.BingRule
 import org.matrix.androidsdk.util.Log
@@ -46,9 +46,9 @@ import org.matrix.androidsdk.util.Log
 class VectorFirebaseMessagingService : FirebaseMessagingService() {
 
     // Tells if the events service running state has been tested
-    private var mCheckLaunched: Boolean = false
+    private var mCheckLaunched = false
 
-    private val notifiableEventResolver: NotifiableEventResolver by lazy {
+    private val notifiableEventResolver by lazy {
         NotifiableEventResolver(this)
     }
 
@@ -64,18 +64,18 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
      */
     override fun onMessageReceived(message: RemoteMessage?) {
         if (message == null || message.data == null) {
-            Log.e(LOG_TAG, "## onMessageReceivedInternal() : received a null message or message with no data")
+            Log.e(LOG_TAG, "## onMessageReceived() : received a null message or message with no data")
             return
         }
-        if (BuildConfig.DEBUG) {
-            Log.i(LOG_TAG, "%%%%%%%% :" + message.data.toString())
-            Log.i(LOG_TAG, "%%%%%%%%  ## onMessageReceived() from FCM with priority " + message.priority)
+        if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) {
+            Log.i(LOG_TAG, "## onMessageReceived()" + message.data.toString())
+            Log.i(LOG_TAG, "## onMessageReceived() from FCM with priority " + message.priority)
         }
 
         //safe guard
         val pushManager = Matrix.getInstance(applicationContext).pushManager
         if (!pushManager.areDeviceNotificationsAllowed()) {
-            Log.i(LOG_TAG, "## onMessageReceivedInternal() : the notifications are disabled")
+            Log.i(LOG_TAG, "## onMessageReceived() : the notifications are disabled")
             return
         }
 
@@ -107,7 +107,7 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
      */
     private fun onMessageReceivedInternal(data: Map<String, String>, pushManager: PushManager) {
         try {
-            if (BuildConfig.DEBUG) {
+            if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) {
                 Log.i(LOG_TAG, "## onMessageReceivedInternal() : $data")
             }
             // update the badge counter
@@ -129,7 +129,6 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
         } catch (e: Exception) {
             Log.e(LOG_TAG, "## onMessageReceivedInternal() failed : " + e.message, e)
         }
-
     }
 
     //legacy code
@@ -160,15 +159,15 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
                     for (session in sessions) {
                         if (session.dataHandler?.store?.isReady == true) {
                             session.dataHandler.store?.getEvent(eventId, roomId)?.let {
-                                Log.e(LOG_TAG, "## onMessageReceivedInternal() : ignore the event " + eventId
+                                Log.e(LOG_TAG, "## isEventAlreadyKnown() : ignore the event " + eventId
                                         + " in room " + roomId + " because it is already known")
                                 return true
                             }
                         }
                     }
                 }
             } catch (e: Exception) {
-                Log.e(LOG_TAG, "## onMessageReceivedInternal() : failed to check if the event was already defined " + e.message, e)
+                Log.e(LOG_TAG, "## isEventAlreadyKnown() : failed to check if the event was already defined " + e.message, e)
             }
 
         }
@@ -178,7 +177,7 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
     private fun handleNotificationWithoutSyncingMode(data: Map<String, String>, session: MXSession?) {
 
         if (session == null) {
-            Log.e(LOG_TAG, "VectorFirebaseMessagingService: handleNotificationWithoutSyncingMode cannot find session")
+            Log.e(LOG_TAG, "## handleNotificationWithoutSyncingMode cannot find session")
             return
         }
         val notificationDrawerManager = VectorApp.getInstance().notificationDrawerManager
@@ -197,7 +196,7 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
                     session.myUserId,
                     eventId,
                     true, //It's an issue in this case, all event will bing even if expected to be silent.
-                    title = "New Event",
+                    title = getString(R.string.notification_unknown_new_event),
                     description = "",
                     type = null,
                     timestamp = System.currentTimeMillis(),
@@ -214,14 +213,14 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
             val event = parseEvent(data)
             if (event?.roomId == null) {
                 //unsupported event
-                Log.e(LOG_TAG, "VectorFirebaseMessagingService: Received an event with no room id")
+                Log.e(LOG_TAG, "Received an event with no room id")
                 return
             } else {
 
                 var notifiableEvent = notifiableEventResolver.resolveEvent(event, null, session.fulfillRule(event), session)
 
                 if (notifiableEvent == null) {
-                    Log.e(LOG_TAG, "VectorFirebaseMessagingService: Unsupported notifiable event ${eventId}")
+                    Log.e(LOG_TAG, "Unsupported notifiable event ${eventId}")
                 } else {
 
 
@@ -260,19 +259,6 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
         return roomName
     }
 
-//    private fun findSenderDisplayNameNameBestEffort(data: Map<String, String>, session: MXSession?, store: IMXStore?): String? {
-//        var roomName: String? = data?.get("room_name")
-//        val roomId = data?.get("room_id")
-//        if (null == roomName && null != roomId) {
-//            // Try to get the room name from our store
-//            if (store?.isReady == true) {
-//                val room = store.getRoom(roomId)
-//                roomName = room?.getRoomDisplayName(this)
-//            }
-//        }
-//        return roomName
-//    }
-
     /**
      * Try to create an event from the FCM data
      *

vector/src/main/java/im/vector/activity/VectorHomeActivity.java

@@ -162,9 +162,6 @@ public class VectorHomeActivity extends VectorAppCompatActivity implements Searc
 
     // jump to a group details sheet
     public static final String EXTRA_GROUP_ID = "VectorHomeActivity.EXTRA_GROUP_ID";
-//
-//    // When notification is tapped, to know if notification for messages should be cleared
-//    public static final String EXTRA_CLEAR_ALL_MESSAGES_NOTIFICATIONS = "VectorHomeActivity.EXTRA_CLEAR_ALL_MESSAGES_NOTIFICATIONS";
 
     // there are two ways to open an external link
     // 1- EXTRA_UNIVERSAL_LINK_URI : the link is opened as soon there is an event check processed (application is launched when clicking on the URI link)

vector/src/main/java/im/vector/activity/VectorRoomActivity.java

@@ -710,12 +710,6 @@ public void jumpToBottom() {
         mDefaultTopic = intent.getStringExtra(EXTRA_DEFAULT_TOPIC);
         mIsUnreadPreviewMode = intent.getBooleanExtra(EXTRA_IS_UNREAD_PREVIEW_MODE, false);
 
-        // the user has tapped on the "View" notification button
-//        if ((null != intent.getAction()) && (intent.getAction().startsWith(NotificationUtils.TAP_TO_VIEW_ACTION))) {
-//            // remove any pending notifications
-//            NotificationUtils.INSTANCE.cancelAllNotifications(this);
-//        }
-
         if (mIsUnreadPreviewMode) {
             Log.d(LOG_TAG, "Displaying " + roomId + " in unread preview mode");
         } else if (!TextUtils.isEmpty(mEventId) || (null != sRoomPreviewData)) {

vector/src/main/java/im/vector/notifications/NotifiableEventResolver.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright 2018 New Vector Ltd
+ * Copyright 2019 New Vector Ltd
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,13 +19,21 @@ import android.content.Context
 import android.support.v4.app.NotificationCompat
 import android.util.Log
 import android.widget.ImageView
+import im.vector.R
 import im.vector.util.RiotEventDisplay
 import org.matrix.androidsdk.MXSession
 import org.matrix.androidsdk.data.RoomState
 import org.matrix.androidsdk.data.store.IMXStore
 import org.matrix.androidsdk.rest.model.Event
+import org.matrix.androidsdk.rest.model.RoomMember
 import org.matrix.androidsdk.rest.model.bingrules.BingRule
 
+/**
+ * The notifiable event resolver is able to create a NotifiableEvent (view model for notifications) from an sdk Event.
+ * It is used as a bridge between the Event Thread and the NotificationDrawerManager.
+ * The NotifiableEventResolver is the only aware of session/store, the NotificationDrawerManager has no knowledge of that,
+ * this pattern allow decoupling between the object responsible of displaying notifications and the matrix sdk.
+ */
 class NotifiableEventResolver(val context: Context) {
 
     private val eventDisplay = RiotEventDisplay(context)
@@ -61,12 +69,12 @@ class NotifiableEventResolver(val context: Context) {
                             timestamp = event.originServerTs,
                             description = body,
                             soundName = bingRule?.notificationSound,
-                            title = "New Event",
+                            title = context.getString(R.string.notification_unknown_new_event),
                             type = event.type)
                 }
 
                 //Unsupported event
-                Log.i(LOG_TAG, "NotifiableEventResolver Received an unsupported event matching a bing rule")
+                Log.w(LOG_TAG, "NotifiableEventResolver Received an unsupported event matching a bing rule")
                 return null
             }
         }
@@ -81,7 +89,7 @@ class NotifiableEventResolver(val context: Context) {
         val room = store.getRoom(event.roomId /*roomID cannot be null (see Matrix SDK code)*/)
 
         if (room == null) {
-            Log.e(LOG_TAG, "## NotifiableEventResolver: Unable to resolve room for eventId [${event.eventId}] and roomID [${event.roomId}]")
+            Log.e(LOG_TAG, "## Unable to resolve room for eventId [${event.eventId}] and roomID [${event.roomId}]")
             return null
         } else {
 
@@ -90,7 +98,7 @@ class NotifiableEventResolver(val context: Context) {
                 return null
             }
 
-            val body = eventDisplay.getTextualDisplay(event, room.state)?.toString() ?: "New Event"
+            val body = eventDisplay.getTextualDisplay(event, room.state)?.toString() ?: context.getString(R.string.notification_unknown_new_event)
             val roomName = room.getRoomDisplayName(context)
             val senderDisplayName = room.state.getMemberName(event.sender) ?: event.sender ?: ""
 
@@ -130,22 +138,22 @@ class NotifiableEventResolver(val context: Context) {
     }
 
     private fun resolveStateRoomEvent(event: Event, bingRule: BingRule?, session: MXSession, store: IMXStore): NotifiableEvent? {
-        if ("invite" == event.contentAsJsonObject?.getAsJsonPrimitive("membership")?.asString) {
+        if ("invite" == event.contentAsJsonObject?.getAsJsonPrimitive(RoomMember.MEMBERSHIP_INVITE)?.asString) {
             val room = store.getRoom(event.roomId /*roomID cannot be null (see Matrix SDK code)*/)
-            val body = eventDisplay.getTextualDisplay(event, room.state)?.toString() ?: "Invite"
+            val body = eventDisplay.getTextualDisplay(event, room.state)?.toString() ?: context.getString(R.string.notification_new_invitation)
             return InviteNotifiableEvent(
                     session.myUserId,
                     eventId = event.eventId,
                     roomId = event.roomId,
                     timestamp = event.originServerTs,
                     noisy = bingRule?.notificationSound != null,
-                    title = "New Invitation",
+                    title = context.getString(R.string.notification_new_invitation),
                     description = body,
                     soundName = bingRule?.notificationSound,
                     type = event.type,
                     isPushGatewayEvent = false)
         } else {
-            Log.e(LOG_TAG, "## NotifiableEventResolver: unsupported notifiable event for eventId [${event.eventId}]")
+            Log.e(LOG_TAG, "## unsupported notifiable event for eventId [${event.eventId}]")
             //TODO generic handling?
         }
         return null