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

Add stats provider #5057

Closed
wants to merge 12 commits into from
Closed

Conversation

mikehardy
Copy link
Member

@mikehardy mikehardy commented Oct 15, 2018

Pull Request template

Purpose / Description

Implement a stats provider for AnkiDroid, based on work by @MarcinMoskala

Fixes

  • Fixes Add stats provider #4881 Responds to comment on original PR asking for a contract
  • Moves original PR and API up to master (AndroidX, api levels etc)

Approach

Original PR has info: #4881

Contract and Provider were done with re-use of logic from card provider
I added a read-only permission so people didn't have to grant full access

How Has This Been Tested?

I added a demonstration to the apisample, and it's a good thing - the query didn't work without a quick fix - ankidroid/apisample#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

@mikehardy mikehardy requested a review from timrae October 15, 2018 06:41
@mikehardy mikehardy added this to the v2.9 release milestone Oct 15, 2018
@mikehardy mikehardy self-assigned this Oct 15, 2018
@mikehardy
Copy link
Member Author

Also self-reviewed and polished this one. Best to review it commit-by-commit otherwise my extraction of ProviderUtils makes things terrible to review, but if I didn't extract that I would have duplicated a lot of security logic in StatsProvider. This affects the API so I will not merge this without review, but I think it's ready to go.

Because Stats is in libanki it mostly got special dispensation vs adaptation
@timrae
Copy link
Member

timrae commented Oct 16, 2018

Perhaps you're not understanding the purpose of this contract file, since you're not using it in the sample project you made... The idea is that users copy the contract file and use that instead of hardcoding paths. Ideally it should be part of a higher level API like I mentioned in that PR.

@mikehardy
Copy link
Member Author

Okay, so something like AddContentApi.java, how do you manage development across api and apisample? By which I mean how do you do a local "api build" such that apisample can consume it with gradle dependencies and everything? Just a gradle target and a mavenLocal() repo add in apisasmple? I only see a jar dumped into a sub-directory in the api build.gradle so I'm not sure how to iterate over on the apisample side

For google-analytics-java I have a local deploy (provided by maven as mvn install) that puts jar into $HOME/.m2 and then if I add mavenLocal() as a gradle dep, the version is ends in 'SNAPSHOT', and tell gradle to check changing (i.e., SNAPSHOT) artifacts every time, I have very nice cross-project iteration.

That would make working on the contract file and API a lot more friendly.

As for the UI, yeah it's tacked on :-). Here I figured I'd get extra credit for at least exercising the API (and finding a bug! and realizing permissions were unhandled in StatsProvider) and I get dinged on bad UX. I mean, you're right, but my inner child is going "do I have to..."

@timrae
Copy link
Member

timrae commented Oct 16, 2018

how do you manage development across api and apisample?

apisample is a totally different project from the main AnkiDroid project, so I release a new version of the API to bintray before doing work there.

As for the UI, yeah it's tacked on :-). Here I figured I'd get extra credit for at least exercising the API (and finding a bug! and realizing permissions were unhandled in StatsProvider) and I get dinged on bad UX. I mean, you're right, but my inner child is going "do I have to..."

If you just want to test that your code is working in the content provider, then your approach here is fine, in which case no need to open a PR. If you want to make a fully featured stats API and for developers to be able to easily use it correctly by giving them sample code, then yes, you need to think about UX as well.

@mikehardy
Copy link
Member Author

OK - I think can go for full-featured. Let me add some tooling across the projects to do what I mentioned (local maven install available on one side, dep resolution consuming local on the other) so the iteration isn't painful because no, I have no experience in contract files, and little experience in APIs - so I iterate a lot. I'll ping you again if I have questions or when they're in shape

@mikehardy mikehardy modified the milestones: v2.9 release, v2.10 release Dec 2, 2018
@mikehardy
Copy link
Member Author

This one doesn't necessarily hold up 2.9 because it doesn't involve strings - we can get betas out prior to completing it but I'm still worried about 2.9 timing - I'd rather see it happen sooner than later so I'm moving milestones

@mikehardy mikehardy modified the milestones: v2.10 release, 2.11 release Feb 15, 2020
@mikehardy mikehardy modified the milestones: 2.11 release, 2.12 release May 25, 2020
@mikehardy mikehardy modified the milestones: 2.12 release, 2.13 release Jun 19, 2020
@github-actions
Copy link
Contributor

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 github-actions bot added the Stale label Aug 18, 2020
@github-actions github-actions bot closed this Aug 25, 2020
@mikehardy mikehardy deleted the add_stats_provider branch October 30, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants