Forward-port to Loader changes in support library 27.1.1

[mikehardy]

Pull Request template

Please, go through these checks before you submit a PR.

• You have not changed whitespace unnecessarily (it makes diffs hard to read)
• You have a descriptive commit message with a short title (first line, max 50 chars).
• Your code follows the style of the project (e.g. never omit braces in if statements)
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code

Purpose / Description

Support library 27.1.x changed the LoaderCallbacks behavior so that onLoadFinished() was called more often then we assumed it would be, and that blew up our View state

Fixes

Fixes #4938 and is related to #4937 and #4939

Approach

The general idea is to only notify the view layer about collection loads if really necessary, but that has to be managed across thread contexts

How Has This Been Tested?

This affects AnkiActivity, so I tested everything that directly or indirectly subclasses it, and all functionality seemed to work across pause/resume on emulators API15 through API28. I think it works, but threading issues are always fraught.

Learning (optional, can help others)

The author of the loader changes describing it:
https://medium.com/androiddevelopers/loaders-in-support-library-27-1-0-b1a1f0fee638

Apparently we should really move to "ViewModels" and "LiveData" but I don't even know what that is, so...

A nice MVP framework that used Loaders where this 27.0 -> 27.1 upgrade was discussed, and it was determined their solution already worked, so I adapted it to AnkiDroid:
https://github.com/benoitletondor/Android-Studio-MVP-template/blob/master/MVPBoilerplate/root/src/app_package/classes/BaseActivity.java.ftl

[mikehardy]

Posting my own criticism so I don't forget before I go to bed. Is there anything else in the code base using Loader? Shouldn't we just not load instead of protecting from results notification? This solution does work but may not be complete or optimal yet

[mikehardy]

@timrae I think this is ready. No one else was using Loaders this way so no one else needs the forward-port treatment. There is the possibility of a refactor because Fragment transactions may be committed in response to onLoadFinished() now, but most of the DialogHandler / message-passing thing is happening because of AsyncTasks not Loaders, so I don't think we can refactor that out. That leaves the idea that maybe we should gate things inbound during startLoadingCollection vs onLoadFinished, but I don't see much difference between them (they both should be rare doubled-events) and I've already tested this version plus have an example to base it on - so I'd like it to go as is unless you see something I missed

[timrae]

Ok, I need to think about a bit from a design perspective

[mikehardy]

This is non-blocking now. It's a very small change, but definitely think-worthy

[timrae]

I think I'd probably prefer to have a go at using these new classes in the lifecycle support library for 2.9 since it seems:

• Loaders are now deprecated in Android P
• The changes in the support library loader implementation are quite big and new, and probably won't be maintained well given the above
• The implementation here seems quite convoluted to me, yet I don't see a much better way to do it

The idea was never for the Loader to be re-triggered on Activity::onResume(); it's a bit annoying that they changed that behavior (and seemingly without actually mentioning it lol), but it would appear that was in fact the behavior intended by the Loader developers from the beginning, since when I looked at the current master branch with the debugger, the loader does in fact get triggered by onResume(), but then the notification back to the Activity gets mysteriously dropped somewhere by the LoaderManger (presumably an irredeemable bug) lol.

[mikehardy]

I agree those components are the right path. I want to do that and the shift to androidx also. I have two hangups - one, that stuff is relatively new vs Loader - the current version for the lifecycle stuff in androidx is "2.0.0-beta01" (most of the other androidx is beta or rc as well). And I'm loathe to do a threading re-think and delay 2.9 more vs finishing the features that needs new localization so we can freeze. In my experience threading changes always result in at least one issue (like this one!) that takes a few full development days to fix (like this one) and then careful review. Can we wait for 2.10 for re-architecture in this area and just drop the unexpected-but-should-have-been-there-forever onResultsLaoded call in our code as patched here for now?

[mikehardy]

If so I'd log an issue to track of course, maybe even do a 2.10 milestone to start collecting even (as we shift things I'm tagging 2.9 now after sizing them if they don't seem to fit for a "do 2.9 soon" idea)

[timrae]

Well if I haven't done it before the beta then yeah we can re-discuss, but it doesn't seem that difficult

[mikehardy]

it doesn't seem that difficult - says the person that wrote the original ;-). I just learned the lifecycle plus I'm new to this code so I don't want to commit myself. I'll leave this branch open in case we need it. I don't think moving from library 27.0.2 to 27.1.1 is blocking anything for 2.9, it's just general dependency hygiene so this isn't a must-merge-now thing

[timrae]

Superseded by #4972