ankidroid
Decoupled Notification From Widget
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.
- [x] You have not changed whitespace unnecessarily (it makes diffs hard to read)
- [x] You have a descriptive commit message with a short title (first line, max 50 chars).
- [x] Your code follows the style of the project (e.g. never omit braces in
ifstatements) - [x] You have commented your code, particularly in hard-to-understand areas
- [x] 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
Issues
- Widget is not updating when app is not opened or in not present in background (real-time).
- 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.
To move this along: we either need to re-instate the widget based logic, or provide a better replacement
Yes, I also think so that we have to re-engineer the widget part . Should I create a new issue ?
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
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 ⬇️ https://github.com/ankidroid/Anki-Android/blob/8ada06b2e6521aa4e9eff2a0539dc192ef89a894/AnkiDroid/src/main/java/com/ichi2/widget/WidgetStatus.java#L114
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
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
- https://proandroiddev.com/android-alarmmanager-as-deep-as-possible-909bd5b64792
- https://developer.android.com/guide/topics/appwidgets
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
- For now I have set alarm repeat time to
15 sec. for both Notification Alarm and Notification Widget. - For ease of logging I have given the tag
AACCfor all the changes. I have implemented. - Test first without widget and after adding widget.
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.
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.
I thought the point of this change was to find another mechanism which doesn't use the widget to dispatch notifications? AlarmManager + service for example? We can't guarantee that a user has a widget, and we want to dispatch notifications without it.
I thought the point of this change was to find another mechanism which doesn't use the widget to dispatch notifications? AlarmManager + service for example? We can't guarantee that a user has a widget, and we want to dispatch notifications without it.
Exactly that - the widget should just be a widget all by itself, with no connection to notifications
The notifications should work 100% without the widget, according to notification configuration
That there is any connection between the two (other than them both relying on the same app state and scheduler info) is (to me) bizarre and an abomination that should be removed
(that said: I tried at one point to do the same and couldn't get through it, so I'm not complaining, just stating how I feel ;-) )
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.
Oh!! Really Sorry. I miss completely misunderstood this . I will find solution for this ASAP.
PS: For now the last 2 commits is completely unessary.
@mikehardy @david-allison-1 Please review the plan.
I don't think we want to reuse the alarm. I feel we'd be better with two alarms, as that reduces complexity further as it removes the double query of the data store and the dispatch.
The most important thing to me is finding the mechanism to display the notification once the app's been stopped and likely isn't in the background (like how a messenger still shows messages if it's not running).
Okay, I Understood your point. You want to work me like this.
Major Drawback with this approach is that widget and notification will not be synchronous with each other. eg. due card is 150 and notification alarm trigger which shows the notification of due card 150. After 30 sec Due cards count updates to 155. and widget alarm triggers and updates the widget. Then the widget will show 155 due cards where as notification is still showing 150 which confuses the user and user is not able to figure out correct due cards count. Thats why I Suggested the above approach.
The most important thing to me is finding the mechanism to display the notification once the app's been stopped and likely isn't in the background (like how a messenger still shows messages if it's not running).
This and above approach both will run in background when alarm triggers (even when app is killed) and is completely separate from complete app.
as that reduces complexity further as it removes the double query of the data store and the dispatch.
I can reduce the double fetching of data in first approch with the help of storing the number of due cards shown in last notification/widget update and comparing it with current due card number. If both are same then don't trigger the services (in context of first approcach
I'm personally fine with that caveat (as long as it's documented, and until we get a user complaint).
Once we do, I feel we want the dependency to be from NotificationService -> Widget, rather than a general dispatch method which goes to both.
I predict that most users using the widget only use it to enable notifications.
I think the "30 minute" wait is too long. If there's no restriction, then checking each minute seems sensible (and determining a threshold to re-show the notification if it's already been displayed)
I'm personally fine with that caveat
Okay. I will work according to new plan.
Once we do, I feel we want the dependency to be from NotificationService -> Widget, rather than a general dispatch method which goes to both.
Sure, we can plan this in future. (If we get complains from users?)
DELETING LAST 2 COMMITS
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).
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
The ranking of terrible Android implementations changes constantly, this site is useful: https://dontkillmyapp.com/
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
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
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[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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
@david-allison-1 Sorry I am unable to proceed this further as I am busy in my college classes.
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