[GSoC] Implemented Notification Datastore and Work Manager

[prateek-singh-3212]

Pull Request template

Purpose / Description

Implemented Notification Datastore and Work Manager for new notification subsystem. child of (#11487 )

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

⚠️
We should handle this now.

For example, why can't we use a Long timestamp in the map, if that's our implementation, or provide a custom comparator

[Arthur-Milchior]

That was explained in TimeOfNotification's javadoc. Because jsonobject requires string as map key.
Personally, I don't believe that blocking over an issue that will occur in 260 years is required. I highly doubt this code will still be in use by then

[david-allison]

The issue occurs immediately if getting the time fails and 'Jan 1 1970' is returned.

Note: Users do change their time to mess with the scheduler

[Arthur-Milchior]

Regarding epoch, I admit I had not thought about it. I have no real idea of what to do in this case because, if this occur, the whole system breaks. It assumes time is increasing and valid. Everything needs to be think through again. This is not just a problem of number of digits

Regarding changing their time, do you mean that we may consider the case where a user set time 260 years in the future? Or enough years in the past to get a digit less? In this case, again, the whole notification system will fail, since if you add even 24 hours, all notifications will be seen as overdue.

If you really want to consider the entire Long, then I guess we should use a Map whose keys are Long, and convert it to a map whose keys are String when writting it (and reciprocally). So at least conversions are done only once and not each time there is a search, insertion or deletion from the map

[david-allison]

All this takes to fix is constructing the class with a comparator.

We have a known bug which will be a nightmare to diagnose with an easy fix. I don't see why we're shying away from this fix

                NotificationTodo { o1, o2 -> o1.toLong().compareTo(o2.toLong()) }

[david-allison]

This needs to be passed into the constructor of TreeMap for it to perform sorting correctly

[david-allison]

This still isn't handled

[prateek-singh-3212]

@david-allison If, I am thinking correct then when we create the object of NotificationTodo then only we can pass the constructor and can write the below line

   NotificationTodo { o1, o2 -> o1.compare(o2) }

And In the current PR we are not creating the any object of NotificationTodo

[david-allison]

Why not make it so the object can only be constructed correctly?

[prateek-singh-3212]

Done added

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

[david-allison]

⚠️

I don't think this accurately models our users needs from notifications (primarily minCardDue).

What did your research into our use cases provide, and how would this be defined as a data structure

my uninformed thoughts - to read after commenting

I would wager that most people want:

• Remind me once in the day
• Remind me once at 23:00 if I haven't done any reviews

I suspect a more nuanced view is that users care about:

• Reviewing one card (to keep up their streaks)
• Finishing the decks they care about ('completed Anki for the day')

I suspect that some decks will want to be excluded from this calculation. EDIT: Most users will want this on an 'exclusion' basis

I suspect that we'll want a different mode for 'catching up': a user is happy if they've completed X cards per day from a deck which they've fallen behind from.

[Arthur-Milchior]

Comment before reading your thoughts:

* not having warning if there is only 2 cards seems not that bad. Going back to ankidroid while you did most of the work seems not necessarily a great use of time. At least offering the option to the user seems nice. * I admit that I was used to notification on deck options. But especially if e limit ourself to top level deck, having the notification chosen at the deck level don't seems absurd

Comment after reading your thoughts:

* We can always add more data in the data class than necessary. If we decide for example not to let the user select minutes, fine, it should still work. * I admit that adding an extra field to note "don't warn" if I already did N card seems nice * Since this notification is done at the deck level, I don't think we need anything specific to exclude some deck

[david-allison]

My issue is that we don't seem to have gone through the use cases, and this structure seems very far from what the 'ideal' would be.

Whether we should notify on 2 cards isn't the issue (IMO), it's ether the binary of 'finished/not finished' and 'studied/non studied' which users care about

Again, uninformed, rough draft of how I'd do this. NEEDS WORK

type NotificationTime = {hour: int; minute: int}

// excluding a deck will never complete to these items
type AdditiveCompletion = 'StudiedOneCard' | 'StudiedXCards' | 'StudiedXMinutes'
// Excluding a deck could cause these conditions to be completed.
type ReductiveCompletion =  'CompletedAllDecks' 
type Completion = [AdditiveCompletion; ReductiveCompletion ]

type Reminder = {
    time: NotificationTime;
    completion: Completion;
    enabled: boolean; // TODO: do we want this?
}

type AllDeckReminder = Reminder

// The majority of users will use 'All Deck' settings
type AllDeckNotificationSettings = {
    excludeDecks: DeckId[]; // globally excluded from checks - useful for 'ReductiveCompletion'
    reminders: AllDeckReminder[];
}

type IndividualDeckReminder = Reminder() = {
    deckIds: DeckId[];
    includeChildren: boolean; // edge case: otherwise we can't set notifications on a 'parent' deck without excluding child decks
}

type NotificationSettings = {
    allDeckNotifications: AllDeckNotificationSettings;
    individualDeckReminders: IndividualDeckReminder[]
}

let defaultNotificationSettings = {
  individualDeckReminders = [];
  allDeckNotifications = {
      excludeDecks = [];
      reminders = [{
          time: "8:00 AM";
          completion: "StudiedOneCard";
      }, {
          time: "6:00 PM";
          completion: "CompletedAllDecks";
      }, {
          time: "23:00 PM";
          completion: "StudiedOneCard";
      ]
  };
}

// TODO: Should handle the use case of 'coming back from a break', but happy for this to be non-automated with 'IndividualDeckReminder'

[prateek-singh-3212]

In my opinion we should go with tbe data class which I implemented currently because Either way we have to set the notification according to time. And to trigger notification we have to check the condition (for now minCardDue).

I like your idead to present in the form of some predefined option instead of giving all the control to user( which may confuse the user). So I will take care of this in UI implemntation but again I dont think so that we have to change the current data class.

@GSOCMentor

[david-allison]

My proposal above rests on the fact that minCardsDue is not a good abstraction of the user desires.

In my mind, the desires should be: "started" (streak) and "completed" (done for the day), and these can't be derived from minCardsDue

[prateek-singh-3212]

started is Long i.e day on which deck started.
completed is Boolean (true if Completed for day else false)
@david-allison

[david-allison]

I don't think these are part of this data structure. This is what a user defines, not the current state of notifications

[prateek-singh-3212]

Done please check now.

[david-allison]

Mainly discussion on the schema pending

[david-allison]

link to the method that explains why we're doing this: (getWorkManagerConfiguration)

[david-allison]

This still isn't handled

[prateek-singh-3212]

@david-allison Done with the suggested changes kindly review it.

[oakkitten]

I put some suggestions in #12635, of which this seems to be a part. Not entirely sure why this is a separate PR but I'd like the comments from there addressed here as well.

(Why does this need several PRs?)

[prateek-singh-3212]

@oakkitten That PR is built up on this PR. I did this because notification UI requires the preference datastore. I have created multiple PR's. I want this PR to get merged first.

[BrayanDSO]

This seems like a WIP and fails lint, so I'm going to draft it to clean a little the PRs queue

[github-actions]

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically