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

Limit the displayed review count to 99999 instead of 1000 #5326

Merged
merged 1 commit into from
Jun 14, 2019

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Jun 13, 2019

Purpose / Description

Unlimit the displayed review counts, by limiting them to 99999 instead of 1000, as per #5197 (comment)

Fixes

Fixes #5092, #5197 and #5322

Approach

I don't know Ankidroid's architecture, and at parts of what I've seen it sounds like it aims to be almost a transcription of the python code, so I've changed as little as possible.

Note that here I think I change the constants limiting both the study counters and the deck list counters. Potentially the slowdown caused by counting all the full deck counters will be a problem, in which case it should be possible to revert the Sched{,V2}.java parts of it, I think (or roll them back to “just” 9999, maybe?). This would unfix #5092 and #5197, though, but keep #5322 fixed.

How Has This Been Tested?

This is the first time I attempt to contribute to an android project, so it has not actually been tested. Given what the code around it looks like, I'm relatively confident it's OK-ish, though.

Learning (optional, can help others)

Grep'd through the source code for 1000, looked at all occurrences: rg -w 1000 | rg -v '............................................................................................................................................................................................................................' > 1000-occurrences && vim 1000-occurrences

Checklist

  • 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

@Ekleog
Copy link
Contributor Author

Ekleog commented Jun 13, 2019

(I'm pretty sure the travis failure is unrelated to this PR, as it looks like a missing dependency in the .travis.yml -- I feel forced to suggest using nix for building ankidroid, as it can provide perfectly reproducible builds that should make it impossible that something like this happen when using it, but feel free to ignore me :))

@mikehardy
Copy link
Member

mikehardy commented Jun 13, 2019

Yeah the CI failure is unrelated. How apropos that your first android PR would have a CI failure (testing Android in CI is notoriously painful because of lack of nested virtualization in CI envs). But it relying on Travis' CI images to remain stable has been a safe bet for a couple years vs the pain of taking a raw image in their env and keeping track of my own dependencies. And this looks like a failure in their image to have a fundamental android dev tool, which isn't something that should happen. I'll dig into it in a bit

@mikehardy
Copy link
Member

Underlying operating system changed from debian trusty to xenial per a long-term migration on Travis' part. Our builds were 100% on trusty and 0% on xenial when they migrated our project a couple days ago apparently. They specify in their docs that they only support android on trusty right now, so rather than not specifying and accepting the default we have to specify dist: trusty and we're good

#5327 fixed it - if you rebase from master and force push your branch out, it should be a clean commit on a working CI config then I can look at the PR for real...

@Ekleog Ekleog force-pushed the unlimit-display branch from ef0a50b to b67397e Compare June 13, 2019 23:02
@Ekleog
Copy link
Contributor Author

Ekleog commented Jun 13, 2019

Thank you! Rebased and force-pushed :)

@Ekleog
Copy link
Contributor Author

Ekleog commented Jun 13, 2019

(And I can confirm that the tests passed this time)

@timrae timrae merged commit d57e802 into ankidroid:master Jun 14, 2019
david-allison added a commit to david-allison/Anki-Android that referenced this pull request Mar 24, 2020
We modified `mReportLimit` in ankidroid#5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes it by using a new scheduler with the correct `mReportLimit`
for calculating the number of cards in the selected deck
david-allison added a commit to david-allison/Anki-Android that referenced this pull request Mar 24, 2020
We modified `mReportLimit` in ankidroid#5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes the issue by using a new scheduler with the correct
`mReportLimit` for calculating the number of cards in the selected deck
mikehardy pushed a commit that referenced this pull request Mar 24, 2020
Fixes #5666 - Sync Failure on Large Dynamic Decks

We modified `mReportLimit` in #5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes it by using a new scheduler with the correct `mReportLimit`
for calculating the number of cards in the selected deck
mikehardy pushed a commit that referenced this pull request Mar 26, 2020
Fixes #5666 - Sync Failure on Large Dynamic Decks

We modified `mReportLimit` in #5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes it by using a new scheduler with the correct `mReportLimit`
for calculating the number of cards in the selected deck
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.

Consistency with Anki: show "1000+" new cards instead of exact number
3 participants