Sync from reviewer when we didn't want through the deck picker #17399

Arthur-Milchior · 2024-11-08T23:10:03Z

Sync enabled in the reviewer when the deck picker was not used

Widgets and shortcuts allows to directly review a deck.
This means that automated sync, or even manual sync, is never
available. Except if the user close the reviewer and then manually
open ankidroid. This create a lot of friction on a very important
feature.

This commit enable the sync feature in the reviewer, but hides it
unless the intent actually requested it.

In my opinion, it should be always available. It's available in anki
upstream from the reviewer, and anki allows automated save every 10
minutes. Which means even if your device suddenly dies during a
streak, you only lose up to 10 minutes of review.

Still, this feature at least improve the case that was the most
dangerous for users (and frustrating for me)

I don't know the code of Reviewer2/ReviewerFragment. So this is only
enabled in the legacy reviewer. I hope the code is simple enough for
the feature to be easily ported to the other reviewers now that the
entire sync part of the code was decoupled from the DeckPicker.

Manually tested with:

signin in (when no account present)
full sync (requested locally and requested by ankiweb)
normal sync
changing the selected deck on the other device

To test it you should use a widget or a shortcut.

One point amy need to be changed. The little icon that state that
there is a not-uploaded change. While it's okay in the deckpicker, it
may not be ideal to have it as soon as we have the first review done.

Fixes Widget/shortcut should allow to sync #17084

david-allison

I don't know the code of Reviewer2/ReviewerFragment. So this is only
enabled in the legacy reviewer. I hope the code is simple enough for
the feature to be easily ported to the other reviewers now that the
entire sync part of the code was decoupled from the DeckPicker.

From an architecture perspective, supporting the new reviewer is the most important thing to get right. We want to move to the new reviewer (which affects everyone; this change only affects a subset of users), and if this PR doesn't support the new reviewer, then we're adding more friction to switching to the reviewer

I've reviewed the first two commits and they both look great. I'd strongly support a new PR to split them out if the rework of this PR will take some time.

AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt

Arthur-Milchior · 2024-11-11T01:28:21Z

Done in #17417

I admit I don't know the new reviewer at all. I didn't even know a new reviewer was considered. I'll take a look

david-allison · 2024-11-11T01:34:00Z

Done in #17417

I admit I don't know the new reviewer at all. I didn't even know a new reviewer was considered. I'll take a look

It's in the developer options, it looks great and I feel we all strongly want to make it the default

Arthur-Milchior · 2024-11-11T08:49:16Z

so I tried.
And the issue is that the widget and the shortcut are hardcoded for the first reviewer,even when the settings state to open the second one.
So in order to test the feature, I should first update the widget and shortcut

Arthur-Milchior · 2024-11-11T18:19:09Z

Rebased on top of #17422 so that the deck picker widget can use the new reviewer.

Arthur-Milchior · 2024-11-11T18:20:13Z

Feature is now ported to the new reviewer.
I don't know how currently this reviewer decide what is displayed or not. I decide to let it displayed if the deck picker was skipped

david-allison · 2024-11-11T21:25:38Z

Conflicts

Arthur-Milchior · 2024-12-04T03:30:22Z

Conflict resolved

Arthur-Milchior · 2024-12-07T03:10:52Z

I split it some of its commits in #17570 and #17569 so that they can be reviewed faster and hopefully don't get too much merge conflict.
This will ensure this PR only contains:

the code move from DeckPicker to Sync
the use of Sync from the reviewer

mikehardy · 2024-12-07T18:12:47Z


> Task :AnkiDroid:ktlintMainSourceSetCheck FAILED
/home/runner/work/Anki-Android/Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt:25:1 Unused import
/home/runner/work/Anki-Android/Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt:283:1 Unexpected indentation (16) (should be 12)
/home/runner/work/Anki-Android/Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt:648:5 Declarations and declarations with comments should have an empty space between.

minor lint issue

mikehardy · 2024-12-07T18:15:29Z

AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt

+/**
+ * Class in charge of doing the sync work for another activity.
+ */
+class SyncHandler(


Sync.kt is already pretty big isn't it? Could this reasonably go in its own file?

mikehardy

Excited to see this go in, looks like there was a lot of progress on the build-up refactors etc and most of those will likely be in today or tomorrow with a bit more back and forth. This one maybe tomorrow or next day depending on time available

Arthur-Milchior · 2024-12-07T20:00:36Z

I tried ./gradlew lint and got

milchior@Liamwayl:~/ankidroid$ ./gradlew lint
Starting a Gradle Daemon (subsequent builds will be faster)

BUILD SUCCESSFUL in 1m 18s
153 actionable tasks: 25 executed, 5 from cache, 123 up-to-date

would you have any idea why it success locally and not on github?

mikehardy · 2024-12-07T20:41:18Z

would you have any idea why it success locally and not on github?

no idea but looks like it resolved :-)

Currently it's only an extra indirection layer, as all provider are returning themselves. In the next commit, it'll be used to ensure that the activity don't have to implements the listener itself.

This class deals with the entire Sync part of the deck picker.

The enum was used only in one function. Adding an intermediary step does not add clarity to the code. Each method were also only used once. I think the resulting code is easier to read, thanks to less layer of abstractions.

Widgets and shortcuts allows to directly review a deck. This means that automated sync, or even manual sync, is never available. Except if the user close the reviewer and then manually open ankidroid. This create a lot of friction on a very important feature. This commit enable the sync feature in the reviewer, but hides it unless the intent actually requested it. In my opinion, it should be always available. It's available in anki upstream from the reviewer, and anki allows automated save every 10 minutes. Which means even if your device suddenly dies during a streak, you only lose up to 10 minutes of review. Still, this feature at least improve the case that was the most dangerous for users (and frustrating for me) I don't know the code of Reviewer2/ReviewerFragment. So this is only enabled in the legacy reviewer. I hope the code is simple enough for the feature to be easily ported to the other reviewers now that the entire sync part of the code was decoupled from the DeckPicker. Manually tested with: * signin in (when no account present) * full sync (requested locally and requested by ankiweb) * normal sync * changing the selected deck on the other device To test it you should use a widget or a shortcut. One point amy need to be changed. The little icon that state that there is a not-uploaded change. While it's okay in the deckpicker, it may not be ideal to have it as soon as we have the first review done.

david-allison

Fundamentally: I don't feel we should have a UI to sync from the reviewer.

I wouldn't mind it via the JS API, or being performed automatically on open/close, but not as a UI option. It turns the Reviewer into a "do everything" screen

david-allison · 2024-12-11T23:35:39Z

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/SyncErrorDialog.kt

@@ -33,7 +33,11 @@ import com.ichi2.anki.showError
 import com.ichi2.anki.utils.ext.dismissAllDialogFragments

 class SyncErrorDialog : AsyncDialogFragment() {
-    interface SyncErrorDialogListener {
+    interface SyncErrorDialogListenerProvider {


Can we remove this and use a delegation?

https://kotlinlang.org/docs/delegation.html#overriding-a-member-of-an-interface-implemented-by-delegation

david-allison · 2024-12-11T23:37:21Z

AnkiDroid/src/main/java/com/ichi2/anki/MyAccount.kt

+                    throw exc
+                }
+            updateLogin(baseContext, username, auth.hkey)
+            setResult(RESULT_OK)


This looks incorrect: the activity doesn't wait for the task to complete

david-allison · 2024-12-12T00:05:33Z

AnkiDroid/src/main/java/com/ichi2/anki/SyncHandler.kt

+ * this program.  If not, see <http://www.gnu.org/licenses/>.                           *
+ ****************************************************************************************/
+
+package com.ichi2.anki


can we move this into a subpackage?

david-allison · 2024-12-12T00:06:36Z

AnkiDroid/src/main/java/com/ichi2/anki/SyncHandler.kt

+/**
+ * Class in charge of doing the sync work for another activity.
+ */
+class SyncHandler(


This is far too coupled to implementation details for it to be worthwhile. the UI code shouldn't be here

david-allison · 2024-12-12T00:08:43Z

AnkiDroid/src/main/java/com/ichi2/anki/IntentHandler.kt

@@ -153,6 +154,9 @@ class IntentHandler : AbstractIntentHandler() {
            } else {
                Intent(this, Reviewer::class.java)
            }
+        if (intent.extras?.getBoolean(ENABLE_SYNC) == true) {


Should this be handled by the reviewer at all if there's no UI updates? Can we do it when we receive an intent instead?

david-allison · 2024-12-12T00:09:40Z

AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt

@@ -206,6 +224,12 @@ open class Reviewer :

    private val flagItemIds = mutableSetOf<Int>()

+    // flag keeping track of when the app has been paused
+    var activityPaused = false


This should be using lifecycle aware components, or querying the lifecycle manually

david-allison · 2024-12-12T00:10:18Z

AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt

@@ -409,6 +445,18 @@ open class Reviewer :
                Timber.i("Reviewer:: Home button pressed")
                closeReviewer(RESULT_OK)
            }
+            R.id.action_sync -> {


I don't think this belongs here. If a user wants to sync from a widget, they can turn on auto-sync and have it handled by the code you're adding

Arthur-Milchior force-pushed the sync_handler branch 2 times, most recently from 835a76e to b2e9176 Compare November 9, 2024 00:27

david-allison requested changes Nov 10, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt Outdated Show resolved Hide resolved

david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 10, 2024

Arthur-Milchior mentioned this pull request Nov 11, 2024

Sync separate #17417

Closed

Arthur-Milchior mentioned this pull request Nov 11, 2024

New reviewer is not opened from widget/shortcuts #17420

Closed

Arthur-Milchior force-pushed the sync_handler branch from b2e9176 to 9412182 Compare November 11, 2024 18:19

Arthur-Milchior requested a review from david-allison November 11, 2024 18:20

github-actions bot added the Has Conflicts label Nov 11, 2024

david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 11, 2024

Arthur-Milchior force-pushed the sync_handler branch from 9412182 to f0a5b4f Compare November 11, 2024 23:31

Arthur-Milchior removed the Has Conflicts label Dec 4, 2024

Arthur-Milchior force-pushed the sync_handler branch from f0a5b4f to 55c351d Compare December 4, 2024 03:31

github-actions bot added the Has Conflicts label Dec 5, 2024

Arthur-Milchior force-pushed the sync_handler branch from 55c351d to f3f5180 Compare December 7, 2024 02:47

Arthur-Milchior removed the Has Conflicts label Dec 7, 2024

Arthur-Milchior force-pushed the sync_handler branch 3 times, most recently from 9d96fc9 to 8b053c3 Compare December 7, 2024 02:59

This was referenced Dec 7, 2024

Cleanup in preparation for adding sync in reviewers #17569

Merged

NF: Remove SyncStatus #17570

Closed

Arthur-Milchior force-pushed the sync_handler branch 2 times, most recently from b44f7d5 to 34a5e97 Compare December 7, 2024 03:12

github-actions bot added the Has Conflicts label Dec 7, 2024

Arthur-Milchior force-pushed the sync_handler branch from 34a5e97 to a1b7b11 Compare December 7, 2024 16:15

Arthur-Milchior removed the Has Conflicts label Dec 7, 2024

mikehardy reviewed Dec 7, 2024

View reviewed changes

Arthur-Milchior force-pushed the sync_handler branch from a1b7b11 to e85d76e Compare December 7, 2024 20:28

Arthur-Milchior mentioned this pull request Dec 7, 2024

Some cleaning of Database error dialog #17572

Merged

github-actions bot added the Has Conflicts label Dec 8, 2024

BrayanDSO mentioned this pull request Dec 10, 2024

Replace/Remove Close reviewer and sync #17586

Open

Arthur-Milchior force-pushed the sync_handler branch 2 times, most recently from 18c7462 to be10887 Compare December 11, 2024 21:52

Arthur-Milchior removed the Has Conflicts label Dec 11, 2024

Arthur-Milchior added 5 commits December 11, 2024 23:29

NF: introduce SyncErrorDialogListenerProvider

391986d

Currently it's only an extra indirection layer, as all provider are returning themselves. In the next commit, it'll be used to ensure that the activity don't have to implements the listener itself.

NF: Create SyncHandler

b0829e7

This class deals with the entire Sync part of the deck picker.

NF: Remove SyncStatus

c6ac589

The enum was used only in one function. Adding an intermediary step does not add clarity to the code. Each method were also only used once. I think the resulting code is easier to read, thanks to less layer of abstractions.

NF: Move sync files to their own subfolders

fcd137c

Arthur-Milchior force-pushed the sync_handler branch from be10887 to fcd137c Compare December 11, 2024 22:32

david-allison requested changes Dec 12, 2024

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Dec 12, 2024

github-actions bot added the Has Conflicts label Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync from reviewer when we didn't want through the deck picker #17399

Sync from reviewer when we didn't want through the deck picker #17399

Arthur-Milchior commented Nov 8, 2024 •

edited by david-allison

Loading

david-allison left a comment

Arthur-Milchior commented Nov 11, 2024

david-allison commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

david-allison commented Nov 11, 2024

Arthur-Milchior commented Dec 4, 2024

Arthur-Milchior commented Dec 7, 2024

mikehardy commented Dec 7, 2024

mikehardy Dec 7, 2024

mikehardy left a comment

Arthur-Milchior commented Dec 7, 2024

mikehardy commented Dec 7, 2024

david-allison left a comment

david-allison Dec 11, 2024

david-allison Dec 11, 2024

david-allison Dec 12, 2024

david-allison Dec 12, 2024

david-allison Dec 12, 2024

david-allison Dec 12, 2024

david-allison Dec 12, 2024

Sync from reviewer when we didn't want through the deck picker #17399

Are you sure you want to change the base?

Sync from reviewer when we didn't want through the deck picker #17399

Conversation

Arthur-Milchior commented Nov 8, 2024 • edited by david-allison Loading

david-allison left a comment

Choose a reason for hiding this comment

Arthur-Milchior commented Nov 11, 2024

david-allison commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

david-allison commented Nov 11, 2024

Arthur-Milchior commented Dec 4, 2024

Arthur-Milchior commented Dec 7, 2024

mikehardy commented Dec 7, 2024

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

Arthur-Milchior commented Dec 7, 2024

mikehardy commented Dec 7, 2024

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthur-Milchior commented Nov 8, 2024 •

edited by david-allison

Loading