Fix crash when returning to swap after cancelling

Fixes #409 (closed). 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 (closed), 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 Fragments towards an Activity with individual Views. 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. Fragments 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.

@eighthave has a much better understanding of this code than I do, so I'd rather have him review and merge.

Do you think this should make it into 0.99.1?

Well, if you are up for it we may as well. Just make sure to cherry-pick all of the commits. I wouldn't expect it to be problematic to include these commits in 0.99.1.

Glad to see this moving more event-based with proper registering and unregistering!

The View technique really needs to be event-based to succeed. That means using "event buses" like LocalBroadcastManager rather than lifecycle callbacks to update the View. The View lifecycle callbacks should be for registering and unregistering for events. Unregistering is essential, and that lack is clearly one key cause of the instability of this swap implementation.

It also requires that all state and related events are stored in a long-lived Service and broadcasts need to coordinated there. Anything relying on methods in an Activity will have issues around the lifecycle since the user can switch away from the Activity, yet the swap stuff is still active (web server, etc).

Reassigned to @eighthave

That said, after testing this update on two devices, I'm totally unable to get a working swap.

so let's leave this out of 0.99.1 and just push that update ASAP.

The View lifecycle callbacks should be for registering and unregistering for events

Yup, agreed. The problem I have is that I'm reinventing the lifecycle callbacks in the absense of fragments, and I don't know if I'm doing a substandard job or not.

Unregistering is essential, and that lack is clearly one key cause of the instability of this swap implementation.

Yeah. That is what this MR is about. If you are happy with it, please merge into master.

That said, after testing this update on two devices, I'm totally unable to get a working swap.

Right, do you have any specific feedback on this? I understand the process is a bit flakey, but each time I get some feedback (e.g. on the Nfc error that lead to this MR) I can try to nail it down and improve the process. For example, the main problem I'm seeing now is due to #568 (closed). One swap session remembers all of the apks from a previous swap session, even if the list has changed. Other than that, I'm getting more robust behaviour when using the WiFi swapping now.

I think we should pause on fixing the current swap implementation, and think more about changing the core structure. The UI is mostly good as it is, just a few things need fixing.

For me, the swap failed differently each time, more or less, so its hard to give feedback. I'm mostly trying via the discovery mode and with wifi, though I also tried with QR Codes, never with NFC. The "nearby" scrollbox always only ever shows one device, even if there are more than one. And usually, when it does discover something, it shows multiple copies of the same thing in that list, sometimes it showed like 10 or more copies of a nearby bluetooth device.

I had a pretty smooth test with this MR on an ASUS ZenFone2 and a Xiaomi device. Its still failing with the Samsung Galaxy Tab 3, so I'll have to investigate there.

Status changed to merged

Milestone changed to 0.100

mentioned in merge request !247 (merged)