More backend integration work (undo, progress in GUI, cancellation, refactoring) #11740
Conversation
dae
force-pushed
the
new-anki5
branch
2 times, most recently
from
12c2b12
to
2e6d415
Compare
|
So, the issue is obvious in hindsight - the launched coroutines need to be awaited to ensure they've completed before the test continues. The ShadowLooper.runUiThreadTasksIncludingDelayedTasks() calls added a delay which helped, but the tests were still flaking occasionally. I've added a helper in 03962bf to make this easy in tests. |
|
Windows failure looks like #10101 |
|
haven't looked at the code but restarted windows test |
dae
force-pushed
the
new-anki5
branch
4 times, most recently
from
bfee92f
to
de0d5a6
Compare
dae
force-pushed
the
new-anki5
branch
2 times, most recently
from
159a108
to
a1a4ad6
Compare
|
(rebased due to conflict) |
|
This is next up for review + merge. I just synced translations (a couple times...) last night, merged the easy stuff and kicked off an alpha release to clear the decks so we have a clean tag before these go in, in case there are any issues Will attempt to merge this one, then #11808 in series, Anki-Android-Backend will proceed in parallel, then I will point AnkiDroid to new Anki-Andrioid-Backend and merge #11881 if you want to start prepping those (or getting prepped mentally). Will be an all day process here I think as I do it in and among travel-related tasks |
There was a problem hiding this comment.
Something crashed for me in a forced full sync in legacy mode while testing this locally on a throwaway test collection I use (full of junk cards but nothing pathological, it normally works fine) - ./gradlew installPlayDebug then force full sync, review a couple cards, I think I imported another deck with media, then force full sync choose ankiweb to overwrite local,
07-20 09:53:21.960 24875 25264 D FullSyncer: Full Sync: Downloaded file was not corrupt
07-20 09:53:21.961 1444 9515 I MediaProvider: Observed non-standard /storage/emulated/0/AnkiDroid/.nomedia
07-20 09:53:21.961 1444 9515 I MediaProvider: Observed non-standard /storage/emulated/0/AnkiDroid/.nomedia
07-20 09:53:21.962 1444 9515 W MediaProvider: Forgot to handle a top level directory in getContentUriForFile?
07-20 09:53:21.965 1444 9515 W MediaProvider: Database update failed while renaming /storage/emulated/0/AnkiDroid/collection.anki2.tmp
07-20 09:53:21.965 1444 9515 W MediaProvider: android.database.sqlite.SQLiteConstraintException: UNIQUE constraint failed: files._data (code 2067 SQLITE_CONSTRAINT_UNIQUE)
07-20 09:53:21.965 1444 9515 W MediaProvider: at android.database.sqlite.SQLiteConnection.nativeExecuteForChangedRowCount(Native Method)
07-20 09:53:21.965 1444 9515 W MediaProvider: at android.database.sqlite.SQLiteConnection.executeForChangedRowCount(SQLiteConnection.java:892)
07-20 09:53:21.965 1444 9515 W MediaProvider: at android.database.sqlite.SQLiteSession.executeForChangedRowCount(SQLiteSession.java:756)
07-20 09:53:21.965 1444 9515 W MediaProvider: at android.database.sqlite.SQLiteStatement.executeUpdateDelete(SQLiteStatement.java:67)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.util.DatabaseUtils.executeUpdateDelete(DatabaseUtils.java:468)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.util.SQLiteQueryBuilder.update(SQLiteQueryBuilder.java:697)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.util.SQLiteQueryBuilder.lambda$update$2(SQLiteQueryBuilder.java:619)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.util.SQLiteQueryBuilder.$r8$lambda$HsU2MxAJo7HcqvsOurIzkF-GF5M(Unknown Source:0)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.util.SQLiteQueryBuilder$$ExternalSyntheticLambda2.apply(Unknown Source:10)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.DatabaseHelper.runWithTransaction(DatabaseHelper.java:574)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.util.SQLiteQueryBuilder.update(SQLiteQueryBuilder.java:618)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.updateDatabaseForFuseRename(MediaProvider.java:2079)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.updateDatabaseForFuseRename(MediaProvider.java:2055)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.updateDatabaseForFuseRename(MediaProvider.java:2049)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.renameFileForFuse(MediaProvider.java:2399)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.renameFileUncheckedForFuse(MediaProvider.java:2363)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.renameUncheckedForFuse(MediaProvider.java:2466)
07-20 09:53:21.965 1444 9515 W MediaProvider: at com.android.providers.media.MediaProvider.renameForFuse(MediaProvider.java:2513)
07-20 09:53:21.966 1444 9515 I MediaProvider: Retrying database update after deleting conflicting entry
07-20 09:53:21.985 24875 25264 I FullSyncer: Full Sync Success: Overwritten collection with downloaded file
07-20 09:53:21.985 24875 25264 I Collection: (Re)opening Database: /storage/emulated/0/AnkiDroid/collection.anki2
07-20 09:53:21.985 1444 1514 W MediaProvider: Failed to update quota for uri: content://media/external_primary/file/1931
07-20 09:53:21.985 1444 1514 W MediaProvider: java.io.FileNotFoundException: No item at content://media/external_primary/file/1931
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider.queryForSingleItem(MediaProvider.java:7335)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider.queryForDataFile(MediaProvider.java:7281)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider.queryForDataFile(MediaProvider.java:7273)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider.updateQuotaTypeForUri(MediaProvider.java:602)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider.access$1000(MediaProvider.java:254)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider$3.lambda$onUpdate$1(MediaProvider.java:689)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider$3.$r8$lambda$ZdUE6FQqSMahblcS35StEQHJ7ww(Unknown Source:0)
07-20 09:53:21.985 1444 1514 W MediaProvider: at com.android.providers.media.MediaProvider$3$$ExternalSyntheticLambda1.run(Unknown Source:8)
07-20 09:53:21.985 1444 1514 W MediaProvider: at android.os.Handler.handleCallback(Handler.java:938)
07-20 09:53:21.985 1444 1514 W MediaProvider: at android.os.Handler.dispatchMessage(Handler.java:99)
07-20 09:53:21.985 1444 1514 W MediaProvider: at android.os.Looper.loopOnce(Looper.java:201)
07-20 09:53:21.985 1444 1514 W MediaProvider: at android.os.Looper.loop(Looper.java:288)
07-20 09:53:21.985 1444 1514 W MediaProvider: at android.os.HandlerThread.run(HandlerThread.java:67)
🤔
I didn't get to this yesterday unfortunately and I'm in the airport now so low-availability will sadly continue. Was hoping this passed checks cleanly and I could slam it in prior to takeoff but no 😅
Maintainer question: I won' thave time for a while (as mentioned ad nauseum) so doing my previous style of checking per-commit and re-ordering things - which is pretty time-consuming - would delay things even more. I like the idea of separate commits here since the changes are related of course but also independently useful, but for velocity I think squash merge is more realistic. The only way to do separate commits would be to pop commits one by one while running what I do locally (CI analog) to verify, if interested. I may have time just can't commit. This is what I run when doing it anyway
/bin/nice -n 19 ./gradlew clean jacocoTestReport :api:lintRelease :AnkiDroid:lintPlayRelease ktlintCheck lint-rules:test
Here's my shell history command where I was popping off the last PR until I got to the right commit, possibly more for my reference than anything
1082 reset && while [
git rev-parse HEAD!= "79483d6a3b228a118604fc5488725deaec7ca4a6" ]; do echo not there yet currently at; git rev-parse HEAD; ./gradlew clean uninstallPlayDebug jacocoTestReport :api:lintRelease :AnkiDroid:lintPlayRelease ktlintCheck lint-rules:test; git reset --hard HEAD~1; done
|
It would be really useful - since with crashing I think the app will restart in non-V16 mode? - for the developer option of the V16 "tap to enable" to actually be a toggle and turn V16 on or off. Or at minimum (if it's difficult for any reason) for it to at least indicate if you are in V16 mode. Scoped Storage toggle would also be good to indicate if it was in scoped storage mode as well if that was for some reason really really easy since it's right next to it |
|
I might be mis-diagnosing unexpected-to-me LeakCanary activity as crashing, as a fair warning - as I test this and switch to V16 repeatedly, then do cycles of "force full sync -> full sync -> review a bit -> sync -> force full sync -> full sync" I get a lot of LeakCanary popups but I'm having difficulty finding stack traces. So maybe it is not crashing but LeakCanary is somehow triggered for another reason? I can't think of why that would happen, but perhaps it is possible [Edit: in fact, forcing full sync, or perhaps in the general case "changing some preferences"(?) does an Activity restart or similar when it goes back to DeckPicker and I think that is confusing LeakCanary - resulting in behavior that looks like a crash but is not in fact a crash - I'm seeing my V16 toggled-on state persist (as verified by the way syncing is behaving with the new sync code - and I am not seeing anything that looks like a crash. Stated differently: I think this is very close to working and I don't think I'm crashing in the full sync case on V16 at least except for those odd stacks pasted above on legacy case, which may have been a warning but did not seem to be a crash. Maybe it's all good to go...] |
|
I am not seeing a full sync on V16 case when I toggle on V16, use settings -> sync -> force full sync, then hit the sync button. I am seeing a normal sync right now |
Follow-up of ankidroid@b02515b + Convert Optional references to native Kotlin nullable types
- Progress is now shown in the GUI - Cancellation of tasks is supported - Removes some boilerplate and improves ergonomics - runInBackgroundWithProgress is now scoped to AnkiActivity. - When errors encountered, they are shown until user clicks OK. - Hooks up new backend to import->colpkg option Cancellation will not work correctly until the backend is updated, as the cancellation routines need to be excluded from the backend mutex - until updating, attempting to cancel will block the UI until the operation completes.
Can be tested by importing an apkg from the deck list, then choosing the undo option. - If a backend undo entry is present, it takes priority over the old undo system to ensure data integrity. AD's reviewer saves config vars and decks as part of the review process, and it was triggering undo entries for those changes, which prevent the v2 scheduler review from being undone. The calls have been updated to clear the backend undo, so that the review takes precedence. The desktop code has the same problem, and it will be avoidable once updating to the v3 scheduler. - Introduce a helper to await a job in unit tests, to avoid race conditions
+ Delete the cards inside the default deck if user tries to delete it Closes ankidroid#11703
CardTemplateEditorTest is no longer failing with the latest code 🎉
mCol was null. Technically libanki code should not be making reference to GUI code, but this code should be retired once AD has migrated to the new background syncing code anyway.
These toggles were implemented by @david-allison, and I think he deliberately wanted them to revert on restart when he introduced them. I don't actually use the toggle; I find it easier to modify local.properties. :-) I've seen LeakCanary messages pop up, and presume it's related to the way the toggles restart the activity. Maybe not an issue, since these are just temporary debug/testing tools?
Looks like something related to scoped storage rather than the backend changes? Perhaps related to #10335 (comment)
That's happening because col.mod was not being bumped, so it thinks there are no changes that need syncing. Fixed.
I've run the tests across all commits*. There was one broken one due a line being included in the following commit, but that's been fixed now. I can do this for the other PRs once they're rebased. But I'm not particularly bothered if you'd prefer to squash.
|
This worked in my initial testing, but with a smaller collection, the UI is now racing with the download on my machine: - getSyncStatus() was assuming col.db would still be open by the time it gets around to calling methods on it - onCreateOptionsMenu() likewise assumes that if colIsOpen() returns true, it is free to call methods on it A better way way to fix this would be to wrap all collection access in a mutex, so that the collection is never touched without the lock being acquired. Code holding the mutex could then be guaranteed that the collection is open and valid for the duration of its run. Changing all that legacy code is a big task however. I've added a lock to the upload case as well, as it's possible it's likewise buggy and just not triggering on my machine.
|
(re-pushed to trigger CI due to flake) |
|
on v16 after full sync, review, undo, sync, the sync badge is still active 🤔 ? also it appeared on full sync that the media check / sync did not run - or at least it was different then when I did a sync after the full sync - on the sync it did an extra dialog with media check stuff |
|
? |
I want to temporarily sunset the sync badge. It's based off DB access which won't work with the new backend and I don't deem it to be higher priority than getting this in. I'd rather we removed the display logic and added a cleanup to recode it in a manner which works with the new backend |
|
Okay - RustCleanup on the sync badge then? I really really like that functionality but I can see how the implementation is currently problematic. Perhaps it should decorate access to the undo stack in the new backend or something, but agreed this should go in if at all possible - it's a big train of work here and I want to see it keep on moving |
|
Actually, I'll peel an issue from one of the comments here and keep moving forward --> #11902 |
There was a problem hiding this comment.
Okay with V16 sync badge peeled to a new issue this is good to go for me. @david-allison you've commented, I'm not sure if you want to review or not but I want to give you the opportunity in case you want to take advantage of it? I would merge otherwise and I'll set a sort of soft timer on here of "merge in 24 hours or so if no response at all" but if you state an intention (any intention) I'll wait or whatever. Excited to get it in!
mikehardy
added
Pending Merge
|
I won't have any time until next Saturday/Sunday. @mikehardy full trust in your judgment. Not looking at the code. 'Go for it' |
|
Ok, going for it then to break the logjam, follow ups as needed. Vamos! |
github-actions
bot
removed
Pending Merge
|
Hi there @dae! This is the OpenCollective Notice for PRs merged from 2022-07-01 through 2022-07-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 note that GSoC contributions are okay for this process. Our philosophy is that our users have donated to AnkiDroid for all contributions. The only PRs that will not go through the OpenCollective process are ones directly related to am accepted GSoC project from a selected participant, since those receive a stipend from GSoC itself. 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! |
When playing with this, recommend you use a local build of ankidroid/Anki-Android-Backend#218 if you want to test cancelling a progress dialog, or else attempting to cancel a task will cause the GUI to hang until it completes. It doesn't require a backend update to pass the unit tests/function correctly with defaultLegacySchema though, so it's not blocked on a backend update.
The latest commit has caused a few more tests to fail; this time it should not even be triggering an exception, so it seems like the issue is just that the coroutines are not running correctly under Robolectric.Any coroutine gurus have any ideas? To try out this code, addlegacy_schema=falseto local.properties then run the unit tests with this PR. Don't use it with your collection without a backup. @lukstbit @divyansh-dxn @ShridharGoelEdit: appears to have been solved with ShadowLooper.runUiThreadTasksIncludingDelayedTasks()Edit 2: see #11740 (comment)
Closes #11736
Closes #11733
Closes #11735
Closes #11732
Closes #11703