migration of countModules() asyncTask to coroutines

[sauravrao637]

Purpose / Description

part of fix for #7108 This PR is part of migration of asyncTask to coroutines

Fixes

countModules() async task successfully migrated to coroutines

Approach

currently I have used

    launchWhenCreated{} 

in loadModules() according to my learnings so far and maybe it needs to be changed to other (please let me know the same as I am still learning )

How Has This Been Tested?

Have been tested manually on RedmiNote 5 Pro (with LineageOS android version 10) and also tested with all the written tests.

reason for avoiding toList() for items in DisplayPairAdapter resource 1 resource 2

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

[welcome[bot]]

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

[lukstbit]

Whenever we migrate to coroutines we should make sure we are able to inject/replace the dispatchers used to make testing easier.

[sauravrao637]

Whenever we migrate to coroutines we should make sure we are able to inject/replace the dispatchers used to make testing easier.

@lukstbit that's a good point, so should the dispatcher be passed as parameter to countModules() or there is another better way?

[lukstbit]

@sauravrao637 Yes, the minimum you can do is passing the dispatcher to the countModels() method:

suspend fun countModels(col: Collection, dispatcher: CoroutineDispatcher = Dispatcher.IO)

and then using the new parameter. The main dispatcher can be reset manually in tests.

[sauravrao637]

@lukstbit Hey, injecting the dispatcher make checks fail, any idea what can be the reason and how can it be fixed?

[lukstbit]

@sauravrao637 The instrumented tests are known to be flaky and I've seen the same failure for unit tests on windows in my own PRs so I don't think they are related to that small change.

[mikehardy]

I just restarted the emulator test, that is a known flake tracked here #10601

The windows test has two failures, one has already been disabled on windows as it is a known flake and if you rebased to current tip of main branch it would go away

The other windows failure is odd, never seen it. Might be this PR?

com.ichi2.anki.servicemodel.UpgradeVolumeButtonsToBindingsTest > gesture_set_no_conflicts[0: isValid(TestData(affectedPreferenceKey=gestureVolumeUp, keyCode=24, unaffectedPreferenceKey=gestureVolumeDown, binding=com.ichi2.anki.reviewer.MappableBinding@1900db))={1}] FAILED
    java.lang.AssertionError: 
    Expected: iterable with items ["preferenceUpgradeVersion", "gestureVolumeUp", "binding_SHOW_ANSWER"] in any order
         but: no item matches: "preferenceUpgradeVersion", "gestureVolumeUp", "binding_SHOW_ANSWER" in []
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at com.ichi2.anki.servicemodel.UpgradeVolumeButtonsToBindingsTest.gesture_set_no_conflicts(UpgradeVolumeButtonsToBindingsTest.kt:97)

The PR has also taken a conflict unfortunately - there are a LOT of PRs going in right and unfortunately conflicts happen under those conditions. Hopefully easy to resolve while rebasing to current tip of main branch to get the windows unit test failure fix

[sauravrao637]

@mikehardy rebase done, no test fails now

[sauravrao637]

Hi all, should I keep this PR open or I may close it?

[mikehardy]

I don't see a reason to close it unless you have no desire to update it to remove the conflicts

It's sitting in a needs review state just because current maintainers are really busy right now - I think the change is good, moving to coroutines is (I think) a priority, just that there is a list of priorities that starts with scoped storage, continues with GSoC mentoring, and then goes to "fix ankidroid crash reporting database" and "integrate critical Anki-Android-Backend changes" at least for me, so I haven't had a chance to come back to PR review though I recognize and regret that's de-motivating for PR authors. I can only ask for patience

if you're not interested though - that's fine and I understand, you may close if you like

[sauravrao637]

@mikehardy Thanks for replying. I asked because each time I resolve conflicts, new conflicts are already there before the review. Instead maintainers can let me know once they have enough time to review so as to avoid resolving conflicts and reviewing multiple times. PS. I totally understand everyone being busy and it's fine :)

[mikehardy]

Understood - yes it's frustrating. Unsure on time available at the moment, I think @Arthur-Milchior reviewed last and mentioned he was catching up recently, so he's probably best to comment on review availability?

[github-actions[bot]]

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically