Decoupled Notification From Widget

[prateek-singh-3212]

Pull Request template

Purpose / Description

Notification is tightly coupled with SmallWidget status which results in Notification whenever widget updates.

Fixes

Fixes #8114
Fixes #6476

Approach

We can make the new repository which holds the meta Data (or Meta information) of all decks like Total deck cards, Total New cards to Review, Total Learning cards etc in sqlite Database and whenever the data is required by widget , Notification or any other activity, Service.

This discussion is documented in this PR (this).

Flow Chart of Above Explained.

Flow Chart of Above Explained.

How Has This Been Tested?

Firefox browser and Galaxy M51 (API 30) Aspect ratio 20:9

Checklist

Please, go through these checks before submitting the PR.

• You have not changed whitespace unnecessarily (it makes diffs hard to read)
• You have a descriptive commit message with a short title (first line, max 50 chars).
• Your code follows the style of the project (e.g. never omit braces in if statements)
• 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]

Thanks!

This could do with more documentation, either by splitting out the commits, or a message in the commit message and PR summary.

I'll need to reread this after the comments are added, but from scanning, this seems to remove functionality (widgets cause notifications) with no replacement

[prateek-singh-3212]

Issues

1. Widget is not updating when app is not opened or in not present in background (real-time).
2. Widget takes 4-5 sec to update whenever their is update in data of due cards and if we kill the app while widget is updating (before 4-5 sec) then it will not show the updated data.

[david-allison]

To move this along: we either need to re-instate the widget based logic, or provide a better replacement

[prateek-singh-3212]

Yes, I also think so that we have to re-engineer the widget part . Should I create a new issue ?

[david-allison]

Yes, I also think so that we have to re-engineer the widget part . Should I create a new issue ?

I thought we'd discussed in the issue that the widget based logic would need to be improved/replaced? I don't think we need a new issue

[prateek-singh-3212]

I thought we'd discussed in the issue that the widget based logic would need to be improved/replaced?

I am unable to find any discussion on widget based logic. There is only one issue related to this i.e #6476.
And this PR solves this (#6476) issue also by removing this part ⬇️

Anki-Android/AnkiDroid/src/main/java/com/ichi2/widget/WidgetStatus.java

Line 114 in 8ada06b

Intent intent = new Intent(NotificationService.INTENT_ACTION);

[david-allison]

Our widget-based notification code is known to be buggy, and better alternatives exist.

But, people are using the widget to get some notification-based functionality in AnkiDroid, and this removes this possibility without providing an alterative.

I don't think we should remove this functionality for users until we have a better alternative.

I'd approve this PR as just a refactoring to make future replacement easier, or if it refactors and replaces the widget code with something better

[prateek-singh-3212]

@david-allison-1 I will refactor the widget part first.

[prateek-singh-3212]

REFACTORED WIDGET
✅ Notification will come irrespective of widget.
✅ Implemented Alarm Manager to update the widget which will improve the battery drain.
✅ Improved the feature which shows the notification whenever widget updates.

WIDGET WORKING
Widget will work completely independently from the app. Widget will auto update itself after a fixed interval of time (for now 30 min). If widget is able to fetch data then it show it else no data will be shown (i.e empty widget but rare case when DeckDueTreeNode is unable the send data.

WIDGET NOTIFICATION
There is feature which shows the notification whenever widget updates. So I refactored it by storing the card no. when widget is last updated. If the new due cards is not equal to last updated due cards then it will show the notification else no change will be their. If we want to update the widget whenever app closes then we have to implement a callback which starts the Update notification service (I will try this in next commit).

DONE TODO

TODO @mikehardy - we can reduce battery usage by widget users by removing updatePeriodMillis from metadata
and replacing it with an alarm we set so device doesn't wake to update the widget, see:
https://developer.android.com/guide/topics/appwidgets/#MetaData

LEARNING RESOURSE

1. https://proandroiddev.com/android-alarmmanager-as-deep-as-possible-909bd5b64792
2. https://developer.android.com/guide/topics/appwidgets

[prateek-singh-3212]

Commit : 8047da5

CardBrowser.java
DeckPicker.java
ModelBrowser.java
ModelFieldEditor.java
NoteEditor.java
Reviewer.java
StudyOptionsActivity.java

Removed Widget.update() from on Stop of these activities.

TESTING PROCESS

1. For now I have set alarm repeat time to 15 sec. for both Notification Alarm and Notification Widget.
2. For ease of logging I have given the tag AACC for all the changes. I have implemented.
3. Test first without widget and after adding widget.

[david-allison]

This is looking better!

Could you split this out into more commits (ideally breaking out the "functional" change from the "refactoring" changes)

I'll come back to this tomorrow with a fresh pair of eyes (there's more to review), I'm finding it difficult to understand the context of the refactorings, vs the functional change, and more atomic commits would help here

[david-allison]

I'm following this a bit more, but could you break up the commits into more atomic changes.

This combines Mike's suggested battery improvement with other changes, and attributing a line change to an intention is difficult currently.

I think we'll be able to remove a fair amount of code from this, it feels more complicated than it should be.

[prateek-singh-3212]

Done with atomic commits, David kindly review this once again.

[david-allison]

Much nicer!

Needs a little more done on the first commit (review comments, and removing/splitting out the functional change)

I don't think the fourth commit needs to exist. I don't think there needs to be a linkage between the notification and the widget.

If we still need one, the reasoning needs to be well documented

[prateek-singh-3212]

I don't think the fourth commit needs to exist. I don't think there needs to be a linkage between the notification and the widget.

If we still need one, the reasoning needs to be well documented

Yes it is needed. If we don't want to send the the notification when user is using our app.
Plan is to don't show the notification when widget is updating from onStop() of activites. We only show the notification when widget is updating from widget update Alarm.

[prateek-singh-3212]

David Please review it once again. Done All things as per plan

[mikehardy]

then checking each minute seems sensible

I'm not sure if this is an issue or not but If we recompute the counts as a poll, instead of doing this as an event-based thing we'll be put in the "you use too much power" penalty bucket and background services will be adaptively killed (or start-denied) by power algorithms on device and notifications won't work for anyone

This stuff is hard to get right, but the fundamental thing is that notifications and widget counts need to be updated (not delete+re-create - same notification id re-used and just posted with new content) via an event subscription of some sort for the cases where the user is actually altering counts, then they should have zero activity except a PendingIntent for the notification to fire at the scheduled time (if any).

[david-allison]

Is there documentation for the criteria (from the most strict OS) for being put in the 'low power' bucket? Is this "too many wakeups", or "using too much time when woken up"

---

From a scheduling perspective, we can calculate the next time deterministically:

Reviews/day learn: We only need to check once per day at the day cutoff threshold - no issue. BUT: We may want this on a separate notification channel which won't cause vibrations, nobody wants a 4AM wakeup call to do their cards

Intraday, except when the app is being used, all we care about is:

• "sub-day learn" cards, which depend on the current time. We can calculate the time that the notification would next need to be shown trivially.
• API usage

---

If we show "you have over 250 cards due", then we don't need to calculate the exact number, as we don't display it.

If the issue is "using too much time when woken up", when the app is running, we can calculate the next time to wake up in a background thread (event based, rather than database-based). This would add a lot of complexity to the app, so I'd rather avoid it

[mikehardy]

The ranking of terrible Android implementations changes constantly, this site is useful: https://dontkillmyapp.com/

[prateek-singh-3212]

Sorry, I was a little busy from past few weeks.
@david-allison-1 can we get back to this PR?

Regarding the algorithm which calculates the next time of notification.
Please give me the computing points to make algorithm like if reviews/day < 3 then show next notification in 30 minutes which says "Hey! where are you? Your 250 cards are pending for today" like that.
My plan is this

[david-allison]

Glad to see you back!

This seems rather complicated.

Why do we need three services here?

As for the criteria: I'm open to suggestions but for a sensible first step, I'd use:

• One at the start of the day, shown once
• Check every interval for new "learn" cards, shown once each time the learn card count is over the threshold AND a specified amount of time has passed since the last notification

---

• (out of scope for the first implementation as it needs discussions) - one at the end of the day, if the user hasn't done X (for me, if any reviews/news are still pending)

@mikehardy @Arthur-Milchior - pinging as this is off the top of my head and I haven't worked through consequences

[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

[david-allison]

@prateek-singh-3212 Added the "needs a new dev" label as it went stable, let me know if you'll have the time to finish this one off

[prateek-singh-3212]

@david-allison-1 Sorry I am unable to proceed this further as I am busy in my college classes.

[david-allison]

No worries! Life takes priority.

[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

[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

[krmanik]

@prateek-singh-3212 I think you are implementing in another PR #11487. If you wish to continue this PR then its okay, otherwise current PR can be closed.

[prateek-singh-3212]

No this PR for reference only. I am impleting this in new PR.

[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