eurofurence / ef-app_android

Eurofurence App (Android version)
MIT License
13 stars 5 forks source link

Dismiss Notifications when Announcements are opened #259

Closed Pinselohrkater closed 6 years ago

Pinselohrkater commented 6 years ago

When a single announcement is opened, dismiss the corresponding push notification that went with it.

If it's not possible to target that individually, it's also acceptable to dismiss all notifications (of the EF app) when an announcement is opened.

MarcusWolschon commented 6 years ago

This is a duplicate of #143 . It was closed but not (properly) fixed.

Each notification has an individual id. It is possible to address them individually. You just need to keep a map of event-id/announcement-id to notification-id and if an announcement/event is navigated to in the app, look up if there is a notification associated with it and have the NotificationManager dismiss it.

Pinselohrkater commented 6 years ago

I was assuming/hoping that it's possible to target them individually - thanks for confirming that. My main concern is however that we probably do not have the announcement_id or message_id to map against - on Android.

We have different payloads for iOS/Android devices, and announcement_id isn't part of the push notification we send to Android: https://github.com/eurofurence/ef-app_backend-dotnet-core/blob/master/src/Eurofurence.App.Server.Services/PushNotifications/FirebaseChannelManager.cs#L39

The short term (EF24, hopefully) solution imho. would be to just dismiss all notifications when you open the app, because the home screen shows announcements + personal messages, the later even indicate if there are unread ones.

Long term (after EF24), we should add correlation info like announcement_id to the Android push payload - and then we should be able to create said mapping, and dismiss items individually as they are read.

MarcusWolschon commented 6 years ago

Let's see...

here you receive the GCM/Firebase push message:

https://github.com/eurofurence/ef-app_android/blob/master/app/src/main/kotlin/org/eurofurence/connavigator/gcm/PushListenerService.kt#L39

here you build the Notification:

https://github.com/eurofurence/ef-app_android/blob/master/app/src/main/kotlin/org/eurofurence/connavigator/gcm/NotificationFactory.kt

For some reason you do not show the notification directly but serialize it and send it to a broadcast listener to be shown here:

https://github.com/eurofurence/ef-app_android/blob/master/app/src/main/kotlin/org/eurofurence/connavigator/gcm/NotificationPublisher.kt#L29

As the ID (needed to update or cancel that notification) you have chosen to use notification.hashCode() here: https://github.com/eurofurence/ef-app_android/blob/master/app/src/main/kotlin/org/eurofurence/connavigator/gcm/NotificationFactory.kt#L55 You just need to remember that ID together with whatever you notified about (obviously you need to find out but since you manage to build the intentToExecute with the pendingIntent, that should ne easy since that pendingIntent would already deep-link to the specific event or message that the user is being notified about.

How to cancel a notification: https://developer.android.com/reference/android/app/NotificationManager.html#cancel(int)

Pinselohrkater commented 6 years ago

You're missing my point, sorry.

I'll try to explain it differently.

Announcement "X" is raised on our backend. Let's assume it's defined as { id: 1, title: "Test", text: "This is a test" }

This leads to two things:

  1. A push notification sent to the device, that contains { type: 'announcement', title: "Test", text: "This is a test" }
  2. A new announcement is served by our backend when the app syncs content: { id: 1, title: "Test", text: "This is a test" }

We now have an active notification, and when we open the app, it obtains the same announcement again via the backend and shows it. Note the the id (1) is NOT part of the payload that gets sent via push.

What's missing is the link between the record served by the backend, and the notification sent via push; the notification data does not contain the id of the announcement. The annoucnements defined within the app are not generated from what the app gets via push, but retrieved from the backend. If you had more than one announcement, if you were to tap on Announcement "X" in the app, you wouldn't be able to dismiss the notification for "X" because there's no id that connects the record served by the backend and the notification that got sent via push; you'd have to compare text/message of the announcement and the to-be-dismissed notification.

That's why we first have to add this id to the notification data, so that the app can match it against content served by the backend. notification.hashCode() won't link against anything served by our API.

lukashaertel commented 6 years ago

@MarcusWolschon

For some reason you do not show the notification directly but serialize it and send it to a broadcast listener to be shown here:

They are serialized and added to a pending intent, which allows us to trigger the event reminder notifications before the events happen without running a timer or background service.

MarcusWolschon commented 6 years ago

(please be careful with wording. This gets confusing. It's a "Notification"(via Notification Manager) that is shown in the notification area of the screen and a "Push Message"(from Firebase Cloud Messaging) that is received )

For (public service) announcements I have one suggestion: After including the ID, make the app queue the download of that notification. (e.g. via work manager) This way the announcement is already loaded and can be shown right away without the device having any cellphone or wifi reception at that moment. If the user clicks the notification and the text fails to download, do NOT close the notification because the user has not been able to actually ready it. (A lot of apps don't think about network failures and get this wrong. Resulting in the user clicking a notification, getting an error message and then being lost. Knowing there is something important but not how to retry or being forced to leave that one app in the foreground until they are able to retry.)

I guess this is different for notifications about "event marked as favorite coming up" instead of "(public service )announcement" because they would be generated offline by timer? I can't test that case until EF has started and events actually take place.