forcedotcom / SalesforceMobileSDK-Android

Android SDK for Salesforce
Other
341 stars 388 forks source link

@W-17051459: [TRUST] Improve Android Lifecycle events/actions #2638

Closed JohnsonEricAtSalesforce closed 1 week ago

JohnsonEricAtSalesforce commented 1 week ago

🎸 Ready For Review! 🥁

This updates a deprecation for the OnLifecycleEvent code generation annotation in SalesforceSDKManager. It turns out the fix was pretty simple since SalesforceSDKManager already registered as a ProcessLifecycleOwner observer. It just needed to implement one of the LifecycleObserver sub-interfaces, such as DefaultLifecycleObserver, so the two existing lifecycle-related methods could be called without the deprecated code generation.

I tested this on API 35 and compared how the two lifecycle methods are called in comparison to the previous version. It certainly seems on par.

JohnsonEricAtSalesforce commented 1 week ago

@brandonpage, for this one:

Do we know if this fixes the issue of onAppForegrounded (now onResume(LifecycleOwner)) being triggered in rare occasions when the app isn't actually foregrounded?

How are we detecting that issue? If there's a use case we know of, I'd love to try. Otherwise, we might have to wait and see. This is still using the same library so I reckon a lot of the logic is actually unchanged. That said, we're also no longer using the annotation's generated code so that's one place where the undesired behavior might be eliminated.

JohnsonEricAtSalesforce commented 1 week ago

@brandonpage, for the next one:

And thoughts on keeping this approach vs the ActivityLifecycleCallbacks we have for dev support menu? Those supply the foregrounded activity so it seems like that would guarantee it?

So, I actually added that ActivityLifecycleCallbacks but would very much like to remove it someday 😉 It does serve its purpose, but it feels like the way we're capturing context and causes the manager to entangle rather than separate concerns. We can do that now since we can resolve the application from context. Someday when we do something about context this will likely change. The lifecycle library is really cool, it seems, since it abstracts away a lot of the details.

JohnsonEricAtSalesforce commented 1 week ago

Thinking more about context, it probably is OK to receive it in our initializers and use it as a local. So, perhaps the Activity lifecycle callbacks could remain. The concrete need I've been tracking with context is to no longer store static reference to it ✅