[GSoC] Migrated CollectionTask.AddNote to Coroutines

[thedroiddiv]

Purpose / Description

Migrated CollectionTask.AddNote to Coroutines

Fixes

Fixes a part of #7108

Approach

Extracted the codes from AsyncTask callbacks, into a proper sequence of suspend function calls.

How Has This Been Tested?

Passes the existing unit tests, runs as expected on realms 6i API 30

Checklist

• [x] You have not changed whitespace unnecessarily (it makes diffs hard to read)
• [x] You have a descriptive commit message with a short title (first line, max 50 chars).
• [x] Your code follows the style of the project (e.g. never omit braces in if statements)
• [x] You have commented your code, particularly in hard-to-understand areas
• [x] 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

[thedroiddiv]

And one thing is on the todo is to improve the documentation, haven't done it for now just removed the irrelevant parts.

[dae]

Disclaimer: I am not an AnkiDroid maintainer, and have not spent much time working with AnkiDroid's code, so I can only offer comments as an external observer. Hopefully a maintainer will offer their own thoughts when they have a chance.

I can see you've put a lot of effort into integrating coroutines into the existing CollectionTask API, and what you've done is technically impressive. That said, I am not sure this is the best path forward.

As an occasional contributor, my view of the CollectionTask API is not great - it makes it hard to follow the flow of the code, as it relies on a bunch of callbacks. IMHO, the main appeal of coroutines in that they allow asynchronous code to be written in a more straight-forward way without the use of callbacks. By keeping the existing CollectionTask API, I feel like this main benefit of coroutines is being lost.

Instead of keeping to the existing API, maybe there would be more value in rewriting the code to use coroutines natively? This will be more work in the short term, but should lead to easier to read code going forward.

Because I found CollectionTask hard to understand, I avoided it in the V16 code paths. For example, the sync code launches a coroutine instead of a collection task: https://github.com/ankidroid/Anki-Android/blob/bbeaf6dc926b9d5365675faa3e65269c1a59baef/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt#L1505 https://github.com/ankidroid/Anki-Android/blob/bbeaf6dc926b9d5365675faa3e65269c1a59baef/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt#L44

Perhaps a similar approach could be used for existing CollectionTasks?

There's also the question of what else CollectionTask is providing. For serialization of requests, that should hopefully be addressed in https://github.com/ankidroid/Anki-Android/pull/11849. Are there other things that CollectionTask provides that would be missed?

[ShridharGoel]

@divyansh-dxn Is there a reason for not using CoroutineExceptionHandler?

[ShridharGoel]

@dae has provided some good advice, it would be better to rewrite this flow instead of trying to migrate to coroutines using the existing code.

[thedroiddiv]

@dae @ShridharGoel I would suggest to take a look on the recent push.

[thedroiddiv]

Disclaimer: I am not an AnkiDroid maintainer, and have not spent much time working with AnkiDroid's code, so I can only offer comments as an external observer. Hopefully a maintainer will offer their own thoughts when they have a chance.

I can see you've put a lot of effort into integrating coroutines into the existing CollectionTask API, and what you've done is technically impressive. That said, I am not sure this is the best path forward.

As an occasional contributor, my view of the CollectionTask API is not great - it makes it hard to follow the flow of the code, as it relies on a bunch of callbacks. IMHO, the main appeal of coroutines in that they allow asynchronous code to be written in a more straight-forward way without the use of callbacks. By keeping the existing CollectionTask API, I feel like this main benefit of coroutines is being lost.

Instead of keeping to the existing API, maybe there would be more value in rewriting the code to use coroutines natively? This will be more work in the short term, but should lead to easier to read code going forward.

Because I found CollectionTask hard to understand, I avoided it in the V16 code paths. For example, the sync code launches a coroutine instead of a collection task:

https://github.com/ankidroid/Anki-Android/blob/bbeaf6dc926b9d5365675faa3e65269c1a59baef/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt#L1505

https://github.com/ankidroid/Anki-Android/blob/bbeaf6dc926b9d5365675faa3e65269c1a59baef/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt#L44

Perhaps a similar approach could be used for existing CollectionTasks?

There's also the question of what else CollectionTask is providing. For serialization of requests, that should hopefully be addressed in #11849. Are there other things that CollectionTask provides that would be missed?

I was also thinking to re-implement it, but was afraid that other maintainers wouldn't allow to change from existing CollectionTask api. And yes, it is very hard to follow the current implementation. Took a lot of time for me also to just get to proper control-flow.

[Arthur-Milchior]

@divyansh-dxn Is this something that must be squashed or should we kept all commits?

To ask the same question in a different way. If I review the first commit alone, will it makes sens, or should I directly review the whole PR?

[thedroiddiv]

@divyansh-dxn Is this something that must be squashed or should we kept all commits?

To ask the same question in a different way. If I review the first commit alone, will it makes sens, or should I directly review the whole PR?

Changes are to be squashed. Reviewing whole PR is better. Because first commit consisted of codes that I tried to replicated, and now no longer required.

[thedroiddiv]

@Arthur-Milchior Now I think individual commit make more sense and it would be easy to follow up from previous to next commits.

[thedroiddiv]

@ShridharGoel @Arthur-Milchior requires review.

[Arthur-Milchior]

FYI you have a merrge conflict

[thedroiddiv]

Created a new PR #12059 for the first commit. Going through the discussion I finally agree, as @Arthur-Milchior earlier said, it would be clean, merge fastly and make this PR to review easier.

[thedroiddiv]

#12070 Combines the logic of onProgressUpdate and onPostExecute in SaveNoteHandler. Merging #12070 would also simplify changes on this one.

[thedroiddiv]

For the first commit, function copy-pasted and moved elsewhere. While doing at the same place, lines were getting jumbled in git diff and that would have been harder to review as compared to copy-paste elsewhere.

[thedroiddiv]

Before and after comparison for handled error. Throw an Exception manually to test this behaviour.

https://user-images.githubusercontent.com/69595691/185702339-6156eaaa-afdc-459e-b97b-181445cbd881.mp4

https://user-images.githubusercontent.com/69595691/185702329-80a27aa2-168f-47e4-b332-4d66f9fe9d4f.mp4

[thedroiddiv]

@ShridharGoel I believe these are kotlin cleanup, and deserve independent PRs. That would unnecessarily populate this PR. I believe ?

[ShridharGoel]

@divyansh-dxn Okay, you can add them in the cleanup then.

Message ID: @.***>

[dae]

No more objections on my end - this is a nice improvement over what there was before. :-)

[github-actions[bot]]

Hi there @divyansh-dxn! This is the OpenCollective Notice for PRs merged from 2022-08-01 through 2022-08-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!