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

Switch the auto advance feature to use the settings in the deck options #15224

Merged
merged 10 commits into from
Jan 20, 2024

Conversation

abdnh
Copy link
Contributor

@abdnh abdnh commented Jan 14, 2024

Purpose / Description

This switches the auto advance feature to use the shared settings in the deck options.

Fixes

Approach

  • The code was changed to use the secondsToShowQuestion and secondsToShowAnswer deck config properties. AnkiDroid-specific preference/config values are no longer used, except for the timeoutAnswer preference used to toggle the feature (The computer version requires the user to explicitly toggle it on in every review session - see Add auto-advance options to deck preset ankitects/anki#2765 (review)).
  • The preferences to set the timeouts and answer action were removed.
  • The "Wait for audio" option didn't exist in the AnkiDroid implementation (the default was to always wait), so it was added.

Before

image

After

image

How Has This Been Tested?

Manual testing in the emulator.

Learning (optional, can help others)

Refer to ankitects/anki#2765

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • 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

  • 'Answer Easy' is not available in the computer version. @dae I assume this is intentional? (UPDATE: found this post in the forums)

Copy link
Contributor

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

@@ -132,7 +132,7 @@
<string name="notification_minimum_cards_due_blink" maxLength="41">Blink light</string>
<string name="timeout_answer_text" maxLength="41">Automatic display answer</string>
<string name="timeout_answer" maxLength="41">Timeout answer</string>
<string name="timeout_answer_summ">Show answer automatically without user input. Delay includes time for automatically played audio files.</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add a new string so that translators get notified? That's the case for Pontoon. Not sure about Crowdin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, this only changes the english translation. Change the string key to trigger translations everywhere

@abdnh
Copy link
Contributor Author

abdnh commented Jan 14, 2024

I'm getting random errors like this while running unit tests locally using ./gradlew jacocoUnitTestReport:

java.io.IOException: Unable to create parent directories of C:\Users\Abdo\AppData\Local\Temp\6172405558266228753\org\robolectric\android-all-instrumented\13-robolectric-9030017-i4\android-all-instrumented-13-robolectric-9030
017-i4.jar
            at com.google.common.io.Files.createParentDirs(Files.java:479)
            at org.robolectric.internal.dependency.MavenArtifactFetcher.createArtifactSubdirectory(MavenArtifactFetcher.java:172)
            at org.robolectric.internal.dependency.MavenArtifactFetcher.fetchArtifact(MavenArtifactFetcher.java:73)
            ... 76 more

UPDATE: fixed by running ./gradlew robolectricSdkDownload

@dae
Copy link
Contributor

dae commented Jan 15, 2024

Making it consistent with the other clients might be better in the long term, but that's perhaps better done in a future PR, as it will require some work to shift from the current automatic always-on approach to an action that toggles it on/off.

Edit: sorry, looks like I replied to a deleted comment.

@BrayanDSO
Copy link
Member

Edit: sorry, looks like I replied to a deleted comment.

If this were about my comment, I asked if it wouldn't be better to remove the global option to keep it consistent with the desktop version. But I came to the same conclusion of Damien and deleted my comment minutes later

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks a lot

@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label Jan 19, 2024
* @see AutomaticAnswerAction
*/
const val CONFIG_KEY = "automaticAnswerAction"
const val CONFIG_KEY = "answerAction"
Copy link
Member

@david-allison david-allison Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/non-blocking: better to inline this in he callers using with the constant from libAnki

@david-allison
Copy link
Member

david-allison commented Jan 19, 2024

@abdnh happy with a squash-merge The pull request currently requires maintainers to "Squash Merge" here, or want to rebase so all commits compile and we can rebase merge?

Blocked on Strings

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge The pull request currently requires maintainers to "Squash Merge" and removed Needs Second Approval Has one approval, one more approval to merge labels Jan 19, 2024
@abdnh
Copy link
Contributor Author

abdnh commented Jan 20, 2024

Let's go with a squash merge.

@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Jan 20, 2024
@BrayanDSO
Copy link
Member

Strings merged. Pending squash @david-allison

@david-allison david-allison merged commit 9458ab0 into ankidroid:main Jan 20, 2024
7 checks passed
@github-actions github-actions bot added this to the 2.17 release milestone Jan 20, 2024
@github-actions github-actions bot removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge The pull request currently requires maintainers to "Squash Merge" labels Jan 20, 2024
@david-allison
Copy link
Member

david-allison commented Jan 20, 2024

Perfect, thanks all!

@abdnh abdnh deleted the deckoptions-auto-advance branch January 21, 2024 15:29
Copy link
Contributor

Hi there @abdnh! This is the OpenCollective Notice for PRs merged from 2024-01-01 through 2024-01-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants