Implement new notification features

[mueller-ma]

• Parsing via FCM
• Parsing from JSON using the new payload field
• Image loading: URL, URL from Item state, base64 from Item state
• Notification list: Title
• Notification list: Show Image
• Notification: Title
• Notification: Actions
• Notification: onClick
• Notification: Show Image
• Rename severity to tag
• Check docs if anything is missing
• Hide notification
• referenceId

[maniac103]

Notification: onClick

@digitaldan How is onClickAction meant to be handled? Executing the action is clear, but should that be done in addition to or instead of opening the notification content in the lst?

[maniac103]

Also, how exactly is referenceId handled? From the cloud code I gather it's only used for collapse_key, but doesn't that only affect notifications which aren't delivered yet? That is, if we have shown a notification for reference ID X and another notification for X is sent via FCM, the collapse key won't change anything according to the docs:
A collapsible message is a message that may be replaced by a new message if it has yet to be delivered to the device.

AFAICT, that means we need to pass the reference ID via FCM payload, and need to do client side processing to remove older notifications for that reference ID.

[maniac103]

Also, what about hiding notifications? If the user chooses to hide notifications by tag or reference ID, will the cloud code send hideNotification messages for each respective notification, or will we need to do this client side? If the latter, does the hideNotification message provide the reference-id or tag fields?

[mueller-ma]

I tried to fix the notification trampoline, but now clicking the button of a ui:... action does nothing, even no log :/

[maniac103]

Since Android 12, the trampoline BroadcastReceiver -> Activity isn't allowed anymore, one must go to the activity from the notification directly. This means for UI actions you need a different PendingIntent. See docs here

[digitaldan]

Also, how exactly is referenceId handled? From the cloud code I gather it's only used for collapse_key,

Its both set as the "collapse_key" as well as is included in the payload , so you should have access to it already. I actually assumed it collapsed delivered notifications, but sounds like you can do that programmatically from the app?

Also, what about hiding notifications?

See my comment here, openhab/openhab-addons#16934 (comment)

How is onClickAction meant to be handled?

If the action is a ui: action, then it should open the app, command: actions should not open the app, just send the command. http actions should open the app with an embedded browser for the link (or you could just open the user's browser, i guess up to you, i chose to keep everything in the app on iOS)

[maniac103]

I actually assumed it collapsed delivered notifications, but sounds like you can do that programmatically from the app?

It can't, because FCM isn't what posts those notifications, so the app must be responsible for that. Is this different on iOS?

[digitaldan]

Is this different on iOS?

It is, the collapse key automatically removes / replaces the message with the same collapse id, although i might be able to do that strictly in the app if i wanted to, but this is what Apple recommends. On iOS the app has the opportunity to be called before a non-background notification is posted to the message center, allowing it to mutate the message (so add media for example). For the "hide notifications" feature, we use background messages (no title or message) , which do not get posted to the notification center by default and only the app gets them. This would be the equivalent to android, where the app would then be completely responsible for creating a visual notification based on meta data in the background message.

[maniac103]

Thinking about the reference-id handling, I think it should be sufficient to just use it instead of persistedId for notification ID generation. This should give us the expected behavior with regards to notification replacement and removal.
For 'remove by tag', we can query the notifications and remove them by group key.

[digitaldan]

I think it should be sufficient to just use it instead of persistedId for notification ID generation

Just FYi this field is only set if the rule or action sets it, so its not guaranteed right now to always be present. I could change that on the cloud server side (so generate a uuid if its not set) if thats a problem.

[mueller-ma]

This PR is ready to merge from my side. Hiding notifications is tracked in #3686.

My debug cert has been added to the Firebase project. When you send me the SHA-1 of your debug cert, I can add it to the app.

[maniac103]

When you send me the SHA-1 of your debug cert, I can add it to the app.

Will do, thanks 👍

Should invoking a notification action hide the notification or not? I tend to say they should, but currently that's not the case.

Hiding notifications is tracked in #3686.

Do we actually need this endpoint? FcmMessageListenerService already should support hiding notifications by persisted ID, reference ID and/or tag.

[maniac103]

Some other thing I noticed: maniac103@418b6f3

[mueller-ma]

Should invoking a notification action hide the notification or not? I tend to say they should, but currently that's not the case.

Yes, probably it should hide the notification.

Do we actually need this endpoint?

It's required that the app sends a "hide this notification" to other app installations of the same user.

[maniac103]

Yes, probably it should hide the notification.

I've added a change for that to my repo, please pick.

It's required that the app sends a "hide this notification" to other app installations of the same user.

Thanks, makes sense, didn't think of that.

[stefan-hoehn]

@mueller-ma
@maniac103

I am currently implementing the Blockly blocks for the notifications. Most of it works already (I'm on beta 3.15.0) but I ran into the following issue

• The notification was received
• But when I click on the notification it sends me to the notification screen which shows:

Ever since that with every notification that is received and I click on, this error screens pops up. Even closing the App doesn't fix it and it is therefore broken.

I guess I have seen some "wrong" notification which now has created a wrong notification history entry that breaks the App.

Any idea how we can debug that?

[maniac103]

What does the app log say? Additionally, what's the JS code which you used for creating that broken notification?

[stefan-hoehn]

I connected the mobile phone and locked into logcat with Android studio:

2024-07-14 15:09:40.499  5178-5178  CloudNotif...stFragment org.openhab.habdroid                 D  Notification response could not be parsed
     org.json.JSONException: No value for message
     	at org.json.JSONObject.get(JSONObject.java:398)
     	at org.json.JSONObject.getString(JSONObject.java:559)
     	at org.openhab.habdroid.model.CloudNotificationKt.toCloudNotification(Unknown Source:102)
     	at org.openhab.habdroid.ui.CloudNotificationListFragment$loadNotifications$1.invokeSuspend(Unknown Source:132)
     	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(Unknown Source:11)
     	at kotlinx.coroutines.DispatchedTask.run(Unknown Source:98)
     	at android.os.Handler.handleCallback(Handler.java:942)
     	at android.os.Handler.dispatchMessage(Handler.java:99)
     	at android.os.Looper.loopOnce(Looper.java:201)
     	at android.os.Looper.loop(Looper.java:288)
     	at android.app.ActivityThread.main(ActivityThread.java:7898)
     	at java.lang.reflect.Method.invoke(Native Method)
     	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
     	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)

Does that help?

How can I access the "applog"?

[maniac103]

How can I access the "applog"?

At the bottom of the settings page ... but it's just logcat made accessible from the app, so what you posted is sufficient.

So the crash says you have a notification without message. How is that even possible?
Looking at the code message is non-nullable and sent unconditionally?

[stefan-hoehn]

First of all, I have now built the project in Android Studio to be able to debug it directly.
To deploy it to my phone it told me that I had to remove the already installed version (which is the beta).
Now, unfortunately I don't get the error anymore :-(

Looking at the code message is non-nullable and sent unconditionally?

I have an idea:

I implemented the hide notification as follows:

which creates the following code

actions.notificationBuilder('').withReferenceId('refId111').hide().send();

As the message does not make sense in case of hiding a notification, I kept it empty, so maybe this causes the issue?

And guess what, as soon as I sent the above issue the error came up again!

I replaced the line with

message = payload?.optString("message").orEmpty(),

so now it doesn't crash anymore...
The side effect though is that this notification, which is only used to hide a referenced id notification, with the "empty" message is shown in the notifications which is probably not what we want, do we?

[maniac103]

I guess the actual bug/unexpected behavior is the cloud backend returning messages with type hideNotification when being asked for a list of notifications. hideNotification is expected to not have a message.
@digitaldan Is it expected for those messages to appear in the list returned by api/V1/notifications? If it is, we need to add some filtering code to both the list fragment and the notification poller.

[stefan-hoehn]

Additional information:

Let's say I send

actions.notificationBuilder("hide notification").withReferenceId('refId113').withTag('info').withOnClickAction('command:AZ_Govee_Light_Strip_Power:OFF').hide().send();

Even IF I sent a message with the hide notification everything is omitted except

{"reference-id":"refId113","type":"hideNotification"}

So even the tag is missing here which is at least surprising.

Btw, I would't have expected to receive the action but it actually would say it makes sense even in a hide message to have it: for example a notification could turn on a light and hiding the notification could turn of that light...

[maniac103]

The purpose of hideNotification is just to remove the notification from the device. This is why everything besides the ID is omitted: after all, there's no notification to show the additional content in. If there is, the client did something wrong ;-)

[stefan-hoehn]

Keep in mind though, that tags are allowed

see https://www.openhab.org/addons/automation/jsscripting/#cloud-notification-actions

.hide(): Hides notifications with the specified referenceId or tag.

[maniac103]

Oh, right, indeed, tag should be kept. I was mainly referring to the expectation about actions being present.

[digitaldan]

@digitaldan Is it expected for those messages to appear in the list returned by api/V1/notifications? If it is, we need to add some filtering code to both the list fragment and the notification poller.

The cloud service logs all notification messages. As of a few weeks ago, the API always returns the message payload for notifications, which in this case contains the type: 'hideNotification' as opposed to 'type: 'notification' for normal messages, which i believe the android client is also using when the message is delivered live (vs the api) .

actions.notificationBuilder("hide notification").withReferenceId('refId113').withTag('info').withOnClickAction('command:AZ_Govee_Light_Strip_Power:OFF').hide().send();

This is a helper function that @florian-h05 wrapped around the binding action for the JS library, but i believe if you use the 'hide' function there, its going to internally call hideNotification which will not include a message, title, commands, etc.... so those are being dropped in the helper library.

[maniac103]

The cloud service logs all notification messages. As of a few weeks ago, the API always returns the message payload for notifications, which in this case contains the type: 'hideNotification' as opposed to 'type: 'notification' for normal messages

OK, so this means the hideNotification message also gets written to the DB and thus gets its own _id like all other notification messages? That should be fine then and we should just filter it in the client.

so those are being dropped in the helper library.

@stefan-hoehn Sounds like tag being dropped is worth a bug report in openhab-js then.
Edit: it's fine, it's just either reference ID or tag ... See here

[florian-h05]

Tag should not be kept. The JS Scripting docs are unclear at that point, but as the cloud connector add-on only provides methods to hide by tag and other methods to hide by reference id, they cannot be used together when hiding notifications.
The notification builder implementation in openhab-js prefers referenceId over tag when hiding notifications.

[mueller-ma]

If it is, we need to add some filtering code to both the list fragment and the notification poller.

Both done here: #3750

[mueller-ma]

One remaining issue: The notification list doesn't load any new notifications beyond the first 20. I guess the issue is caused by CloudNotificationAdapter.addLoadedItems() when it filters out items.

[maniac103]

The notification list doesn't load any new notifications beyond the first 20

I'm pretty sure I tried this in the scope of this PR, and it worked fine? The cloud only keeps the latest 50 notifications, are you sure you didn't run into that?

The only thing relevant for loading past the first 20 is the hasMoreItems flag, which triggers the last item which in turn triggers the loading of further data.

[mueller-ma]

It didn't work in #3750 anymore, but that's fixed now.