888039 - Use support library's SimpleCursorAdapter if swapCursor() is needed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

swapCursor() is only available on pre-honeycomb devices.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Thanks Android for you Loader, but don't force us to use newer methods!

Comment 2

[:Margaret Leibovic]

Comment on attachment 768607 [details] [diff] [review]
Patch

We also use swapCursor in BrowserSearch.java. Let's change that, too.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Using the support library.

Comment 4

[:Margaret Leibovic]

Just curious here, is there a reason we want to be using swapCursor instead of changeCursor? The docs say that changeCursor will close the cursor for us, but swapCursor won't. We're currently not doing anything with the cursor that swapCursor returns, so are we ending up with a bunch of unclosed cursors?

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #4)
> Just curious here, is there a reason we want to be using swapCursor instead
> of changeCursor? The docs say that changeCursor will close the cursor for
> us, but swapCursor won't. We're currently not doing anything with the cursor
> that swapCursor returns, so are we ending up with a bunch of unclosed
> cursors?

I believe lucasr added swapCursor() because android recommends it.

http://developer.android.com/reference/android/app/LoaderManager.html <-- search for swapCursor() in their example implementation. They've documented it.

Comment 6

[Sriram Ramasubramanian [:sriram]]

From their docs:

    public void onLoadFinished(Loader<Cursor> loader, Cursor data) {
        // Swap the new cursor in.  (The framework will take care of closing the
        // old cursor once we return.)
        mAdapter.swapCursor(data);
        ... more junk code ...
    }

Comment 7

[:Margaret Leibovic]

Okay, I trust Lucas knows what he's doing, but I'm going to needinfo him just to make sure. However, I'm fine with landing this patch right now since he's away for a few days.

Comment 8

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/2f186c2d4a41

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #7)
> Okay, I trust Lucas knows what he's doing, but I'm going to needinfo him
> just to make sure. However, I'm fine with landing this patch right now since
> he's away for a few days.

SimpleCursorLoader owns the cursor and closes it for us when needed. We only need to make sure we don't keep dangling references to closed cursors. Have a look at SimpleCursorLoader's implementation for reference.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: CursorAdapter

Lucas asked me to file a followup for bug 887982 and bug 887985. Reusing this bug as this tracks a similar issue, and the bug is trivial.

Comment 11

[:Margaret Leibovic]

Comment on attachment 769768 [details] [diff] [review]
Patch: CursorAdapter

I feel like we should start a wiki page about Cursor-related best practices. Maybe I'll ask ckitching to do that because he's also been fixing some Cursor issues recently.

Comment 12

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/cfcde93a090f

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f186c2d4a41
https://hg.mozilla.org/mozilla-central/rev/cfcde93a090f