Collect: instead of using deprecated method startManagingCursor (which is UI blocking) use Loaders instead( Asynchronous, Event driven and Non UI Blocking)

[pranavgupta1234]

Software and hardware versions

Android 5.0.2 API Level 21

Problem description

Usage of deprecated method startManagingCursor

Steps to reproduce the problem

Open MainMenuActivity

Expected behavior

Data should be loaded asynchronously

Other information

Use Loaders :
The new Loader API is a huge step forward, and significantly improves the user experience. Loaders ensure that all cursor operations are done asynchronously, thus eliminating the possibility of blocking the UI thread. Further, when managed by the LoaderManager, Loaders retain their existing cursor data across the activity instance (for example, when it is restarted due to a configuration change), thus saving the cursor from unnecessary, potentially expensive re-queries. As an added bonus, Loaders are intelligent enough to monitor the underlying data source for updates, re-querying automatically when the data is changed.

[pranavgupta1234]

For more reference : http://www.androiddesignpatterns.com/2012/07/loaders-and-loadermanager-background.html
@yanokwa any views on this idea ?

[shobhitagarwal1612]

I did some debugging and found out that the cursors opened are not getting closed, even after using try-finally blocks for closing them. It gives the error mentioned in #757. The line for closing the cursor executes successfully and on checking the status of the cursor after the execution of cursor.close() it shows that the cursor is closed. We can't use the try-with-resources for autoclosing as it needs a minSdk API 19 (current minSdk is 16).
So, using CursorLoader and LoaderManager we can fix both #757 and improve the performance.
We can retain the current DAO design structure (FormsDao and InstancesDao) for getting various kinds of CursorLoader.
@lognaturel @yanokwa I am interested in handling this issue.

[pranavgupta1234]

Sorry I mistakenly closed the issue, I am also interested to work on it.

[lognaturel]

@pranavgupta1234 @shobhitagarwal1612 In principle this certainly sounds like a good idea and something we should do but these are APIs I'm not familiar with so I'd like to get your help understanding the implications a little bit better so we can decide when to do it. It's good to know that the current DAO layer can be maintained, I think that's an important abstraction. I presume all of the List Activities/Fragments will need to change somewhat as well. The more we can isolate the DB layer, the better (e.g. in the DAO classes). Are there any other classes that this change would touch?

It seems like this is a change that could introduce subtle lag, maybe some weirdness when the server data changes, things like that. How would you verify it thoroughly? I would suggest starting by writing up a list of cases that need to be considered (loading >1000 forms or instances from XML, deleting >1 forms or instances at once, etc). Even if we end up doing manual testing, having that list will ensure we know what to think about when evaluating the change.

[shobhitagarwal1612]

Sounds like a good plan. Also, we should start by converting the cursors for a single class and comparing the changes with the older build.
This will only change the part the code which interacts with the database. So other things won't be affected by this.

[shobhitagarwal1612]

Shall I proceed with this?

[lognaturel]

Let's start by answering some of the questions I had. Since @pranavgupta1234 filed this issue and was also interested in addressing it, he should be the one to take it on!

[lognaturel]

Of course, you're welcome to work together as well, I'll let you two figure out the details!

[shobhitagarwal1612]

What are the things that should be noted while making the list? Only the case or the behaviour too?

[lognaturel]

I think the cases are sufficient. Usually the expected behavior is implied but if not you can add an extra sentence about it.

[shobhitagarwal1612]

ok :white_check_mark:

[pranavgupta1234]

Sure we will work on it together @shobhitagarwal1612

[shobhitagarwal1612]

Since, you were the one to raise this issue. How do you propose we should start tackling this issue?

[shobhitagarwal1612]

@pranavgupta1234 Any thoughts?

[pranavgupta1234]

Give me little bit more time to figure out how to implement this with minimal affect on the present code.I will try to discuss asap.

[shobhitagarwal1612]

Do discuss if you come across any problem

[shobhitagarwal1612]

@pranavgupta1234 It has been quite some time now. Have you found any approach or should I start working on it?

[lognaturel]

@pranavgupta1234 It's been a couple of weeks now so let's give @shobhitagarwal1612 the lead on this one now. Of course if you have any ideas, they're very welcome and I'm sure @shobhitagarwal1612 will appreciate your review if you have time!

[pranavgupta1234]

Sure @lognaturel , I will catch up with you @shobhitagarwal1612

[seadowg]

We're looking at moving async work towards a new Scheduler abstraction in #3886

[lognaturel]

Also, all the Loader machinery is now deprecated in favor of ViewModel/LiveData/Room.

Agreed that we should move towards a unified model for async work. Related to this conversation, we've also been hoping to work towards higher level abstractions for DB-stored data than what the existing DAO provides. There's some general messiness in the DB layer described in the class comment on DatabaseInstancesRepository. At some point we should discuss what it would take to start using Room instead of rolling our own abstractions.