Show notifications even when app is in background (fixes #4615) #4697

Nutomic · 2017-09-05T14:54:36Z

Finally got around to this. There seems to be some bug left, as I couldn't get the Notification to show during my testing. This might have to do with the behaviour of setInexactRepeating(), according to the documentation, "there may be a delay of almost an entire alarm interval before the first invocation of the alarm" (so almost 24 hours delay in our case). We might have to use a different method to schedule the alarm instead.

By the way, I noticed that BootService would only get started by a device boot. In the following cases, it would never run:

app is newly installed/updated
app is force stopped and started again

I've added the line startService(new Intent(this, BootService.class)) in AnkiDroidApp to make sure it is always started.

Nutomic · 2017-09-06T06:40:26Z

Fixed, just need to handle settings changes now.

Nutomic · 2017-09-06T10:59:11Z

Should be complete now, assuming I understood the settings right.

timrae · 2017-09-11T21:03:09Z

@madhead

Would you be able to review this?

madhead · 2017-09-11T21:39:33Z

@Nutomic

Regarding the BootService:

app is newly installed/updated

In case of fresh installation there will be no existing notification preferences. Not sure about application updates, but maybe the existing alarms will work?

app is force stopped and started again

But it implies that the device already booted, so the alarms are in place.

Regarding to the SO answers and articles I've read, boot (completed) event is enough for the notifications to work after reboots: 1, 2.

Also, I see this code touches existing logic for the widget, isn't it? What is the purpose?

Thanks!

madhead · 2017-09-11T21:43:07Z

@timrae, what were the original reasons of declining #4006 / #4450 was it only "inexact" timing or there were other defects?

Thanks!

timrae · 2017-09-11T22:09:10Z

@madhead

Thanks for the review.

"Decline" is a strong word; it's in the master (i.e. 2.9 branch), just not in the 2.8 release. :)
As far as I remember #4525 and #4530 were the main ones, but here is the list of notification related issues:

https://github.com/ankidroid/Anki-Android/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20label%3Aadvanced-reminders%20

Nutomic · 2017-09-12T01:10:02Z

@madhead One easy way to break it is by running the app from Android Studio. That's not a boot, so the receiver will not be called (verified with logs). The same thing should happen if the app is updated from Google Play, or if the app is force stopped from app settings, and started again.

Regarding the widget, it was starting the NotificationService on every update. Now that the service is started automatically via the BootService, there's no reason to start it from the widget any more.

Nutomic · 2017-09-12T01:16:02Z

@timrae Most of these issues seem related to the app logic/settings logic, nothing I can comment on. But #4525 is caused by the fact that you are using setInexactRepeating(). But tbh 7 minutes delay seems fine to me.

timrae · 2017-09-12T02:17:26Z

7 minutes delay is ok with me, but you know some of our users are Anki fanatics, and they don't like it... Also, people who were testing the feature for the first time will keep opening issues to say that it's "broken", which wastes our time as well :-/ when I looked into it, it seemed very easy to make it more exact, you just have to set a new alarm each time it gets fired

timrae · 2017-09-12T02:20:26Z

Honestly I'm not super knowledgeable about services in Android, and haven't looked through the code in detail, but a pre-requisite for accepting this PR is that it doesn't noticeably impact battery usage, so I'm worried that the service is gonna be permanently running.

Nutomic · 2017-09-12T02:56:13Z

Its the same service that was there before, and its only started once (doesnt run in background). Using exact alarms would increase battery usage in theory, but that's probably not noticeable if its only once per day.

timrae · 2017-09-12T03:27:54Z

AnkiDroid/src/main/java/com/ichi2/anki/services/BootService.java

    public BootService() {
        super("BootService");
    }

    @Override
    protected void onHandleIntent(Intent intent) {
-        final AlarmManager alarmManager = (AlarmManager) getSystemService(ALARM_SERVICE);
+        if (sWasRun)


Please use braces with your one-line if statements

timrae · 2017-09-12T03:30:29Z

AnkiDroid/src/main/java/com/ichi2/anki/services/BootService.java

+    public static void scheduleNotification(Context context) {
+        AlarmManager alarmManager = (AlarmManager) context.getSystemService(ALARM_SERVICE);
+        SharedPreferences sp = PreferenceManager.getDefaultSharedPreferences(context);
+        if (Integer.parseInt(sp.getString("minimumCardsDueForNotification", "1000001")) <= 1000000)


Maybe it's time we separated this into two settings instead of using the "magic number" 1000000 to represent "pending messages available"? We originally did this in an attempt to minimize the number of settings, but it's kind of counter-productive here.

timrae · 2017-09-12T03:41:32Z

@madhead @Nutomic

I think it's a bit confusing having two different places where notifications are set... Maybe we can merge these two similar features to some extent? Is the main motivation for #4530 to allow opting-out of notifications for specific decks? Or is it to have a separate reminder (with a separate notification time) for each deck?

Nutomic · 2017-09-12T06:21:16Z

Yes I think it makes sense to seperate the notification settings. But I'm don't think it necessarily belongs into this PR. Also, like I said, I'm not really familiar with the settings.

madhead · 2017-09-12T09:01:47Z

Or is it to have a separate reminder (with a separate notification time) for each deck?

The original idea was this. Each deck can remind about itself at some time of the day, independently of others.

timrae · 2017-09-12T18:35:13Z

Or is it to have a separate reminder (with a separate notification time) for each deck?

The original idea was this. Each deck can remind about itself at some time of the day, independently of others.

At the moment it's done on a per-note-type rather than per-deck basis though, right?

timrae · 2017-09-12T19:06:21Z

I fixed the failing build issue, so if you could also rebase onto the latest master that would be appreciated!

git fetch --all
git rebase upstream/master
git push origin background-notifications -f

Nutomic · 2017-09-13T03:28:41Z

done ;)

timrae · 2017-09-13T03:33:29Z

Thanks! Can you add the missing curly braces as well?

Nutomic · 2017-09-13T04:02:03Z

Right, I missed your comment before.

Nutomic force-pushed the background-notifications branch from 51eb3c4 to 273c93c Compare September 5, 2017 14:54

Nutomic changed the title ~~[WIP] Show notifications even when app is in background (fixes #4615)~~ Show notifications even when app is in background (fixes #4615) Sep 11, 2017

timrae suggested changes Sep 12, 2017

View reviewed changes

Nutomic added 3 commits September 13, 2017 12:15

Show notifications even when app is in background (fixes ankidroid#4615)

e7a1c46

call correct receiver, dont run BootService more than once

6c6ae79

handle settings changes

681392f

Nutomic force-pushed the background-notifications branch from ca315a1 to 681392f Compare September 13, 2017 03:15

curly braces

26c979a

timrae merged commit 711e41f into ankidroid:master Sep 13, 2017

snowtimeglass mentioned this pull request Sep 13, 2017

Bug: The reminders in the deck options don't work on time #4525

Closed

tda1009 mentioned this pull request Dec 11, 2019

Fix "No notifications after update 2.9 and 2.9.1" (#5535) #5642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show notifications even when app is in background (fixes #4615) #4697

Show notifications even when app is in background (fixes #4615) #4697

Nutomic commented Sep 5, 2017

Nutomic commented Sep 6, 2017

Nutomic commented Sep 6, 2017

timrae commented Sep 11, 2017

madhead commented Sep 11, 2017

madhead commented Sep 11, 2017

timrae commented Sep 11, 2017

Nutomic commented Sep 12, 2017

Nutomic commented Sep 12, 2017

timrae commented Sep 12, 2017 •

edited

Loading

timrae commented Sep 12, 2017

Nutomic commented Sep 12, 2017

timrae Sep 12, 2017

timrae Sep 12, 2017

timrae commented Sep 12, 2017

Nutomic commented Sep 12, 2017

madhead commented Sep 12, 2017

timrae commented Sep 12, 2017 •

edited

Loading

timrae commented Sep 12, 2017

Nutomic commented Sep 13, 2017

timrae commented Sep 13, 2017

Nutomic commented Sep 13, 2017

Show notifications even when app is in background (fixes #4615) #4697

Show notifications even when app is in background (fixes #4615) #4697

Conversation

Nutomic commented Sep 5, 2017

Nutomic commented Sep 6, 2017

Nutomic commented Sep 6, 2017

timrae commented Sep 11, 2017

madhead commented Sep 11, 2017

madhead commented Sep 11, 2017

timrae commented Sep 11, 2017

Nutomic commented Sep 12, 2017

Nutomic commented Sep 12, 2017

timrae commented Sep 12, 2017 • edited Loading

timrae commented Sep 12, 2017

Nutomic commented Sep 12, 2017

timrae Sep 12, 2017

Choose a reason for hiding this comment

timrae Sep 12, 2017

Choose a reason for hiding this comment

timrae commented Sep 12, 2017

Nutomic commented Sep 12, 2017

madhead commented Sep 12, 2017

timrae commented Sep 12, 2017 • edited Loading

timrae commented Sep 12, 2017

Nutomic commented Sep 13, 2017

timrae commented Sep 13, 2017

Nutomic commented Sep 13, 2017

timrae commented Sep 12, 2017 •

edited

Loading

timrae commented Sep 12, 2017 •

edited

Loading