3/4 `fun View.setBackHandler()` replaces `val View.backPressedHandler` #1028

rjrjr · 2023-06-14T18:47:43Z

val View.backPressedHandler is overly complicated and kind of naive. The replacement, fun View.setBackHandler(), carefully echoes the API and behavior of @Composable fun BackHandler(). We also provide a single argument overload that take a nullable handler function, and sets enabled to true for non-null handlers, false for null.

The biggest change in implementation is that we now use the preferred OnBackPressedDispatcher.addCallback(LifecycleOwner, OnBackPressedCallback) overload, which should allow AndroidX to do all the lifecycle bookkeeping for us -- taking advantage of all the hard work we've done to make WorkflowLifecycleOwner behave correctly.

This work revealed a problem where there was no WorkflowLifecycleOwner in place in time for the first update of the content view in ScreenOverlayDialogFactory, which prevented us from repeating that mistake in the new ComponentDialog.setContent() extension function introduced two PRs up. So that's nice.

.../src/main/java/com/squareup/sample/timemachine/shakeable/ShakeableTimeMachineLayoutRunner.kt

samples/tictactoe/app/src/main/java/com/squareup/sample/authworkflow/LoginViewFactory.kt

...ui/core-android/src/main/java/com/squareup/workflow1/ui/container/ContentDialogSetContent.kt

rjrjr · 2023-06-14T19:21:08Z

...core-android/src/main/java/com/squareup/workflow1/ui/container/ScreenOverlayDialogFactory.kt

+        // so becomes our parent.
+        WorkflowLifecycleOwner.installOn(
+          view,
+          initialEnvironment.onBackPressedDispatcherOwner(view)


This will pull in the wrong OnBackPressedDispatcherOwner, but that was always the case. This problem is why we've deprecated ScreenOverlayDialogFactory and replaced it with fun ComponentDialog.setContent() -- it was fundamentally broken, requiring us to create the content view before the Dialog that would host it.

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/BackButtonScreen.kt

rjrjr · 2023-06-15T23:46:23Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewBackHandler.kt

Should name this file SetBackHandler.kt

steve-the-edwards · 2023-06-16T15:23:04Z

workflow-ui/core-android/src/androidTest/java/com/squareup/workflow1/ui/ViewBackHandlerTest.kt

+    .around(scenarioRule)
+    .around(IdlingDispatcherRule)
+
+  private var viewHandlerCount = 0


nit:

Suggested change

private var viewHandlerCount = 0

private var viewHandlerBackPressedCount = 0

steve-the-edwards · 2023-06-16T15:24:06Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/BackPressHandler.kt

@@ -13,6 +13,7 @@ import com.squareup.workflow1.ui.androidx.WorkflowAndroidXSupport.onBackPressedD
 * A function passed to [View.backPressedHandler], to be called if the back
 * button is pressed while that view is attached to a window.
 */
+@Deprecated("Use View.backHandler()")


Suggested change

@Deprecated("Use View.backHandler()")

@Deprecated("Use View.setBackHandler()")

steve-the-edwards · 2023-06-16T15:24:26Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/BackPressHandler.kt

 @WorkflowUiExperimentalApi
+@Deprecated("Use setOrClearBackHandler")


Suggested change

@Deprecated("Use setOrClearBackHandler")

@Deprecated("Use setBackHandler")

I see you went through some naming iterations 😄

steve-the-edwards · 2023-06-16T15:25:10Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewBackHandler.kt

+import com.squareup.workflow1.ui.androidx.WorkflowAndroidXSupport.onBackPressedDispatcherOwnerOrNull
+
+/**
+ * A function to be called if the device back button is pressed while this


Suggested change

* A function to be called if the device back button is pressed while this

* Sets a function to be called if the device back button is pressed while this

steve-the-edwards · 2023-06-16T15:25:58Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewBackHandler.kt

+  onBack: () -> Unit
+) {
+  val callback = onBackPressedCallbackOrNull ?: MutableOnBackPressedCallback().apply {
+    onBackPressedCallbackOrNull = this


well this is an interesting pattern. 🤔 .

steve-the-edwards · 2023-06-16T15:29:58Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewBackHandler.kt

+public fun View.setBackHandler(onBack: (() -> Unit)?) {
+  onBack?.let { setBackHandler(enabled = true, it) }
+    ?: setBackHandler(enabled = false) {}
+}


Do you think setBackHandler(null) is the most understandable way of clearing/disabling this?

I personally like setBackHandler(enabled = false) and then making {} the default value of onBack in the version of the function above (not needing this overload then). I could see how you could end up with setBackHandler(false) but even that I don't find confusing.

steve-the-edwards · 2023-06-16T15:30:00Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewBackHandler.kt

+private var View.onBackPressedCallbackOrNull: MutableOnBackPressedCallback?
+  get() = getTag(R.id.view_back_handler) as MutableOnBackPressedCallback?
+  set(value) {
+    setTag(R.id.view_back_handler, value)
+  }


You truly are an Android view maestro how you nicely abstract this. 👨🏻‍🍳 💋

steve-the-edwards · 2023-06-16T15:32:03Z

...core-android/src/main/java/com/squareup/workflow1/ui/container/ScreenOverlayDialogFactory.kt

      // decorView before updating, to ensure that the global androidx
      // OnBackPressedDispatcher doesn't fire any set by lower layers. We put this
      // in place before each call to show(), so the real content view will be able
      // to clobber it.
-      if (modal) content.view.backPressedHandler = {}
+      if (modal) content.view.setBackHandler {}


Given all the no-op handlers we use, having a default of {} might be nice. Although maybe that's just in these internal use cases?

`val View.backPressedHandler` is overly complicated and kind of naive. The replacement, `fun View.setBackHandler()`, carefully echoes the API and behavior of [`@Composable fun BackHandler()`](https://developer.android.com/reference/kotlin/androidx/activity/compose/package-summary#BackHandler(kotlin.Boolean,kotlin.Function0)). We also provide a single argument overload that take a nullable handler function, and sets `enabled` to true for non-`null` handlers, false for `null`. The biggest change in implementation is that we now use the preferred `OnBackPressedDispatcher.addCallback(LifecycleOwner, OnBackPressedCallback)` overload, which should allow AndroidX to do all the lifecycle bookkeeping for us -- taking advantage of all the hard work we've done to make `WorkflowLifecycleOwner` behave correctly. This work revealed a problem where there was no `WorkflowLifecycleOwner` in place in time for the first update of the content view in `ScreenOverlayDialogFactory`, which prevented us from repeating that mistake in the new `ComponentDialog.setContent()` extension function introduced two PRs up. So that's nice.

rjrjr changed the title ~~fun View.backHandler() replaces val View.backPressedHandler~~ 3/3 fun View.backHandler() replaces val View.backPressedHandler Jun 14, 2023

rjrjr commented Jun 14, 2023

View reviewed changes

rjrjr changed the title ~~3/3 fun View.backHandler() replaces val View.backPressedHandler~~ 3/3 fun View.backHandler() replaces val View.setBackPressedHandler Jun 14, 2023

rjrjr force-pushed the ray/3-new-back-handler branch from 4a789a8 to 053dccd Compare June 14, 2023 19:31

rjrjr changed the title ~~3/3 fun View.backHandler() replaces val View.setBackPressedHandler~~ 3/3 fun View.setBackHandler() replaces val View.backPressedHandler Jun 14, 2023

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from d29aea2 to f62b52f Compare June 14, 2023 21:04

rjrjr force-pushed the ray/3-new-back-handler branch 4 times, most recently from c7194c9 to 6664cfe Compare June 14, 2023 21:34

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from f62b52f to 35ce30e Compare June 14, 2023 22:44

rjrjr force-pushed the ray/3-new-back-handler branch 3 times, most recently from f4b8dc7 to 8900743 Compare June 14, 2023 22:52

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from 35ce30e to dfe3296 Compare June 14, 2023 22:56

rjrjr force-pushed the ray/3-new-back-handler branch from 8900743 to df94fc9 Compare June 14, 2023 22:56

helios175 reviewed Jun 15, 2023

View reviewed changes

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/container/BackButtonScreen.kt Show resolved Hide resolved

rjrjr force-pushed the ray/3-new-back-handler branch from df94fc9 to fcb88a7 Compare June 15, 2023 16:48

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from dfe3296 to faec646 Compare June 15, 2023 16:48

rjrjr marked this pull request as ready for review June 15, 2023 16:48

rjrjr requested review from a team and zach-klippenstein as code owners June 15, 2023 16:48

rjrjr force-pushed the ray/3-new-back-handler branch 2 times, most recently from bf2d754 to bb48afd Compare June 15, 2023 23:24

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from 78620bb to 08e8dd3 Compare June 15, 2023 23:24

rjrjr changed the title ~~3/3 fun View.setBackHandler() replaces val View.backPressedHandler~~ 3/4 fun View.setBackHandler() replaces val View.backPressedHandler Jun 15, 2023

rjrjr assigned steve-the-edwards Jun 15, 2023

rjrjr commented Jun 15, 2023

View reviewed changes

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/ViewBackHandler.kt

Copy link

Contributor Author

rjrjr Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should name this file SetBackHandler.kt

steve-the-edwards approved these changes Jun 16, 2023

View reviewed changes

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from 08e8dd3 to d417e33 Compare June 21, 2023 20:30

rjrjr force-pushed the ray/3-new-back-handler branch from bb48afd to af3caaf Compare June 21, 2023 20:30

rjrjr force-pushed the ray/3-new-back-handler branch from af3caaf to eb3de9c Compare June 24, 2023 00:51

rjrjr force-pushed the ray/2-install-on-back-dispatcher branch from d417e33 to 0335e65 Compare June 24, 2023 00:51

Base automatically changed from ray/2-install-on-back-dispatcher to ray/1-component-set-dialog June 26, 2023 21:40

rjrjr merged commit 2103b5f into ray/1-component-set-dialog Jun 26, 2023

rjrjr deleted the ray/3-new-back-handler branch June 26, 2023 21:41

rjrjr mentioned this pull request Jun 26, 2023

1/4 fun ComponentDialog.setContent() replaces ScreenOverlayDialogFactory #1026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3/4 `fun View.setBackHandler()` replaces `val View.backPressedHandler` #1028

3/4 `fun View.setBackHandler()` replaces `val View.backPressedHandler` #1028

rjrjr commented Jun 14, 2023 •

edited

Loading

rjrjr Jun 14, 2023

rjrjr Jun 15, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

steve-the-edwards Jun 16, 2023

	private var viewHandlerCount = 0
	private var viewHandlerBackPressedCount = 0

	@Deprecated("Use View.backHandler()")
	@Deprecated("Use View.setBackHandler()")

		@WorkflowUiExperimentalApi
		@Deprecated("Use setOrClearBackHandler")

	@Deprecated("Use setOrClearBackHandler")
	@Deprecated("Use setBackHandler")

	* A function to be called if the device back button is pressed while this
	* Sets a function to be called if the device back button is pressed while this

3/4 fun View.setBackHandler() replaces val View.backPressedHandler #1028

3/4 fun View.setBackHandler() replaces val View.backPressedHandler #1028

Conversation

rjrjr commented Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3/4 `fun View.setBackHandler()` replaces `val View.backPressedHandler` #1028

3/4 `fun View.setBackHandler()` replaces `val View.backPressedHandler` #1028

rjrjr commented Jun 14, 2023 •

edited

Loading