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

Implement API26 Background Limitations #4928

Merged
merged 2 commits into from
Sep 8, 2018

Conversation

mikehardy
Copy link
Member

@mikehardy mikehardy commented Sep 5, 2018

Pull Request template

Please, go through these checks before you submit a 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

Purpose / Description

If we don't target API>=26 before Nov 1, we will no longer be able to publish to the Play Store

Fixes

#4883

Approach

After investigating all of our service usage, it appeared that we were very close to purely event-based in behavior, though set up as service-oriented. I made the app purely event-based so we don't have to consider background service limitations at all and can now successfully target API 26

I optimized that diff for patch size + review by keeping all the logic in the XXXService objects even though they are just Receivers now. Post-review, moving XXXService to XXXReceiver might be more object-oriented pure.

How Has This Been Tested?

So far only manual testing in API 16, API 24 and API 27 emulators with the SmallWidget on the desktop, and deck reminders set. They triggered successfully inside a single run plus across reboots, and the widget status stayed correct

Learning (optional, can help others)

This ended up being simpler than I thought once I realized we could run purely event-based.

@ankidroid ankidroid deleted a comment Sep 5, 2018
@ankidroid ankidroid deleted a comment Sep 5, 2018
@mikehardy mikehardy force-pushed the api26/background-limitations branch from 8b407d1 to 5f8538c Compare September 5, 2018 21:37
@mikehardy
Copy link
Member Author

@timrae since you merged #4922 I rebased this so it's clean

@mikehardy mikehardy changed the title Api26/background limitations Implement API26 Background Llimitations Sep 5, 2018
@ankidroid ankidroid deleted a comment Sep 5, 2018
@timrae
Copy link
Member

timrae commented Sep 6, 2018

If it's purely event based then doesn't this mean that study reminders will only work when the AnkiDroid process is running?

@mikehardy
Copy link
Member Author

Nope, we build calendars, send them in to the alarm manager, it broadcasts events that we receive, we build notifications and display them right then and schedule the next reminder (as needed). We were already doing all of that except just the notification build was happening in a spawned service. I stuffed that into the event reception and voila

@ankidroid ankidroid deleted a comment Sep 8, 2018
@timrae timrae merged commit aa5c547 into ankidroid:master Sep 8, 2018
@timrae
Copy link
Member

timrae commented Sep 8, 2018

Thanks for this, it seems sane, though I think we need to merge these two notification features together into something more coherent before we release 2.9

@mikehardy
Copy link
Member Author

mikehardy commented Sep 8, 2018

Now that they are both merged I'll move from xxxservice to xxxreceiver and do non-logic cleanup as a PR separate. I liked that this was easy to review though as it was more of a thinking change

@timrae
Copy link
Member

timrae commented Sep 8, 2018

I think we need to merge these two notification features together into something more coherent

Just to clarify in case there was a misunderstanding, by "merge" I meant that instead of having two separate features for deck based reminders vs global reminders, we should make them both part of the same feature, and display them in a single place in the UI.

@mikehardy
Copy link
Member Author

Oh no a UI change. But I was thinking also to merge at least the back end to one notifications related receiver. I'll see about the front end. Might be time...

@mikehardy mikehardy deleted the api26/background-limitations branch September 8, 2018 13:21
@timrae
Copy link
Member

timrae commented Sep 8, 2018

Well the UI change is relatively trivial to implement once we decide what it should look like. The backend refactoring is the main part so that would be a big help

@mikehardy
Copy link
Member Author

Background service limitations are enforced on all apps running on API28 devices regardless of their targetSdkVersion, so release of this change is now required to publish updates to the store after Nov 1, and for anyone that uses API28

mikehardy added a commit to mikehardy/Anki-Android that referenced this pull request Sep 14, 2018
timrae pushed a commit that referenced this pull request Sep 15, 2018
@mikehardy mikehardy changed the title Implement API26 Background Llimitations Implement API26 Background Limitations Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants