Karl/BobStore
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'fix-409--nfc-swap-crash' into 'master'

Browse Source 
Fix crash when returning to swap after cancelling

Fixes #409. The problem was that there was some listeners being added for broadcast events when the swap view was shown. These were never removed, and so cancelling swap, then returning to it would spin up a _new_ activity with new views, while the old listeners were still around. When the old listeners received events, they would try to talk to their associated `Activity`. This no longer existed, so a crash ensued.

While I was fixing the specific bug associated with #409, I took the opportunity to make more of the event listeners well behaved in the swap process. I don't think any of them were liable to cause crashes, but were likely to cause some weirdness at some point in time if they were not fixed.

*Note:* This swap view was an exercise in moving away from `Fragment`s towards an `Activity` with individual `View`s. I'm going to call this a bit of a failure at this point, because there is so much work that needs to be invested in implementing lifecycle stuff in our custom views. `Fragment`s naturally come with lifecycle methods that are familiar to other Android dev's looking to contribute to this project (even if they are a little difficult to understand at times). Implementing our own custom Views instead still results in similar classes of bugs (i.e. talking to an `Activity` when the view no longer is part of that activity).

A classic example of this is in my usage of the `onDetachedFromWindow` function in the `View`. I have no idea if this is the best place to unregister listeners or not. In a Fragment, it would be a matter of `onPause` or one of the more well defined lifecycle methods. Empirically, `onDetachedFromWindow` seems to be well behaved. The other alternative would be for the Activity to explicitly invoke a `onRemoved` type method each view when it knows it is transitioning from one state to the next. However at this point, we are then really into reimplementing `Fragment` land.

See merge request !232
This commit is contained in:
Hans-Christoph Steiner 2016-03-25 10:22:47 +00:00
commit ba9f32e9f4
4 changed files with 92 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
F-Droid/src/org/fdroid/fdroid/views/swap/JoinWifiView.java
View File

@ -60,19 +60,23 @@ public class JoinWifiView extends RelativeLayout implements SwapWorkflowActivity
});
refreshWifiState();
// TODO: This is effectively swap state management code, shouldn't be isolated to the
// WifiStateChangeService, but should be bundled with the main swap state handling code.
LocalBroadcastManager.getInstance(getActivity()).registerReceiver(
new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
refreshWifiState();
}
},
onWifiStateChange,
new IntentFilter(WifiStateChangeService.BROADCAST)
);
}
/**
* Remove relevant listeners/receivers/etc so that they do not receive and process events
* when this view is not in use.
*/
@Override
protected void onDetachedFromWindow() {
super.onDetachedFromWindow();
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(onWifiStateChange);
}
// TODO: Listen for "Connecting..." state and reflect that in the view too.
private void refreshWifiState() {
TextView descriptionView = (TextView) findViewById(R.id.text_description);
@ -138,4 +142,11 @@ public class JoinWifiView extends RelativeLayout implements SwapWorkflowActivity
public String getToolbarTitle() {
return getResources().getString(R.string.swap_join_same_wifi);
}
private final BroadcastReceiver onWifiStateChange = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
refreshWifiState();
}
};
}

100
F-Droid/src/org/fdroid/fdroid/views/swap/SwapAppsView.java
View File

@ -8,7 +8,6 @@ import android.content.Intent;
import android.content.IntentFilter;
import android.database.ContentObserver;
import android.database.Cursor;
import android.graphics.drawable.Drawable;
import android.net.Uri;
import android.os.Build;
import android.os.Bundle;
@ -32,7 +31,6 @@ import android.view.MenuInflater;
import android.view.MenuItem;
import android.view.View;
import android.view.ViewGroup;
import android.widget.AdapterView;
import android.widget.Button;
import android.widget.ImageView;
import android.widget.ListView;
@ -110,18 +108,24 @@ public class SwapAppsView extends ListView implements
// either reconnect with an existing loader or start a new one
getActivity().getSupportLoaderManager().initLoader(LOADER_SWAPABLE_APPS, null, this);
setOnItemClickListener(new OnItemClickListener() {
public void onItemClick(AdapterView<?> parent, View v, int position, long id) {
showAppDetails(position);
}
});
displayImageOptions = Utils.getImageLoadingOptions().build();
LocalBroadcastManager.getInstance(getActivity()).registerReceiver(
pollForUpdatesReceiver, new IntentFilter(UpdateService.LOCAL_ACTION_STATUS));
schedulePollForUpdates();
}
private BroadcastReceiver pollForUpdatesReceiver;
/**
* Remove relevant listeners/receivers/etc so that they do not receive and process events
* when this view is not in use.
*/
@Override
public void onDetachedFromWindow() {
super.onDetachedFromWindow();
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(pollForUpdatesReceiver);
}
private void pollForUpdates() {
if (adapter.getCount() > 1 ||
@ -132,37 +136,6 @@ public class SwapAppsView extends ListView implements
Utils.debugLog(TAG, "Polling swap repo to see if it has any updates.");
getActivity().getService().refreshSwap();
if (pollForUpdatesReceiver != null) {
pollForUpdatesReceiver = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
int statusCode = intent.getIntExtra(UpdateService.EXTRA_STATUS_CODE, -1);
switch (statusCode) {
case UpdateService.STATUS_COMPLETE_WITH_CHANGES:
Utils.debugLog(TAG, "Swap repo has updates, notifying the list adapter.");
getActivity().runOnUiThread(new Runnable() {
@Override
public void run() {
adapter.notifyDataSetChanged();
}
});
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(pollForUpdatesReceiver);
break;
case UpdateService.STATUS_ERROR_GLOBAL:
// TODO: Well, if we can't get the index, we probably can't swapp apps.
// Tell the user somethign helpful?
break;
case UpdateService.STATUS_COMPLETE_AND_SAME:
schedulePollForUpdates();
break;
}
}
};
// TODO: Unregister this properly, not just when successful (see swithc statement above)
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(pollForUpdatesReceiver);
}
}
private void schedulePollForUpdates() {
@ -212,12 +185,6 @@ public class SwapAppsView extends ListView implements
return getResources().getString(R.string.swap_success);
}
private void showAppDetails(int position) {
Cursor c = (Cursor) adapter.getItem(position);
App app = new App(c);
// TODO: Show app details screen.
}
@Override
public CursorLoader onCreateLoader(int id, Bundle args) {
Uri uri = TextUtils.isEmpty(mCurrentFilterString)
@ -354,10 +321,10 @@ public class SwapAppsView extends ListView implements
this.app = app;
apkToInstall = null; // Force lazy loading to fetch the correct apk next time.
// NOTE: Instead of continually unregistering and reregistering the observer
// NOTE: Instead of continually unregistering and re-registering the observer
// (with a different URI), this could equally be done by only having one
// registration in the constructor, and using the ContentObserver.onChange(boolean, URI)
// method and inspecting the URI to see if it maches. However, this was only
// method and inspecting the URI to see if it matches. However, this was only
// implemented on API-16, so leaving like this for now.
getActivity().getContentResolver().unregisterContentObserver(appObserver);
getActivity().getContentResolver().registerContentObserver(
@ -437,9 +404,6 @@ public class SwapAppsView extends ListView implements
@Nullable
private LayoutInflater inflater;
@Nullable
private Drawable defaultAppIcon;
AppListAdapter(@NonNull Context context, @Nullable Cursor c) {
super(context, c, FLAG_REGISTER_CONTENT_OBSERVER);
}
@ -452,13 +416,6 @@ public class SwapAppsView extends ListView implements
return inflater;
}
private Drawable getDefaultAppIcon(Context context) {
if (defaultAppIcon == null) {
defaultAppIcon = context.getResources().getDrawable(android.R.drawable.sym_def_app_icon);
}
return defaultAppIcon;
}
@Override
public View newView(Context context, Cursor cursor, ViewGroup parent) {
View view = getInflater(context).inflate(R.layout.swap_app_list_item, parent, false);
@ -486,4 +443,31 @@ public class SwapAppsView extends ListView implements
}
}
private final BroadcastReceiver pollForUpdatesReceiver = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
int statusCode = intent.getIntExtra(UpdateService.EXTRA_STATUS_CODE, -1);
switch (statusCode) {
case UpdateService.STATUS_COMPLETE_WITH_CHANGES:
Utils.debugLog(TAG, "Swap repo has updates, notifying the list adapter.");
getActivity().runOnUiThread(new Runnable() {
@Override
public void run() {
adapter.notifyDataSetChanged();
}
});
break;
case UpdateService.STATUS_ERROR_GLOBAL:
// TODO: Well, if we can't get the index, we probably can't swapp apps.
// Tell the user something helpful?
break;
case UpdateService.STATUS_COMPLETE_AND_SAME:
schedulePollForUpdates();
break;
}
}
};
}

13
F-Droid/src/org/fdroid/fdroid/views/swap/SwapConnecting.java
View File

@ -61,13 +61,24 @@ public class SwapConnecting extends LinearLayout implements SwapWorkflowActivity
}
});
// TODO: Unregister correctly, not just when being notified of completion or errors.
LocalBroadcastManager.getInstance(getActivity()).registerReceiver(
repoUpdateReceiver, new IntentFilter(UpdateService.LOCAL_ACTION_STATUS));
LocalBroadcastManager.getInstance(getActivity()).registerReceiver(
prepareSwapReceiver, new IntentFilter(SwapWorkflowActivity.PrepareSwapRepo.ACTION));
}
/**
* Remove relevant listeners/receivers/etc so that they do not receive and process events
* when this view is not in use.
*/
@Override
protected void onDetachedFromWindow() {
super.onDetachedFromWindow();
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(repoUpdateReceiver);
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(prepareSwapReceiver);
}
private final BroadcastReceiver repoUpdateReceiver = new ConnectSwapReceiver();
private final BroadcastReceiver prepareSwapReceiver = new PrepareSwapReceiver();

26
F-Droid/src/org/fdroid/fdroid/views/swap/WifiQrView.java
View File

@ -78,16 +78,19 @@ public class WifiQrView extends ScrollView implements SwapWorkflowActivity.Inner
}
});
// TODO: Unregister this receiver properly.
LocalBroadcastManager.getInstance(getActivity()).registerReceiver(
new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
setUIFromWifi();
onWifiStateChanged, new IntentFilter(WifiStateChangeService.BROADCAST));
}
},
new IntentFilter(WifiStateChangeService.BROADCAST)
);
/**
* Remove relevant listeners/receivers/etc so that they do not receive and process events
* when this view is not in use.
*/
@Override
protected void onDetachedFromWindow() {
super.onDetachedFromWindow();
LocalBroadcastManager.getInstance(getActivity()).unregisterReceiver(onWifiStateChanged);
}
@Override
@ -167,4 +170,11 @@ public class WifiQrView extends ScrollView implements SwapWorkflowActivity.Inner
}
private BroadcastReceiver onWifiStateChanged = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
setUIFromWifi();
}
};
}