Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore notifications when app starts #1509

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

felixwiemuth
Copy link

@felixwiemuth felixwiemuth commented Sep 17, 2022

Overview

This pull request makes notifications from habit reminders survive reboots, app updates and force-closing by recreating them on app startup. After reboot and app update, notifications reappear without having to start the app manually, after a force-close, they reappear after starting the app manually. In all of these cases, notifications were previously gone and not recreated.

When notifications that had been shown before are recreated, they are shown silently, to not give the impression that they just became due. As before, notifications display the original reminder time - that is, when a notification is recreated, it doesn't display as "now", but shows the relative time since the reminder was due.

This implements #172.

Implementation

Technically, this is implemented in the following way:

  • Active notifications are persisted whenever they are changed, and reloaded from persistent storage whenever the NotificationTray class is instantiated.
    • Persistence is achieved through serializing the map with NotificationData objects (which now also includes a flag whether the notification has been shown already) to SharedPreferences, whenever this map is changed.
    • With persistency, it becomes more important to also remove no more active notifications from the map of active notifications, as they otherwise would be restored, but not reshown, and thus accumulate.
  • Whenever the app is created, HabitsApplication calls NotificationTray.reshowAll() which results in all active notifications being reshown. This also happens after reboot (because of receiving the BOOT_COMPLETED intent) and after app upgrade (because of receiving the MY_PACKAGE_REPLACED intent).
  • Previously, ReminderReceiver initiated a rescheduling directly; this was and is still not necessary, as it was and is initiated in HabitsApplication.

Testing

Manually tested successfully the following on a Pixel 4a (Android 13) to check whether it works as intended, including some previous behaviour:

  • Force stop app while notification showing, it shows again silently when opening app again
  • Force stop app before notification becomes due
    • Start app again before notification becomes due, it shows normally when due
    • Do not start app: notifications are not shown when due (this should be expected, as alarms are cancelled by force-stopping)
      • However, notifications due (but not shown previously) should be shown when eventually starting the app, but currently don't - this problem already exists in the current release version (2.1.0).
  • Force stop app after having marked habit as done in notification, it does not show again when starting app
  • Reboot while notification showing, it shows again silently about 2min after login (this is normal, it can be delayed a bit depending on when Android sends the reboot intent to the app)
  • Reboot before notification is due (while it is not showing), login after it is due: does not show - this problem also exists in release version 2.1.0, as above for the force-stop case
  • Updating app: active notifications show up silently after installing the update without manually starting the app
  • Changing reminder time while notification is showing: notification stays, but renews with sound etc. at the new due time
    • The same with rebooting (or force-stopping) before it becomes due and it becoming due before logging in (or starting the app, respectively): shows silently with old due time (NOTE: I would say this is not optimal but acceptable; I guess this is due to the same reason that new notifications are not shown when they became due while the app was not running, see above)
      • The same but with logging in / receiving boot intent before it becomes due: it first shows silently with old due time and then renews with sound at the new due time
  • When a notification is still active from the previous day, it gets replaced by the new one from the current day when it becomes due

The action is already performed via HabitsApplication.onCreate.
It is sufficient that there is a boot receiver.
- Persist the map of active notifications on every change and reload it on
application startup.
- Reshow all active notifications on application startup using the original
reminder due time.
So that notifications are also restored after app upgrade
@felixwiemuth felixwiemuth marked this pull request as ready for review September 17, 2022 15:49
It is not necessary to keep this field as in the map of active
notifications (and its persisted form) this field is always `true`.
@felixwiemuth
Copy link
Author

Note that I changed the implementation so that notifications are first added to the active map when the ShowNotificationTask is executed, and not when this task is created and scheduled to execute. I guess that this is fine and it is very unlikely that the app stops before the task is run?

This also means that we actually don't have to persist the shown field of NotificationData, as all notifications added to the active map have been shown (or are just about to be shown). I am currently testing this variant (it is a small change) and I think this will be preferable, except anyone sees an advantage of the current version.

@hiqua
Copy link
Collaborator

hiqua commented Sep 18, 2022

Thanks a lot! Any way to have automated tests for this?

@felixwiemuth
Copy link
Author

Any way to have automated tests for this?

I looked into the existing unit tests but didn't find anything related, e.g. some stubbing/test setup that would allow to test whether persistence works correctly.

Regarding instrumented tests, I am not sure how straight-forward it is to test the relevant situations (reboot, force-stop, update). What could be useful is to test whether after starting the app, previously serialized active notifications are shown (however, one has to make sure that they actually should be shown, there are some conditions). At best the serialized notifications are created through the UI as well (schedule habit reminder so that it becomes due). But I am not enough into instrumented tests to say how easy this is.

@felixwiemuth
Copy link
Author

I have now also tested that a notification from the previous day gets replaced by the new one from the current day when it is due (in the slightly updated version without persisting shown though).

@felixwiemuth
Copy link
Author

I now also pushed the small update with removing the unnecessary shown field from NotificationData. I executed all manual test from above again and they still seem to pass in the same way. However, I updated, corrected and extended the test list a bit, see in bold for some undesirable behaviour.

I think what should be fixed is that reminders becoming due while the device is off are not eventually shown when the device or even the app is started. This exists, as noted, already in the current release version 2.1.0.

If you agree that this should be fixed, I would take a look.

Copy link
Owner

@iSoron iSoron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @felixwiemuth, thank you for the pull request. I tried it out, and it seems to work well. Please see some comments below.

Regarding testing, I suggest not testing any uhabits-android classes at this time, but I think it's important to write tests for all changes in uhabits-core. For example, we could:

  1. Store notifications to preferences then immediately retrieve them and check that the retrieved version equals the original one.
  2. Create a NotificationTray with a mock Preferences, and ensure that the correct calls to preferences.setActiveNotifications are made. Also ensure that set/remove/get operations behave as expected.

Comment on lines 97 to 100
Intent.ACTION_BOOT_COMPLETED -> {
Log.d("ReminderReceiver", "onBootCompleted")
reminderController.onBootCompleted()
// NOTE: Some activity is executed after boot through HabitsApplication, so receiving ACTION_BOOT_COMPLETED is essential.
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify why do we still need this block? We would still receive ACTION_BOOT_COMPLETED, even if we remove the block.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block is not needed (and I can see the intent is also logged above); What I deemed important is that somewhere it is noted that it is important that the ACTION_BOOT_COMPLETED intent is received.

Actually, as for both ACTION_BOOT_COMPLETED and MY_PACKAGE_REPLACED the only thing we need is that the application is started (and all the code from ReminderReceiver is not needed), I would suggest that both should be received by a common dummy receiver (as currently with UpdateReceiver), which we could call StartAppReceiver or similar. Then it is clear that these intents are solely received to let the app start.

Comment on lines +8 to +14
class UpdateReceiver : BroadcastReceiver() {

override fun onReceive(context: Context, intent: Intent) {
// Dummy receiver, relevant code is executed through HabitsApplication.
Log.d("UpdateReceiver", "Update receiver called.")
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a new (empty) receiver? Could we add MY_PACKAGE_REPLACED to the list of intent filters of ReminderReceiver instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

for ((habit, data) in active.entries) {
taskRunner.execute(ShowNotificationTask(habit, data))
taskRunner.execute(ShowNotificationTask(habit, data, true))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For boolean arguments, it's best to add the argument name (shown = true)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

private inner class ShowNotificationTask(
private val habit: Habit,
private val data: NotificationData,
private val shown: Boolean
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silent is a bit more clear to me. Changing it would also make it consistent with the other class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, done.

return if (serialized == "") {
HashMap()
} else {
val activeById = Json.decodeFromString(MapSerializer(Long.serializer(), NotificationTray.NotificationData.serializer()), serialized)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the decoding fails? Does the app fail to boot? I think this could happen if: (i) user receives some notifications; (ii) we update NotificationData with some new/renamed fields; (iii) user installs updates and restarts the app. The new version of the app would not be able to read serialized data from the old version.

I suggest adding a try/catch block here, and returning HashMap() if the operation fails.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes currently there would be an uncaught exception if decoding fails, and the app would crash on startup. That is of course bad, even though unlikely. Returning an empty map in case it cannot be deserialized is fine, as (as far as I can see) no other functionality depends on all active notifications being available in the map.

What we could do is show a notification that notifications could not be restored and the user should make sure to look at the habits manually, but regarding that it is a corner case it's probably not worth it.

Btw., decoding will not fail if fields not present in the encoded string have default values in the Kotlin class.

@iSoron
Copy link
Owner

iSoron commented Sep 25, 2022

I think what should be fixed is that reminders becoming due while the device is off are not eventually shown when the device or even the app is started. This exists, as noted, already in the current release version 2.1.0.

This would be a nice feature, but it could also be a separate PR.

In case serialized notifications cannot be deserialized, continue with an empty map of active notifications
@felixwiemuth
Copy link
Author

I think what should be fixed is that reminders becoming due while the device is off are not eventually shown when the device or even the app is started. This exists, as noted, already in the current release version 2.1.0.

This would be a nice feature, but it could also be a separate PR.

I agree that this should be a separate PR (or issue for now). I have also a few other related things where I'd open a new issue for.

@felixwiemuth
Copy link
Author

Regarding testing, I suggest not testing any uhabits-android classes at this time, but I think it's important to write tests for all changes in uhabits-core. For example, we could:

1. Store notifications to preferences then immediately retrieve them and check that the retrieved version equals the original one.

I'm currently looking into adding a test to PreferencesTest but am looking for some help with map comparison (see https://stackoverflow.com/questions/73852632/test-equality-of-collections-with-custom-matcher).

2. Create a NotificationTray with a mock Preferences, and ensure that the correct calls to `preferences.setActiveNotifications` are made. Also ensure that `set/remove/get` operations behave as expected.

I started writing a test with a MockPreferences : Preferences(DummyStorage()) class which simply retains the map passed to it via setActiveNotifications. For this I need to make the set and get operations open, so that they can be overridden with the mock functionality.
Then we can test the public methods show and cancel of NotificationTray, and potentially the four special cases where habits should be removed from the active list during execution of ShowNotificationTask.

Copy link
Author

@felixwiemuth felixwiemuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now added the described tests in their current state. They check most of what we discussed. See the comments for further info.

notificationTray.show(habit, timestamp, reminderTime)

// Verify that the active notifications include exactly the one shown reminder
// TODO are we guaranteed that task has executed?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a ShowNotificationTask is executed by a task runner, I am not sure whether this might be asynchronous and we should for example add a delay here to increase the likelihood that it has executed? Or are tasks (at least for these tests) run in the same thread, which would make tests safer?

notificationTray.cancel(habit)
assertThat(preferences.getActiveNotifications(habitList).size, equalTo(0))

// TODO test cases where reminders should be removed (e.g. reshowAll)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we could add tests whether the remaining methods which touch the active notifications map remove entries in the correct situations. It is a little work to setup all the special cases, I can't promise right now when I'll be able to do that.

Comment on lines +189 to +190
assertThat(a.keys, equalTo(b.keys))
a.forEach { e -> assertThat(b[e.key], equalTo(e.value)) }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a manual workaround for the missing matcher/library function I was looking for.

@hiqua
Copy link
Collaborator

hiqua commented Aug 10, 2023

@iSoron if the state of the PR seems ok I can try to rebase it, let me know. AFAICT from reading the comments, all the concerns were addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants