Double checking if fragment + view holder pattern is implemented properly

Question

I've been re-working some code due to a memory leak. The code is part of the apps help section where we are using a FragmentActivity and FragmentPageAdapter to allow the user to swipe through the different help screens. Each fragment, SectionFragment class below, includes an image, some header text and body text.

The memory leak manifested itself because a new view was being inflated each time the onCreateView was called in the Fragment.

public class SectionFragment extends Fragment {

    private ImageView imgvw;
    private TextView headerTxvw;
    private TextView bodyTxvw;

    public int[][] content;        
    protected int pageIdx;

    public SectionFragment(int idx, int[][] content ){
        super();
        pageIdx = idx;
        this.content = content;
    }

    protected int getPageIdx() {
        return pageIdx;
    }

    protected Drawable getImageDrawable() {
        return getResources().getDrawable( content[pageIdx][0] );
    }

    protected String getHeaderText() {
        return getResources().getString( content[pageIdx][1] );
    }

    protected String getSubeaderText() {
        return getResources().getString( content[pageIdx][2] );
    }

    protected void loadView( View vw ) {
        imgvw = (ImageView)vw.findViewById( R.id.top_img );
        headerTxvw = (TextView)vw.findViewById( R.id.header_txt );
        bodyTxvw = (TextView)vw.findViewById( R.id.body_txt );

        imgvw.setImageDrawable( getImageDrawable() );
        headerTxvw.setText( getHeaderText() );
        bodyTxvw.setText( getSubeaderText() );
    }

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container,
        Bundle savedInstanceState) {
        View vw = inflater.inflate( R.layout.help_fragment, null );
        loadView( vw );
        return vw;
    }
}

I'm still new to Fragments and wasn't part of the original coding effort so I'm not sure that my solution is correct, but it resembles the view holder pattern I've implemented many times in List Activities. I'd really appreciate any feedback on the following implementation so I'd be more comfortable that this is correct.

public class SectionFragment extends Fragment {

static private class ViewHolder {
    ImageView imgvw;
    TextView headerTxvw;
    TextView bodyTxvw;
}

public int[][] _content;
protected int _pageIdx;

public SectionFragment( int idx, int[][] content ){
    super();
    _pageIdx = idx;
    _content = content;
}

protected int getPageIdx() {
    return _pageIdx;
}

protected Drawable getImageDrawable() {
    return getResources().getDrawable( _content[_pageIdx][0] );
}

protected String getHeaderText() {
    return getResources().getString( _content[_pageIdx][1] );
}

protected String getSubeaderText() {
    return getResources().getString( _content[_pageIdx][2] );
}

protected void loadView( View vw ) {

    _holder.imgvw.setImageDrawable( getImageDrawable() );
    _holder.headerTxvw.setText( getHeaderText() );
    _holder.bodyTxvw.setText( getSubeaderText() );
}

private View _vw;
private ViewHolder _holder;

@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container,
        Bundle savedInstanceState) {
    if ( _vw == null ) {
        _vw = inflater.inflate( R.layout.help_fragment, null );

        _holder = new ViewHolder();
        _holder.imgvw = (ImageView)_vw.findViewById( R.id.top_img );
        _holder.headerTxvw = (TextView)_vw.findViewById( R.id.header_txt );
        _holder.bodyTxvw = (TextView)_vw.findViewById( R.id.body_txt );
        _vw.setTag( _holder );
    } else {
        ViewParent oldparent = (ViewParent)_vw.getParent();
        if ( oldparent != container ) {
            ((ViewGroup)oldparent).removeView( _vw );
        }
        _holder = (ViewHolder)_vw.getTag();
    }
    loadView( _vw );
    return _vw;
}
}

I haven't included the other classes associated with this code, specifically the FragmentActivity and FragmentPagerAdapter because they seem to be implemented properly, but if requested I can also include them.

Answer 1

On memory leaks in Android apps in general

Are you certain that it was leaking memory and not just delaying the garbage collection? Have you run for example eclipse memory analyzer plugin on the application? It can often show exactly where the leaks occur.

On fragment viewpager and its recycling (or lack thereof)

My first guess for memory leaks/increases in a fragment pager is having a dynamic adapter. The fragmentpageradapter is pretty dumb and will not handle changes well.

As far as I can see from the source it will never destroy fragments even if you for example change count from 5 to 4 (the fifth fragment will stay in the fragment manager). This is due to it being detached rather than destroyed. The fragmentstatepageradapter will however destroy views when they are outside of the page margin.

And all of this does not actually lead to memory leaks in the long run since those fragments will be cleaned up when the fragmentmanager is cleared of fragments later. It can however increase the memory use quite a bit (potentially leading to an outofmemoryerror).

A new view is indeed created each time the fragment is created (and more often than that if you set retaininstance to true), but unless you use the FragmentStatePagerAdapter that should only occur once per fragment since the normal pager adapter detaches and attaches the same fragments from the fragment manager. And in any case the view will be disposed after the activity is destroyed unless you keep references to it.

You do seem to keep references to views in your class, which is something I normally try to avoid. I prefer using getView().findViewById() when I need them, but in this case I think the member references should be found and removed by the gc. This all depends on if you leak those references elsewhere of course.

On viewholder in fragment pager

You should not try to do something like the viewholder in the fragment. It will probably not hurt in this example, but I can't see you gaining anything from it. Just inflate the view in oncreateview and set the values and be done with it.

There is no recycling of views such as in the listview, so the ViewHolder pattern makes no sense and only risks introducing memory leaks or delayed garbage collections due to dangling references.

Views are not made to be reused, even though they can be during some parent trickery as you have used. This should only be done as a final way out. If anything is off in the flow it can however lead to horrible leaks.

Please correct me if anyone can see a reasonable use for it.

A side note on ViewHolder

I would actually refrain from using viewholder unless I really needed it even in a listview. I have seen those tags lead to really nasty memory leaks in for example some cursor adapters and I would look at making my row layouts simpler before trying reference saving. Findviewbyid will not be that expensive in a good layout.