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

[GSoC] Implemented New notification preference UI #12635

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
313 changes: 313 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/anki/NotificationDatastore.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,313 @@
/*
* Copyright (c) 2022 Prateek Singh <[email protected]>
*
* This program is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License as published by the Free Software
* Foundation; either version 3 of the License, or (at your option) any later
* version.
*
* This program is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
* PARTICULAR PURPOSE. See the GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along with
* this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.ichi2.anki

import android.content.Context
import androidx.datastore.core.DataStore
import androidx.datastore.preferences.core.Preferences
import androidx.datastore.preferences.core.edit
import androidx.datastore.preferences.core.intPreferencesKey
import androidx.datastore.preferences.core.stringPreferencesKey
import androidx.datastore.preferences.preferencesDataStore
import com.fasterxml.jackson.core.JacksonException
import com.fasterxml.jackson.databind.ObjectMapper
import com.ichi2.anki.NotificationDatastore.Companion.getInstance
import com.ichi2.anki.model.DeckNotification
import com.ichi2.anki.model.UserNotificationPreference
import com.ichi2.libanki.DeckId
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.firstOrNull
import kotlinx.coroutines.launch
import org.json.JSONObject
import timber.log.Timber
import java.util.*

/**
* Time at which deck notification will trigger. Stores time in millisecond in EPOCH format.
*/
/*
* We use TimeOfNotification as key of json object. So it must be string.
*/
typealias TimeOfNotification = String
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need a new type? Epoch milliseconds should be simply a Long, or EpochMillis if you wish.

Also, surely jackson can deal with non-string keys? kotlinx does it via storing maps as lists.

Also, when I hear “time of notification” I think of stuff like "HH:MM". This is clearly not it.


/**
* DeckIds which is going to trigger at particular time.
*/
typealias SimultaneouslyTriggeredDeckIds = HashSet<DeckId>

/**
* Indicates all notifications that we must eventually trigger. If a notification time is in the past, we must do it as soon as possible, otherwise we must trigger the notification later.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is way too long. I suggest keeping at around 100 characters max, which is AS default. (My personal favorite is 88.)

* Implemented as map from time to set of deck ids triggered at this time
*/
typealias NotificationTodo = TreeMap<TimeOfNotification, SimultaneouslyTriggeredDeckIds>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a new type? Also, why is this a hash set specifically?

IMO, you only ever use type aliases:

  • to disambiguate things that have the same name,
  • to create aliases for long, hard to read types,
  • to give a name to an object that could be another type or an interface.

If you think you have the last case, perhaps think of it like this.

  • does it make sense to use another class in this case?
  • if yes, can you get away with using a value class?
  • if yes, but it would be too difficult to implement, use a type alias.

I don't think this case applies here.


/**
* Notification which is going to trigger next.
* */
fun NotificationTodo.earliestNotifications(): MutableMap.MutableEntry<TimeOfNotification, SimultaneouslyTriggeredDeckIds>? =
firstEntry()
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, you just created an extension method for all TreeMap<String, HashSet<Long>>. This needs to be a class or something.


/**
* Compare the [TimeOfNotification]. It avoid the failure of comparing 2 [TimeOfNotification] when timestamp gets an extra digit because we'll get new timestamps
* starting with a 1 that will be a number greater than previous numbers but alphabetically shorter
* */
fun TimeOfNotification.compare(timeOfNotification: TimeOfNotification) =
this.toLong().compareTo(timeOfNotification.toLong())
Copy link
Contributor

Choose a reason for hiding this comment

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

...or just use Long in the first place


/**
* Default object for [NotificationTodo]. This object adds comparator for string values.
* */
fun NotificationTodoObject() = NotificationTodo { timeOfNotification1, timeOfNotification2 ->
timeOfNotification1.compare(timeOfNotification2)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, this function here is clear indicator that

Copy link
Member

Choose a reason for hiding this comment

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

I do not know if you're being ironic here, or if you just didn't finish your sentence.

Copy link
Member

Choose a reason for hiding this comment

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

I do not know if you're being ironic here, or if you just didn't finish your sentence.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to write something but I just couldn't bring myself to finish the sentence. But hey you finished yours twice so it evens out!


/**
* Stores the scheduled notification details
* This is a singleton class, use [getInstance]
* */
class NotificationDatastore private constructor(val context: Context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is extremely overengineered. I would like this entire class removed. Instead, I would just take all decks and their times and put it in a string such as "123:12:00 456:18:00" and store it in shared preferences under a single key and call it a day. A couple of lines to decode, a couple of lines to encode.

Also, calling async functions like that creates a race condition. If you call a methods such as putIntSync, reading the value immediately after may yield the old value.


/**
* 1. Stores the String in Notification Datastore.
* 2. It stores the data asynchronously.
* 3. Calling this function guarantees to store value in database.
* @param key The Key of value. Used in fetching the data.
* @param value Value that needs to be stored <b>(VALUE MUST BE STRING)</b>.
* */
suspend fun putStringAsync(key: String, value: String) {
val dataStoreKey = stringPreferencesKey(key)
context.notificationDatastore.edit { metaData ->
metaData[dataStoreKey] = value
}
}

/**
* Stores the String in Notification Datastore
* It stores the data synchronously. It will create Coroutine [Dispatchers.IO] Scope Internally.
* @param key The Key of value. Used in fetching the data.
* @param value Value that needs to be stored <b>(VALUE MUST BE STRING)</b>.
* */
fun putStringSync(key: String, value: String) {
CoroutineScope(Dispatchers.IO).launch {
putStringAsync(key, value)
}
}

/**
* Fetches the String value from Datastore.
* @prams The Key of deck whose data you want to fetch.
* @return Value associated to `key` by the last call to [putStringSync], [putStringAsync], or [default] if none
* */
suspend fun getString(key: String, default: String): String {
val dataStoreKey = stringPreferencesKey(key)
return context.notificationDatastore.data.firstOrNull()?.let {
it[dataStoreKey]
} ?: default
}

/**
* Stores the Integer in Notification Datastore
* It stores the data asynchronously.
* Calling this function guarantees to store value in database.
* @param key The Key of value. Created while storing the data.
* @param value Value that needs to be stored <b>(VALUE MUST BE INTEGER)</b>.
* */
suspend fun putIntAsync(key: String, value: Int) {
val dataStoreKey = intPreferencesKey(key)
context.notificationDatastore.edit { metaDataEditor ->
metaDataEditor[dataStoreKey] = value
}
}

/**
* Stores the Integer in Notification Datastore
* It stores the data synchronously. It will create Coroutine [Dispatchers.IO] Scope Internally.
* @param key The Key of value. Created while storing the data.
* @param value Value that needs to be stored <b>(VALUE MUST BE INTEGER)</b>.
* */
fun putIntSync(key: String, value: Int) {
CoroutineScope(Dispatchers.IO).launch {
putIntAsync(key, value)
}
}

/**
* Fetches the Integer value from Datastore.
* @prams The Key of deck whose data you want to fetch.
* @return Value associated to `key` by the last call to [putIntSync], [putIntAsync], or [default] if none
* */
suspend fun getInt(key: String, default: Int): Int {
val dataStoreKey = intPreferencesKey(key)
return context.notificationDatastore.data.firstOrNull()?.let {
it[dataStoreKey]
} ?: default
}

/**
* Stores the Map of time and list of deck ids to Datastore
* It stores the data asynchronously.
* */
suspend fun setTimeDeckData(data: Map<String, HashSet<Long>>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean NotificationTodo here? Also, why is this saved in the first place?

What is “time deck data”?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean NotificationTodo here? Also, why is this saved in the first place?

Yes, Actually Their is the error when I pass the notificationTodo to Json Object so I wrote this

val dataStoreKey = stringPreferencesKey("TIME_DECK_DATA")
val jsonObj = JSONObject(data)
context.notificationDatastore.edit { metaData ->
metaData[dataStoreKey] = jsonObj.toString()
}
}

/**
* Fetches the Map of time and list of deck ids from Datastore.
* @return The current AllTimeAndDecksMap
* */
/*
* We actually are not blocking the thread. This method throws an exception. It will not create problem for us.
* */
@Suppress("UNCHECKED_CAST", "BlockingMethodInNonBlockingContext")
suspend fun getTimeDeckData(): NotificationTodo? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Method “gets” “time deck data” but returns NotificationTodo?.

Method yields a TreeMap without a comparator used by the NotificationTodoObject thing. Not that NotificationTodoObject is used by anything else, but still.

val datastoreKey = stringPreferencesKey("TIME_DECK_DATA")
return context.notificationDatastore.data.firstOrNull()?.let {
try {
objectMapper.readValue(
it[datastoreKey],
TreeMap::class.java
) as NotificationTodo
} catch (ex: JacksonException) {
Timber.d(ex.cause)
null
}
}
}

/**
* Stores the details of the [notification] scheduling of deck [did]
* @return operation successful of not.
* */
suspend fun setDeckSchedData(did: DeckId, notification: DeckNotification): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this function saves deck scheduling data...

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well because it saves a “notification”. I am not sure what “deck scheduling data” could be, maybe card reviews? but clearly not anything related to notifications.

val dataStoreKey = stringPreferencesKey(did.toString())
return runCatching {
val json = objectMapper.writeValueAsString(notification)
context.notificationDatastore.edit { metaData ->
metaData[dataStoreKey] = json
}
}.isSuccess
}

/**
* Fetches the details of particular deck scheduling.
* @return Deck Notification model for particular deck.
* */
/*
* We actually are not blocking the thread. This method throws an exception. It will not create problem for us.
* TODO: unit test that :
* * if there is no preference at all, we return null
* * if there is a preference without entry for this key we return null
* * if there is a preference whose entry for this key can't be cast to DeckNotification, throw
* * if there is a preference with entry for this key that can be cast, we get expected notification
*/
@Suppress("BlockingMethodInNonBlockingContext")
suspend fun getDeckSchedData(did: DeckId): DeckNotification? {
val datastoreKey = stringPreferencesKey(did.toString())
return context.notificationDatastore.data.firstOrNull()?.let {
try {
if (!it.contains(datastoreKey)) {
// Deck data not stored.
return null
}
objectMapper.readValue(
it[datastoreKey],
DeckNotification::class.java
)
} catch (ex: Exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Code two methods above catches JacksonException and doesn't send a report. Why is this different?

// Let the exception throw
CrashReportService.sendExceptionReport(
ex,
"Notification Datastore-getDeckSchedData",
"Exception Occurred during fetching of data."
)
throw Exception("Unable to find schedule data of given deck id: $did", ex)
}
}
}

/**
* Stores the details of the [userPreference] for calculating user streak and deck completion status.
* @return operation successful of not.
* */
suspend fun setUserPreferenceData(did: DeckId, userPreference: UserNotificationPreference): Boolean {
val dataStoreKey = stringPreferencesKey("$did-user")
return runCatching {
val json = objectMapper.writeValueAsString(userPreference)
context.notificationDatastore.edit { metaData ->
metaData[dataStoreKey] = json
}
}.isSuccess
}

/**
* Fetches the details of user preference data for the give deck.
* @return [UserNotificationPreference] for the particular deck.
* */
suspend fun getUserPreferenceData(did: DeckId): UserNotificationPreference? {
val datastoreKey = stringPreferencesKey("$did-user")
return context.notificationDatastore.data.firstOrNull()?.let {
try {
if (!it.contains(datastoreKey)) {
// Deck data not stored.
return null
}
objectMapper.readValue(
it[datastoreKey],
UserNotificationPreference::class.java
)
} catch (ex: Exception) {
// Let the exception throw
CrashReportService.sendExceptionReport(
ex,
"Notification Datastore-getUserPreferenceData",
"Exception Occurred during fetching of data."
)
throw Exception("Unable to find preference data of given deck id: $did", ex)
}
}
}

companion object {
private val objectMapper = ObjectMapper()
private lateinit var INSTANCE: NotificationDatastore
private val Context.notificationDatastore: DataStore<Preferences> by preferencesDataStore("NotificationDatastore")
private fun instanceInitializedOr(
context: Context,
block: (context: Context) -> NotificationDatastore
) = if (this::INSTANCE.isInitialized) INSTANCE else block(context)

/**
* Thread safe. We're using this pattern because we need a Context to get an instance.
* @return The singleton NotificationDatastore
*/
fun getInstance(context: Context) = instanceInitializedOr(context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciate the double-checked locking but why does this need to be thread safe?

Also, IIRC companion objects are created in a thread-safe manner so you should just be able to say INSTANCE = ....

Copy link
Member

Choose a reason for hiding this comment

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

I can't easily find a source for the thread-safe part. At least not on https://kotlinlang.org/docs/object-declarations.html#semantic-difference-between-object-expressions-and-declarations which is the place I'd expect to see such an information

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a SO answer and not official docs but it makes sense I guess? https://stackoverflow.com/a/30190567

synchronized(this) {
// Check again whether [INSTANCE] is initialized because it could have been initialized while waiting for synchronization.
instanceInitializedOr(context) {
NotificationDatastore(context).also {
INSTANCE = it
}
}
}
}
}
}
57 changes: 57 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/anki/model/DeckNotification.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright (c) 2022 Prateek Singh <[email protected]>
*
* This program is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License as published by the Free Software
* Foundation; either version 3 of the License, or (at your option) any later
* version.
*
* This program is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
* PARTICULAR PURPOSE. See the GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along with
* this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.ichi2.anki.model

import com.ichi2.libanki.DeckId
import com.ichi2.libanki.Decks

/**
* Notification details of particular deck.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment isn't very helpful

* */
data class DeckNotification(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer the name to reflect that this is about reminding to study. It's hard to give this an exact name but maybe something like RemindToStudy. Also I'd ditch enabled and rely on presence or absence of instances of this class.

Also, a matter of personal preference, I'd say deckId. In my opinion, did is a mistake not worth following.

val enabled: Boolean,
val did: DeckId,
val schedHour: Int,
val schedMinutes: Int,
) {

// Empty constructor. Required for jackson to serialize.
constructor() : this(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is way too simple to serialize.

Serialization is fine for possibly complex objects such as encryption keys, or for objects that aren't supposed to be long-lived. Serialization is nice until you need to change how you store your data, or until some weird thing changes and you can't deserialize. For preferences, especially in such simple cases like this, I'd strongly prefer something more manual. E.g. just encode all of enabled decks and their times in a string.

Also, why jackson istead of kotlinx.serialization?

false,
Decks.NOT_FOUND_DECK_ID,
0,
0
)
}

/**
* User Specific preference for notification.
* @param did Id of deck whose preference is stored.
* @param started Day on which deck started.
* @param completed Deck is done for the day or not.
* */
data class UserNotificationPreference(
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what this entire class is about.

val did: DeckId,
val started: Long,
val completed: Boolean
) {
constructor() : this(
Decks.NOT_FOUND_DECK_ID,
-1L,
false
)
}