openvehicles / Open-Vehicle-Android

Android App for OVMS
Other
55 stars 45 forks source link

fix push notifications #176

Open zaemsa opened 1 month ago

zaemsa commented 1 month ago
  1. The apiTask is canceled in the closeConnection() method, which stops the doInBackground method from syncing with the server. I have uncommented the line, and now it listens continuously to the server.

  2. The handleMessage method delegated the push notifications to the callback method onPushNotification, but the method body is empty. I added local push notifications, and now new notifications appear (tested on Android 14). In the future, it would be better to use Firebase instead.

zorgms commented 1 month ago

Works well in my fork. I used this PR with a switch in options menu for on/off

dexterbg commented 1 month ago

Both changes look like nonsense to me.

First of all, closeConnection() has the explicit purpose of shutting down the API task. It's called when…

These are all valid cases of terminating the task, and in all cases, a new task is created as needed and as soon as possible.

Second, ApiTask and ApiService are not responsible for or meant to process push notifications. Push notifications need to work independant of the App's background capability being used, and push notifications need to be received for all cars, not only the currently selected one.

Therefore, push notifications are received via the Firebase push notification service provided by Google. See receiver MyFirebaseMessagingService et al.

So, if you add creating system notifications from the TCP P messages, you will have them twice, unless you're using some third party server, which hasn't been configured for Firebase.

I'll keep this open for further discussion, if you think I'm wrong. Please then give details on your server and an example (with App log excerpts) of the current implementation failing.

Regards, Michael

zaemsa commented 1 month ago

Nonesense but at least a working version of the push notifications... I didn't read all the source code was just meant as a quick fix according to my observations at debugging.

The firebase service seems not to work for me on newer Android Version 14. Server is dexters. Cannot provide app log. The device ID is also completely in capital letters (I read somewhere that this could be a reason).

Actually I am not sure if this problem is faced by others too.

dexterbg commented 1 month ago

I'm also using Android 14 (and 13), and do not see any issues with push notifications on both devices. And yes, obviously I also use my server ;-)

If you see the notifications in the App's message window (btw: check the filter settings there, haven't tested yet if they still work correctly with the Kotlin build), but don't get the system notification, that means you didn't allow system notifications for the App.

Btw, this I learned: when you manually remove the App's background service notification, Android stops delivering all notifications from the App. That should be avoided. Or maybe there is a solution to this, possibly using different channels (?)

zaemsa commented 1 month ago

Thanks ;). But Notifications also do not appear neither in the message window (filters set + not set) . System notifications are allowed. I restarted the device and reconfigured the App but same behavior. The only notification I receive is when I turn on the option "enable background connection" to inform me the service is running. Its running on a s23.

I will test it on another device and see if the same problem occurs there.

zorgms commented 1 month ago
<!-- Only this application can receive the messages and registration result -->
    <permission
        android:name="com.openvehicles.OVMS.permission.C2D_MESSAGE"
        android:protectionLevel="signature" />

@dexterbg You have the right key to sign the app. That's why you get the messages, we with our own signature key don't get any messages. @zaemsa fixed it so to speak because no messages were arriving.

dexterbg commented 1 month ago

That's unfortunately the way, Firebase access grants work, but that doesn't affect normal users, or users of an official development build. It also won't affect custom builders running their own server & Firebase instance, those will simply replace the "google-services.json" file and grant Firebase access to their own App.

I'd be OK with adding this as a configurable "developer" option, off by default, with an explanation that this only is needed for developer/custom App builds, and also explaining that this will only work while the App is running or also has the background service enabled, and in any case only for the currently selected car.

zorgms commented 1 month ago

Adding "DEV" to the Version name to switch on this PR. Is this a way for you @dexterbg ?

    override fun onPushNotification(msgClass: Char, msgText: String?) {
        // This callback only receives MP push notifications for the currently selected vehicle.
        // See MyFirebaseMessagingService for system notification broadcasting.
        // add "DEV" like this android:versionName="4.7.1 EQ-DEV"
        // at AndroidManifest.xml to activate the Firebase Message Service on Developer Version
        val packageInfo = packageManager.getPackageInfo(packageName, 0)
        val versionName = packageInfo.versionName
        if (versionName.contains("DEV", ignoreCase = true)){
            val notificationManager = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
            createNotificationChannel()
            // Define the notification channel (required for Android O and above)
            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
                val channelName = getString(R.string.app_name)
                val channelDescription = "OVMS"
                val channel = NotificationChannel("your_channel_id", channelName, NotificationManager.IMPORTANCE_DEFAULT).apply {
                    description = channelDescription
                }
                notificationManager.createNotificationChannel(channel)
            }
            // Create the notification
            val notificationBuilder = NotificationCompat.Builder(this, "your_channel_id")
                .setSmallIcon(R.drawable.ic_service)
                .setContentTitle("OVMS")
                .setContentText(msgText ?: msgText)
                .setPriority(NotificationCompat.PRIORITY_DEFAULT)
                .setAutoCancel(true)

            // Show the notification
                notificationManager.notify(1, notificationBuilder.build())
        }
    }
glynhudson commented 1 month ago

I can confirm the standard firebase notifications work fine for me using the official build on Android 15

dexterbg commented 1 month ago

Coupling hidden functions to some obscure build code scheme, that even other developers first need to learn about, isn't good.

I'd rather see this as a checkbox in the options UI, so it's available and explained for anyone, and also placed near the background service option.

zorgms commented 1 month ago

image

Something like this?

dexterbg commented 1 month ago

Yes, plus explanation in the page footer.