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
8 changes: 8 additions & 0 deletions uhabits-android/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@
</intent-filter>
</receiver>

<receiver
android:name=".receivers.UpdateReceiver"
android:exported="true">
<intent-filter>
<action android:name="android.intent.action.MY_PACKAGE_REPLACED" />
</intent-filter>
</receiver>

<receiver android:name=".receivers.WidgetReceiver"
android:exported="true"
android:permission="false">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class HabitsApplication : Application() {
taskRunner.execute {
reminderScheduler.scheduleAll()
widgetUpdater.updateWidgets()
notificationTray.reshowAll()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,10 @@ class HabitsModule(dbFile: File) {
taskRunner: TaskRunner,
commandRunner: CommandRunner,
preferences: Preferences,
screen: AndroidNotificationTray
screen: AndroidNotificationTray,
habitList: HabitList
): NotificationTray {
return NotificationTray(taskRunner, commandRunner, preferences, screen)
return NotificationTray(taskRunner, commandRunner, preferences, screen, habitList)
}

@Provides
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,11 @@ class AndroidNotificationTray
habit: Habit,
notificationId: Int,
timestamp: Timestamp,
reminderTime: Long
reminderTime: Long,
silent: Boolean
) {
val notificationManager = NotificationManagerCompat.from(context)
val notification = buildNotification(habit, reminderTime, timestamp)
val notification = buildNotification(habit, reminderTime, timestamp, silent = silent)
createAndroidNotificationChannel(context)
try {
notificationManager.notify(notificationId, notification)
Expand All @@ -83,7 +84,8 @@ class AndroidNotificationTray
habit,
reminderTime,
timestamp,
disableSound = true
disableSound = true,
silent = silent
)
notificationManager.notify(notificationId, n)
}
Expand All @@ -94,7 +96,8 @@ class AndroidNotificationTray
habit: Habit,
reminderTime: Long,
timestamp: Timestamp,
disableSound: Boolean = false
disableSound: Boolean = false,
silent: Boolean = false
): Notification {

val addRepetitionAction = Action(
Expand Down Expand Up @@ -132,6 +135,7 @@ class AndroidNotificationTray
.setSound(null)
.setWhen(reminderTime)
.setShowWhen(true)
.setSilent(silent)
.setOngoing(preferences.shouldMakeNotificationsSticky())

if (habit.isNumerical) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ class ReminderController @Inject constructor(
private val notificationTray: NotificationTray,
private val preferences: Preferences
) {
fun onBootCompleted() {
reminderScheduler.scheduleAll()
}

fun onShowReminder(
habit: Habit,
timestamp: Timestamp,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class ReminderReceiver : BroadcastReceiver() {
}
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.
}
Comment on lines 97 to 100
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.

}
} catch (e: RuntimeException) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.isoron.uhabits.receivers

import android.content.BroadcastReceiver
import android.content.Context
import android.content.Intent
import android.util.Log

class UpdateReceiver : BroadcastReceiver() {

override fun onReceive(context: Context, intent: Intent) {
// Dummy receiver, relevant code is executed through HabitsApplication.
Log.d("UpdateReceiver", "Update receiver called.")
}
}
Comment on lines +8 to +14
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.

2 changes: 2 additions & 0 deletions uhabits-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

plugins {
kotlin("multiplatform")
kotlin("plugin.serialization") version "1.7.10"
id("org.jlleitschuh.gradle.ktlint")
}

Expand All @@ -30,6 +31,7 @@ kotlin {
dependencies {
implementation(kotlin("stdlib-common"))
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core-common:1.3.8")
implementation("org.jetbrains.kotlinx:kotlinx-serialization-json:1.4.0")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.isoron.uhabits.core.models

import kotlinx.serialization.Serializable
import org.isoron.platform.time.LocalDate
import org.isoron.uhabits.core.utils.DateFormats.Companion.getCSVDateFormat
import org.isoron.uhabits.core.utils.DateFormats.Companion.getDialogDateFormat
Expand All @@ -29,6 +30,7 @@ import java.util.Date
import java.util.GregorianCalendar
import java.util.TimeZone

@Serializable
data class Timestamp(var unixTime: Long) : Comparable<Timestamp> {

constructor(cal: GregorianCalendar) : this(cal.timeInMillis)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,17 @@
*/
package org.isoron.uhabits.core.preferences

import kotlinx.serialization.builtins.MapSerializer
import kotlinx.serialization.builtins.serializer
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import org.isoron.platform.time.DayOfWeek
import org.isoron.platform.utils.StringUtils.Companion.joinLongs
import org.isoron.platform.utils.StringUtils.Companion.splitLongs
import org.isoron.uhabits.core.models.Habit
import org.isoron.uhabits.core.models.HabitList
import org.isoron.uhabits.core.models.Timestamp
import org.isoron.uhabits.core.ui.NotificationTray
import org.isoron.uhabits.core.ui.ThemeSwitcher
import org.isoron.uhabits.core.utils.DateUtils.Companion.getFirstWeekdayNumberAccordingToLocale
import java.util.LinkedList
Expand Down Expand Up @@ -135,6 +141,23 @@ open class Preferences(private val storage: Storage) {
storage.putBoolean("pref_short_toggle", enabled)
}

internal fun setActiveNotifications(activeNotifications: Map<Habit, NotificationTray.NotificationData>) {
val activeById = activeNotifications.mapKeys { it.key.id }
val serialized = Json.encodeToString(activeById)
storage.putString("pref_active_notifications", serialized)
}

internal fun getActiveNotifications(habitList: HabitList): HashMap<Habit, NotificationTray.NotificationData> {
val serialized = storage.getString("pref_active_notifications", "")
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.

val activeByHabit = activeById.mapNotNull { (id, v) -> habitList.getById(id)?.let { it to v } }
activeByHabit.toMap(HashMap())
}
}

fun removeListener(listener: Listener) {
listeners.remove(listener)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,18 @@
*/
package org.isoron.uhabits.core.ui

import kotlinx.serialization.Serializable
import org.isoron.uhabits.core.AppScope
import org.isoron.uhabits.core.commands.Command
import org.isoron.uhabits.core.commands.CommandRunner
import org.isoron.uhabits.core.commands.CreateRepetitionCommand
import org.isoron.uhabits.core.commands.DeleteHabitsCommand
import org.isoron.uhabits.core.models.Habit
import org.isoron.uhabits.core.models.HabitList
import org.isoron.uhabits.core.models.Timestamp
import org.isoron.uhabits.core.preferences.Preferences
import org.isoron.uhabits.core.tasks.Task
import org.isoron.uhabits.core.tasks.TaskRunner
import java.util.HashMap
import java.util.Locale
import java.util.Objects
import javax.inject.Inject
Expand All @@ -38,9 +39,31 @@ class NotificationTray @Inject constructor(
private val taskRunner: TaskRunner,
private val commandRunner: CommandRunner,
private val preferences: Preferences,
private val systemTray: SystemTray
private val systemTray: SystemTray,
private val habitList: HabitList
) : CommandRunner.Listener, Preferences.Listener {
private val active: HashMap<Habit, NotificationData> = HashMap()

/**
* A mapping from habits to active notifications, automatically persisting on removal.
*/
private val active = object {
private val m: HashMap<Habit, NotificationData> =
preferences.getActiveNotifications(habitList)

val entries get() = m.entries

operator fun set(habit: Habit, notificationData: NotificationData) {
m[habit] = notificationData
persist()
}

fun remove(habit: Habit) {
m.remove(habit)?.let { persist() } // persist if changed
}

fun persist() = preferences.setActiveNotifications(m)
}

fun cancel(habit: Habit) {
val notificationId = getNotificationId(habit)
systemTray.removeNotification(notificationId)
Expand All @@ -64,8 +87,7 @@ class NotificationTray @Inject constructor(

fun show(habit: Habit, timestamp: Timestamp, reminderTime: Long) {
val data = NotificationData(timestamp, reminderTime)
active[habit] = data
taskRunner.execute(ShowNotificationTask(habit, data))
taskRunner.execute(ShowNotificationTask(habit, data, false))
}

fun startListening() {
Expand All @@ -83,9 +105,9 @@ class NotificationTray @Inject constructor(
return (id % Int.MAX_VALUE).toInt()
}

private fun reshowAll() {
fun reshowAll() {
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.

}
}

Expand All @@ -95,18 +117,26 @@ class NotificationTray @Inject constructor(
habit: Habit,
notificationId: Int,
timestamp: Timestamp,
reminderTime: Long
reminderTime: Long,
silent: Boolean = false
)

fun log(msg: String)
}

internal class NotificationData(val timestamp: Timestamp, val reminderTime: Long)
private inner class ShowNotificationTask(private val habit: Habit, data: NotificationData) :
@Serializable
internal class NotificationData(
val timestamp: Timestamp,
val reminderTime: Long,
)

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.

) :
Task {
var isCompleted = false
private val timestamp: Timestamp = data.timestamp
private val reminderTime: Long = data.reminderTime

override fun doInBackground() {
isCompleted = habit.isCompletedToday()
Expand All @@ -122,6 +152,7 @@ class NotificationTray @Inject constructor(
habit.id
)
)
active.remove(habit)
return
}
if (!habit.hasReminder()) {
Expand All @@ -132,6 +163,7 @@ class NotificationTray @Inject constructor(
habit.id
)
)
active.remove(habit)
return
}
if (habit.isArchived) {
Expand All @@ -142,6 +174,7 @@ class NotificationTray @Inject constructor(
habit.id
)
)
active.remove(habit)
return
}
if (!shouldShowReminderToday()) {
Expand All @@ -152,21 +185,33 @@ class NotificationTray @Inject constructor(
habit.id
)
)
active.remove(habit)
return
}
systemTray.showNotification(
habit,
getNotificationId(habit),
timestamp,
reminderTime
data.timestamp,
data.reminderTime,
silent = shown
)
if (shown) {
systemTray.log(
String.format(
Locale.US,
"Showing notification for habit %d silently because it has been shown before.",
habit.id
)
)
}
active[habit] = data
}

private fun shouldShowReminderToday(): Boolean {
if (!habit.hasReminder()) return false
val reminder = habit.reminder
val reminderDays = Objects.requireNonNull(reminder)!!.days.toArray()
val weekday = timestamp.weekday
val weekday = data.timestamp.weekday
return reminderDays[weekday]
}
}
Expand Down