70

I have a FragmentActivity class with inner class that should display Dialog. But I am required to make it static. Eclipse offers me to suppress error with @SuppressLint("ValidFragment"). Is it bad style if I do it and what are the possible consequences?

public class CarActivity extends FragmentActivity {
//Code
  @SuppressLint("ValidFragment")
  public class NetworkConnectionError extends DialogFragment {
    private String message;
    private AsyncTask task;
    private String taskMessage;
    @Override
    public void setArguments(Bundle args) {
      super.setArguments(args);
      message = args.getString("message");
    }
    public void setTask(CarActivity.CarInfo task, String msg) {
      this.task = task;
      this.taskMessage = msg;
    }
    @Override
    public Dialog onCreateDialog(Bundle savedInstanceState) {
      // Use the Builder class for convenient dialog construction
      AlertDialog.Builder builder = new AlertDialog.Builder(getActivity());
      builder.setMessage(message).setPositiveButton("Go back", new DialogInterface.OnClickListener() {
        @Override
        public void onClick(DialogInterface dialog, int id) {
          Intent i = new Intent(getActivity().getBaseContext(), MainScreen.class);
          startActivity(i);
        }
      });
      builder.setNegativeButton("Retry", new DialogInterface.OnClickListener() {
        @Override
        public void onClick(DialogInterface dialog, int id) {
          startDownload();
        }
      });
      // Create the AlertDialog object and return it
      return builder.create();
    }
  }

startDownload() starts Asynctask.

dda
  • 6,030
  • 2
  • 25
  • 34
user2176737
  • 789
  • 1
  • 5
  • 10
  • 2
    In general its bad practice to ignore lint. It's a pretty smart tool. Try posting your code, to actually get an answer on how you could do a better job. – Stefan de Bruijn Mar 22 '13 at 13:05
  • have You checked this http://code.google.com/p/android/issues/detail?id=41800 in order to know that is ValidFragment about? The lint says that: 'Every fragment must have an empty constructor, so it can be instantiated' – sandrstar Mar 22 '13 at 13:14
  • I did. But I don't see why I must not omit this warning. What can be possible consequences? – user2176737 Mar 22 '13 at 13:23

5 Answers5

94

Non static inner classes do hold a reference to their parent classes. The problem with making a Fragment inner class non-static is that you always hold a reference to the Activity. The GarbageCollector cannot collect your Activity. So you can 'leak' the Activity if for example the orientation changes. Because the Fragment might still live and gets inserted in a new Activity.

EDIT:

Since some people asked me for some example I started writing one, while doing this I found some more problems when using non static Fragments:

  • They cannot be used in a xml file since they do not have a empty constructor (They can have an empty constructor, but you usually instantiate nonstatic nested classes by doing myActivityInstance.new Fragment() and this is different to only calling an empty constructor)
  • They cannot be reused at all - since the FragmentManager sometimes calls this empty constructor too. If you added the Fragment in some Transaction.

So in order to make my example work I had to add the

wrongFragment.setRetainInstance(true);

Line to not make the app crash on orientation change.

If you execute this code you will have an activity with some textviews and 2 buttons - the buttons increase some counter. And the Fragments show the orientation which they think their activity has. At the start everything works correctly. But after changing the screen orientation only the first Fragment works correcly - the second one is still calling stuff at its old activity.

My Activity class:

package com.example.fragmenttest;

import android.annotation.SuppressLint;
import android.app.Activity;
import android.app.Fragment;
import android.app.FragmentTransaction;
import android.content.res.Configuration;
import android.os.Bundle;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.Button;
import android.widget.LinearLayout;
import android.widget.TextView;

public class WrongFragmentUsageActivity extends Activity
{
private String mActivityOrientation="";
private int mButtonClicks=0;
private TextView mClickTextView;


private static final String WRONG_FRAGMENT_TAG = "WrongFragment" ;

@Override
protected void onCreate(Bundle savedInstanceState)
{
    super.onCreate(savedInstanceState);
    int orientation = getResources().getConfiguration().orientation;
    if (orientation == Configuration.ORIENTATION_LANDSCAPE)
    {
        mActivityOrientation = "Landscape";
    }
    else if (orientation == Configuration.ORIENTATION_PORTRAIT)
    {
        mActivityOrientation = "Portrait";
    }

    setContentView(R.layout.activity_wrong_fragement_usage);
    mClickTextView = (TextView) findViewById(R.id.clicksText);
    updateClickTextView();
    TextView orientationtextView = (TextView) findViewById(R.id.orientationText);
    orientationtextView.setText("Activity orientation is: " + mActivityOrientation);

    Fragment wrongFragment = (WrongFragment) getFragmentManager().findFragmentByTag(WRONG_FRAGMENT_TAG);
    if (wrongFragment == null)
    {
        wrongFragment = new WrongFragment();
        FragmentTransaction ft = getFragmentManager().beginTransaction();
        ft.add(R.id.mainView, wrongFragment, WRONG_FRAGMENT_TAG);
        ft.commit();
        wrongFragment.setRetainInstance(true); // <-- this is important - otherwise the fragment manager will crash when readding the fragment
    }
}

private void updateClickTextView()
{
    mClickTextView.setText("The buttons have been pressed " + mButtonClicks + " times");
}

private String getActivityOrientationString()
{
    return mActivityOrientation;
}


@SuppressLint("ValidFragment")
public class WrongFragment extends Fragment
{


    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState)
    {
        LinearLayout result = new LinearLayout(WrongFragmentUsageActivity.this);
        result.setOrientation(LinearLayout.VERTICAL);
        Button b = new Button(WrongFragmentUsageActivity.this);
        b.setText("WrongFragmentButton");
        result.addView(b);
        b.setOnClickListener(new View.OnClickListener()
        {
            @Override
            public void onClick(View v)
            {
                buttonPressed();
            }
        });
        TextView orientationText = new TextView(WrongFragmentUsageActivity.this);
        orientationText.setText("WrongFragment Activities Orientation: " + getActivityOrientationString());
        result.addView(orientationText);
        return result;
    }
}

public static class CorrectFragment extends Fragment
{
    private WrongFragmentUsageActivity mActivity;


    @Override
    public void onAttach(Activity activity)
    {
        if (activity instanceof WrongFragmentUsageActivity)
        {
            mActivity = (WrongFragmentUsageActivity) activity;
        }
        super.onAttach(activity);
    }

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState)
    {
        LinearLayout result = new LinearLayout(mActivity);
        result.setOrientation(LinearLayout.VERTICAL);
        Button b = new Button(mActivity);
        b.setText("CorrectFragmentButton");
        result.addView(b);
        b.setOnClickListener(new View.OnClickListener()
        {
            @Override
            public void onClick(View v)
            {
                mActivity.buttonPressed();
            }
        });
        TextView orientationText = new TextView(mActivity);
        orientationText.setText("CorrectFragment Activities Orientation: " + mActivity.getActivityOrientationString());
        result.addView(orientationText);
        return result;
    }
}

public void buttonPressed()
{
    mButtonClicks++;
    updateClickTextView();
}

}

Note that you should probably not cast the activity in onAttach if you want to use your Fragment in different Activities - but for here its working for the example.

The activity_wrong_fragement_usage.xml:

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical"
tools:context=".WrongFragmentUsageActivity" 
android:id="@+id/mainView">

<TextView
    android:id="@+id/orientationText"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:text="" />

<TextView
    android:id="@+id/clicksText"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:text="" />



<fragment class="com.example.fragmenttest.WrongFragmentUsageActivity$CorrectFragment"
          android:id="@+id/correctfragment"
          android:layout_width="wrap_content"
          android:layout_height="wrap_content" />


</LinearLayout>
Gaston Flores
  • 2,457
  • 3
  • 23
  • 42
Simon Meyer
  • 1,946
  • 12
  • 23
  • very interesting and probably very useful answer. I am dealing with the same issue. Would you mind dropping some sort of source that your answer is based on? – Egis Apr 03 '13 at 08:52
  • 3
    @Egis perhaps this could give you some insight about nested-static inner classes: http://docs.oracle.com/javase/tutorial/java/javaOO/nested.html. – AxeEffect Jun 08 '13 at 10:35
  • 1
    Do you have a reference for this answer? – Mike T Aug 01 '13 at 08:49
  • I believe the WrongFragment instance is maintaining its reference to the old Activity instance (`WrongFragmentActivity.this` used in `onCreateView()`) due to the fact that `WrongFragment.onCreateView()` is not called again; the view is never re-created due to `setRetainInstance(true)` and "keeping" the fragment via `findFragmentByTag()`, as opposed the new instance of CorrectFragment via the XML layout file `fragment` element. Minor point and a byproduct of the Android architecture you're describing, but worth understanding. – MeanderingCode Mar 17 '15 at 22:14
  • `onAttach` is deprecated – trans Oct 23 '16 at 12:33
18

I won't talk about inner fragments, but more specifically about a DialogFragment defined within an activity because it's 99% of the case for this question.
From my point of view, I don't want my DialogFragment (your NetworkConnectionError) to be static because I want to be able to call variables or methods from my containing class (Activity) in it.
It won't be static, but I don't want to generate memoryLeaks either.
What is the solution?
Simple. When you go in onStop, ensure you kill your DialogFragment. It's as simple as that. The code looks like something like that:

public class CarActivity extends AppCompatActivity{

/**
 * The DialogFragment networkConnectionErrorDialog 
 */
private NetworkConnectionError  networkConnectionErrorDialog ;
//...  your code ...//
@Override
protected void onStop() {
    super.onStop();
    //invalidate the DialogFragment to avoid stupid memory leak
    if (networkConnectionErrorDialog != null) {
        if (networkConnectionErrorDialog .isVisible()) {
            networkConnectionErrorDialog .dismiss();
        }
        networkConnectionErrorDialog = null;
    }
}
/**
 * The method called to display your dialogFragment
 */
private void onDeleteCurrentCity(){
    FragmentManager fm = getSupportFragmentManager();
     networkConnectionErrorDialog =(DeleteAlert)fm.findFragmentByTag("networkError");
    if(networkConnectionErrorDialog ==null){
        networkConnectionErrorDialog =new DeleteAlert();
    }
    networkConnectionErrorDialog .show(getSupportFragmentManager(), "networkError");
}

And that way you avoid memory leaks (because it's bad) and you insure you don't have a [expletive] static fragment that cannot access your activity's fields and methods. This is the good way to handle that problem, from my point of view.

Andrew Orobator
  • 7,978
  • 3
  • 36
  • 36
  • looks nice, but will it get errors at time of generation of apk like @hakri Reddy mentioned below – Shirish Herwade Jan 13 '17 at 08:28
  • Yep, but it's not because lint is not smart enough that we need to be "as stupid as it", memort leak are gone using this technic (CanaryLeak will show you that)... By the way, I first run my apk with lint to detect others mistakes that I can have done in my code, fix the issue I think I need to and then run it it with the abortOnError false. And on some project, I customize Lint on this specific rule ("Inner class should be static" drop to weakwarning) – Mathias Seguy Android2ee Jan 25 '17 at 08:50
  • ohh... but in my case, actual apk generation is done by different team sitting in US office(I'm in India, and I provide only git code repository links) because they don't share company signing certificate files to anyone. So they definitely are not going to listen my reason and not going to change their settings :( – Shirish Herwade Jan 25 '17 at 09:41
  • Yep, sometimes there isn't only technical constraints :) and we have to accept them to :) – Mathias Seguy Android2ee Jan 26 '17 at 13:28
  • this approach gives me a `Fragment must be a public static class crash error` – leonardkraemer Mar 10 '18 at 16:53
5

If u develop it in android studio then no problem if you not give it as static.The project will run without any errors and at the time of generation of apk you will get Error :This fragment inner class should be static [ValidFragment]

Thats lint error, you are probably building with gradle, to disable aborting on errors, add:

lintOptions {
    abortOnError false
}

to build.gradle.`

chakri Reddy
  • 2,920
  • 1
  • 15
  • 12
  • 8
    you right, it will pass the building process, but is it right to use it that way? because there's a leaking memory issue and that's why android studio warn us. – XcodeNOOB Nov 10 '15 at 11:56
  • Sometimes I think abortOnError to false is just to hard, I prefer customize the lint's rule "Inner class should be static" to weakwarning or info. – Mathias Seguy Android2ee Jan 25 '17 at 08:55
5

If you want to access the members of outer-class (Activity) and still not want to make members static in Activity (since fragment should be public static), you can do the override onActivityCreated

public static class MyFragment extends ListFragment {

    private OuterActivityName activity; // outer Activity

    @Override
    public void onActivityCreated(Bundle savedInstanceState) {
        super.onActivityCreated(savedInstanceState);
        activity = (OuterActivityName) getActivity();
        ...
        activity.member // accessing the members of activity
        ...
     }
Deepu
  • 598
  • 6
  • 12
-2

add annotation before inner class

@SuppressLint("validFragment")

Sanket Parchande
  • 894
  • 9
  • 14