From 8e3dc066b2de2b6f412856d9093d02ae5e7cbafc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Brey?= Date: Tue, 12 Jul 2022 12:39:39 +0200 Subject: [PATCH] Add method in NewBaseController to handle NPE catches for viewbinding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is really a symptom of unreliable design around the async logic in Controllers. The data async calls should have been separate from the controller, so that the only asynchronicity the Controller has to deal with is UI-related, and can be cancelled safely on destroyView. In the future as those controllers are converted to something that has a separate ViewModel, this kind of catch should be completely unneeded. Signed-off-by: Álvaro Brey --- .../talk/controllers/ContactsController.kt | 40 +++---------------- .../controllers/base/NewBaseController.kt | 23 ++++++++++- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/controllers/ContactsController.kt b/app/src/main/java/com/nextcloud/talk/controllers/ContactsController.kt index 6af75b2d3..1fec9ecd1 100644 --- a/app/src/main/java/com/nextcloud/talk/controllers/ContactsController.kt +++ b/app/src/main/java/com/nextcloud/talk/controllers/ContactsController.kt @@ -384,7 +384,6 @@ class ContactsController(args: Bundle) : } } - @Suppress("Detekt.TooGenericExceptionCaught") private fun fetchData() { dispose(null) alreadyFetching = true @@ -440,33 +439,21 @@ class ContactsController(args: Bundle) : adapter?.filterItems() } - try { + withNullableControllerViewBinding { binding.controllerGenericRv.swipeRefreshLayout.isRefreshing = false - } catch (npe: NullPointerException) { - // view binding can be null - // since this is called asynchronously and UI might have been destroyed in the meantime - Log.i(TAG, "UI destroyed - view binding already gone") } } override fun onError(e: Throwable) { - try { + withNullableControllerViewBinding { binding.controllerGenericRv.swipeRefreshLayout.isRefreshing = false - } catch (npe: NullPointerException) { - // view binding can be null - // since this is called asynchronously and UI might have been destroyed in the meantime - Log.i(TAG, "UI destroyed - view binding already gone") } dispose(contactsQueryDisposable) } override fun onComplete() { - try { + withNullableControllerViewBinding { binding.controllerGenericRv.swipeRefreshLayout.isRefreshing = false - } catch (npe: NullPointerException) { - // view binding can be null - // since this is called asynchronously and UI might have been destroyed in the meantime - Log.i(TAG, "UI destroyed - view binding already gone") } dispose(contactsQueryDisposable) alreadyFetching = false @@ -692,7 +679,6 @@ class ContactsController(args: Bundle) : dispose(null) } - @Suppress("Detekt.TooGenericExceptionCaught") override fun onQueryTextChange(newText: String): Boolean { if (newText != "" && adapter?.hasNewFilter(newText) == true) { adapter?.setFilter(newText) @@ -702,12 +688,8 @@ class ContactsController(args: Bundle) : adapter?.updateDataSet(contactItems as List?) } - try { + withNullableControllerViewBinding { binding.controllerGenericRv.swipeRefreshLayout.isEnabled = !adapter!!.hasFilter() - } catch (npe: NullPointerException) { - // view binding can be null - // since this is called asynchronously and UI might have been destroyed in the meantime - Log.i(TAG, "UI destroyed - view binding already gone") } return true @@ -933,9 +915,8 @@ class ContactsController(args: Bundle) : } } - @Suppress("Detekt.TooGenericExceptionCaught") private fun toggleConversationPrivacyLayout(showInitialLayout: Boolean) { - try { + withNullableControllerViewBinding { if (showInitialLayout) { binding.conversationPrivacyToggle.initialRelativeLayout.visibility = View.VISIBLE binding.conversationPrivacyToggle.secondaryRelativeLayout.visibility = View.GONE @@ -943,26 +924,17 @@ class ContactsController(args: Bundle) : binding.conversationPrivacyToggle.initialRelativeLayout.visibility = View.GONE binding.conversationPrivacyToggle.secondaryRelativeLayout.visibility = View.VISIBLE } - } catch (npe: NullPointerException) { - // view binding can be null - // since this is called asynchronously and UI might have been destroyed in the meantime - Log.i(TAG, "UI destroyed - view binding already gone") } } - @Suppress("Detekt.TooGenericExceptionCaught") private fun toggleConversationViaLinkVisibility(isPublicCall: Boolean) { - try { + withNullableControllerViewBinding { if (isPublicCall) { binding.joinConversationViaLink.joinConversationViaLinkRelativeLayout.visibility = View.GONE updateGroupParticipantSelection() } else { binding.joinConversationViaLink.joinConversationViaLinkRelativeLayout.visibility = View.VISIBLE } - } catch (npe: NullPointerException) { - // view binding can be null - // since this is called asynchronously and UI might have been destroyed in the meantime - Log.i(TAG, "UI destroyed - view binding already gone") } } diff --git a/app/src/main/java/com/nextcloud/talk/controllers/base/NewBaseController.kt b/app/src/main/java/com/nextcloud/talk/controllers/base/NewBaseController.kt index 00aef33ac..a26f39247 100644 --- a/app/src/main/java/com/nextcloud/talk/controllers/base/NewBaseController.kt +++ b/app/src/main/java/com/nextcloud/talk/controllers/base/NewBaseController.kt @@ -54,10 +54,10 @@ import com.nextcloud.talk.controllers.ServerSelectionController import com.nextcloud.talk.controllers.SwitchAccountController import com.nextcloud.talk.controllers.WebViewLoginController import com.nextcloud.talk.controllers.base.providers.ActionBarProvider +import com.nextcloud.talk.controllers.util.ControllerViewBindingDelegate import com.nextcloud.talk.databinding.ActivityMainBinding import com.nextcloud.talk.utils.DisplayUtils import com.nextcloud.talk.utils.preferences.AppPreferences -import java.util.ArrayList import javax.inject.Inject import kotlin.jvm.internal.Intrinsics @@ -296,6 +296,27 @@ abstract class NewBaseController(@LayoutRes var layoutRes: Int, args: Bundle? = } } + /** + * Mainly intended to be used in async listeners that may be called after the controller has been destroyed. + * + * If you need to use this function to patch a NPE crash, something is wrong in the way that the async calls are + * handled, they should have been cancelled when the controller UI was destroyed (if their only purpose was + * updating UI). + */ + @Suppress("Detekt.TooGenericExceptionCaught") + inline fun withNullableControllerViewBinding(block: () -> Unit) { + try { + block() + } catch (e: NullPointerException) { + // Handle only the exceptions we know about, let everything else pass through + if (e.stackTrace.firstOrNull()?.className == ControllerViewBindingDelegate::class.qualifiedName) { + Log.w("ControllerViewBinding", "Trying to update UI on a null ViewBinding.", e) + } else { + throw e + } + } + } + open val appBarLayoutType: AppBarLayoutType get() = AppBarLayoutType.TOOLBAR val searchHint: String