Android - does defining a button static leak my activity? Can I avoid it?

Question

I need the button to be static so I can enable it/ disable it form my services in case the activity is shown. Still I setOnClickListener and anyway static views are considered dangerous. Do I leak ? Can I avoid it ?

public class MonitorActivity extends FragmentActivity implements
        OnClickListener {

    private static Button updateButton; // static??

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_monitor);
        // button
        updateButton = (Button) findViewById(R.id.update_data_button);
        updateButton.setOnClickListener(this); // oops ?
    }

    public static void onDataUpdated(Context ctx) {
        if (updateButton != null) { //that's why I need it static
            updateButton.setEnabled(true); // + set the text etc
        }
    }

    public static void onUpdating() {
        if (updateButton != null) {
            updateButton.setEnabled(false);
        }
    }

    @Override
    public void onClick(View v) {
        switch (v.getId()) {
        case R.id.update_data_button:
            serviceIntent.putExtra(MANUAL_UPDATE_INTENT_KEY, true);
            this.startService(serviceIntent);
        }
    }

    @Override
    protected void onResume() {
        super.onResume();
        Boolean isUpdating = AccessPreferences.get(this, updateInProgressKey,
            false);
        // set the button right
        updateButton.setText((isUpdating) ? defaultUpdatingText
                : getResources().getString(R.string.update_button_text));
        updateButton.setEnabled(!isUpdating);
    }
}

Yes, it's dangerous. The activity is destroyed but you still have a static reference to the button, which in turn has a reference to the activity context so you leak the entire activity (easy to see in DDMS). Instead, create an interface with a callback from your service to the activity or a broadcastreceiver. — Simon, Nov 24 '13 at 12:26
@Simon: broadcasts should be sticky - which a nono (IPC etc). How would I callback without a static member (the onUpdating etc are callbacks) ? — Mr_and_Mrs_D, Nov 24 '13 at 12:29
@Simon: can you post an example ? How would an interface save me from the need of having the button static ? — Mr_and_Mrs_D, Nov 24 '13 at 12:38
http://stackoverflow.com/questions/3398363/how-to-define-callbacks-in-android — Simon, Nov 24 '13 at 12:44
What's your doubt? You register the listener in onStart(), deregister in onPause(). In the service, use for(listener : listeners){listener.callbacK(arguments);}. Why is it more complicated than this? — Simon, Nov 24 '13 at 12:55
@Simon: `onStart()` ? Youmean `onResume()` ? And what if the activity is, say, paused and the service fires ? Callback lost, no ? What if the activity is not even started, service starts, callback lost, activity starts ,UI messed up. Am I missing something ? — Mr_and_Mrs_D, Nov 24 '13 at 13:00
I would use onStart() since you may receive a callback as soon as the listener is registered. onResume() might be too early as the UI is not guaranteed to be built at that point. onStart() is guaranteed anyway regardless of whether you are launching a new instance of resuming an existing one. As to the rest, I don't know your use case. Remember that an activity which is not in the foreground might not even exist. What exactly are you trying to do? Sounds like a flag in preferences, toggled by a callback in the Application singleton might be a better approach? — Simon, Nov 24 '13 at 13:48
@Simon: thanks for the interest - what I am trying to do is rather elementary and it is rather a pain that I have to go through all this. What I want is : I have a service (3 actually) that fire on regular intervals but can also be started manually from the activity (common activity for all 3). So when I start the activity I must disable the manual update button and reenable it when the service ends, in all possible scenarios (activity started before/after service etc). I have come up with a solution which I will post soon and let you know. — Mr_and_Mrs_D, Nov 24 '13 at 13:56

score 1 · Answer 1 · answered Nov 24 '13 at 12:25

1

I think You can create BroadcastReceiver in MonitorActivity. And send extras message from Service to enable/disable button.

answered Nov 24 '13 at 12:25

Serafins

1,237
1
17
36

No - they should be sticky and this is wrong (according to Hackborn) – Mr_and_Mrs_D Nov 24 '13 at 12:27
"they should be sticky" What does this mean? Please provide the linkl to Diane's post saying what's wrong. – Simon Nov 24 '13 at 12:29
@Simon: If I broadcast while the activity is not active the broadcast will be lost - but this is not an option - so the BC should be sticky - but sticky BC are for exceptional cases : [Er... there is NO reason to use sticky broadcasts for communication within your own app. In fact, I'll go farther and say you just should not do this](https://groups.google.com/d/msg/android-developers/8341SaXhvmY/QVCE7HGSnicJ) – Mr_and_Mrs_D Nov 24 '13 at 12:32
'BroadcastReceiver' is what you need, use it with 'LocalBroadcastManager' http://developer.android.com/reference/android/support/v4/content/LocalBroadcastManager.html – JafarKhQ Nov 24 '13 at 13:15

score 1 · Answer 2 · edited Dec 20 '13 at 22:37

1

I suggest you use LocalBroadcastManager
In your Activity define a BroadcastReceiver and register the Broadcast in ~~onStart()~~onResume() and unregister it in ~~onStop()~~onPause().
From your Service send the Broadcast to the Activity if the Activity is active it will receive the Broadcast and update the UI, if not nothing will happen.

Define another BroadcastReceiver in your Service, Register the Broadcast in onCreate() and Unregister it in onDestroy().
When your Activity is started send a Broadcast to the Service and let the Service reply to the Activity using the first Broadcast to update the UI.

UPDATE
After doing some investigation I found you're correct "sticky broadcasts are discouraged", but if you check the date of that post it's on 2008 - before Google implemented the LocalBroadcastManager.

And I have checked the source code of LocalBroadcastManager, it's not a real Broadcast it's an interface, Singleton with a list of BroadcastReceivers (not global and no IPC communication).

I really hate public static and I always avoid them. every body should.

edited Dec 20 '13 at 22:37

Mr_and_Mrs_D

32,208
39
178
361

answered Nov 24 '13 at 13:42

JafarKhQ

8,676
3
35
45

register the Broadcast in OnStart() and Unregister it in OnStop() ?? Anyway this is bit of a mess - if you follow my links above you will see that broadcasts are discouraged within the application. – Mr_and_Mrs_D Nov 24 '13 at 23:20
1- Android uses Broadcasts everywhere (Messages, Calls, Boot, Network update ....etc.). 2- The Broadcasts registered/unregistered in Activity aren’t sticky (Manifest Broadcast are). 3- You can Unregister the Broadcast in 'onPuse()' while this function must be called by Android even in low memory. 4- There is More choices by using libs like 'EventBus' https://github.com/greenrobot/EventBus and 'Otto' https://github.com/square/otto. – JafarKhQ Nov 25 '13 at 08:56
1. https://groups.google.com/d/msg/android-developers/8341SaXhvmY/QVCE7HGSnicJ. 2. ??? 3. that's what I'd do and register onResume 4. this is an overkill for my app but thanks for the links - have a look at my solution and compare it to yours - I will have a look at some point but not any time soon – Mr_and_Mrs_D Nov 25 '13 at 14:30
_Interface singleton ??_ Also "you hate public static" - well half the libraries you use are public static - like Thread.sleep(), Collections.sort() and System.out.println() ;) – Mr_and_Mrs_D Dec 20 '13 at 22:37
Back to the question - another problem with the receiver (yes still trying things out) is that if I unregister `onPause()` then if my activity is on the background (behind a dialog, or stopped) won't be updated - need code in onStart also etc. But failing to unregister the receiver has unknown consequences (in fact I asked) – Mr_and_Mrs_D Dec 20 '13 at 22:41
1

Came up with a solution - this way I can have my activity updating while visible but no worries about unregistration - see my answer. As for local broadcast manager - can be tricky: http://stackoverflow.com/questions/20820244/on-which-thread-does-onreceive-of-a-broacastreceiver-registered-with-localbroa/20832321#20832321 – Mr_and_Mrs_D Dec 30 '13 at 23:16

Mr_and_Mrs_D · Accepted Answer · 2014-01-01T16:47:32.637

So yes - the static button would leak my activity. I came up with callback below but it is ugly. I finally solved it by making the Activity extend OnSharedPreferenceChangeListener

public final class MonitorActivity extends FragmentActivity implements
        OnClickListener, OnSharedPreferenceChangeListener {

    private Button updateButton;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        updateButton = (Button) findViewById(R.id.update_data_button);
        updateButton.setOnClickListener(this); //no need to unregister methinks
    }

    @Override
    public synchronized void onSharedPreferenceChanged(
            SharedPreferences sharedPreferences, String key) {
        if (updateInProgressKey.equals(key)) {
            final Boolean isUpdating = AccessPreferences.get(this,
                updateInProgressKey, false);
            // set the button right
            updateButton.setText((isUpdating) ? sDefaultUpdatingText
                    : sUpdateButtonTxt);
            updateButton.setEnabled(!isUpdating);
        }
    }

    @Override
    protected void onStart() {
        super.onStart();
        AccessPreferences.registerListener(this, this);
        AccessPreferences.callListener(this, this, updateInProgressKey);
    }

    @Override
    protected void onStop() {
        // may not be called (as onStop() is killable), but no leak,
        // see: http://stackoverflow.com/a/20493608/281545
        AccessPreferences.unregisterListener(this, this);
        super.onStop();
    }
}

Callback

onPause() is guaranteed to run - so I null the static fields there and populate them on onResume(). I only do a read from default shared preferences so it should not take long in the synchronized blocks (I have to synchronize cause the service might call onUpdating() or onDataUpdated() any odd time). Not sure about unregistering the listener though

public class MonitorActivity extends FragmentActivity implements
        OnClickListener {

    private static TextView dataTextView; //null this onPause() to avoid a leak
    private static Button updateButton; // null this onPause() to avoid a leak
    // ...

    public static synchronized void onDataUpdated(Context ctx) {
        if (updateButton != null) {
            updateButton.setEnabled(true); // + set the text etc
        }
    }

    public static synchronized void onUpdating() {
        if (updateButton != null) {
            updateButton.setEnabled(false);
        }
    }

    @Override
    public void onClick(View v) {
        switch (v.getId()) {
        case R.id.update_data_button:
            serviceIntent.putExtra(MANUAL_UPDATE_INTENT_KEY, true);
            this.startService(serviceIntent);
        }
    }

    @Override
    protected void onResume() {
        super.onResume();
        synchronized (MonitorActivity.class) {
            Boolean isUpdating = AccessPreferences.get(this,
                updateInProgressKey, false);
            updateButton = (Button) findViewById(R.id.update_data_button);
            updateButton.setOnClickListener(this);
            // set the button right
            updateButton.setText((isUpdating) ? defaultUpdatingText
                    : getResources().getString(R.string.update_button_text));
            updateButton.setEnabled(!isUpdating);
        }
    }

    @Override
    protected void onPause() {
        synchronized (MonitorActivity.class) {
            updateButton.setOnClickListener(null); // TODO : needed ??
            dataTextView = updateButton = null; // to avoid leaking my activity
        }
        super.onPause();
    }
}

@Simon: that's what I came up with - still need to make sure it actually works in all cases but I think it'll do. Has the disadvantage of maybe updating the UI twice in some edhe cases (onResume() then onDataUpdated()) — Mr_and_Mrs_D, Nov 24 '13 at 23:24
Nice solution, cleaner code comparing with LocalBroadcastManager — JafarKhQ, Dec 31 '13 at 09:49

score 0 · Answer 4 · answered Aug 07 '19 at 03:36

0

There are 3 solutions for you:

Set button = null when context is destroyed(onStop);
Use WeakReference for button field, Example:

private static WeakReference<Button> updateButton;

Not creating static button. It's always hold the context.

answered Aug 07 '19 at 03:36

Trung Đoan

643
7
18

Android - does defining a button static leak my activity? Can I avoid it?

4 Answers4

Callback