Skip to content

Commit

Permalink
WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported.
Browse files Browse the repository at this point in the history
On a lot of Samsung devices running Anroid 12 / API 30, the sketchy `WorkflowLayout.takeWhileAttached` method at the heart of `WorkflowLayout.start` was having surprising effects -- we'd see redundant calls to `OnAttachStateChangeListener.onViewAttachedToWindow`. The problem goes away if we get more conventional and use the new `repeatOnLifecycle` function as described in [this blog post](https://medium.com/androiddevelopers/a-safer-way-to-collect-flows-from-android-uis-23080b1f8bda).

The old methods are deprecated and will be deleted soon.

Making this change required making our `BackPressHandler` mechanism more robust, as it too was pretending that `OnAttachStateChangeListener` was an adequate lifecycle. Specifically, we were relying on `onViewAttachedToWindow` to be called in a predicable order, which stopped being the case with the move away from `WorkflowLayout.takeWhileAttached`. This PR changes things so that…

- We register handlers immediately, so that we can count on them being stacked in the expected order
- We lean on `OnAttachStateChangeListener` solely to enable and disable the `BackPressHandler` associated with a view
- And take advantage of our recently added support for `ViewTreeLifecycleOwner` for clean up

`ModalContainer`'s contribution to coping with the back button also had to be beefed up, for similar reasons. It needs to ensure that back button events are blocked by modal windows, just like any other event, which is tricky with the singleton `OnBackPressedDispatcher` that androidx provides. It does this by putting a no-op handler in place as a backstop for any set by modal renderings. Its bespoke code for doing this was also inadvertently relying on the order of `onViewAttachedToWindow` calls. We now achieve this by wrapping modal renderings with a reliable `BackStackScreen`, which is promoted from the sample container set.
  • Loading branch information
rjrjr committed Jan 29, 2022
1 parent be479f2 commit 0176bb4
Show file tree
Hide file tree
Showing 23 changed files with 157 additions and 106 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,7 @@ class HelloBindingActivity : AppCompatActivity() {
val model: HelloBindingModel by viewModels()
setContentView(
WorkflowLayout(this).apply {
start(
renderings = model.renderings,
environment = viewEnvironment
)
start(lifecycle, model.renderings, viewEnvironment)
}
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class HelloComposeWorkflowActivity : AppCompatActivity() {
super.onCreate(savedInstanceState)
val model: HelloComposeModel by viewModels()
setContentView(
WorkflowLayout(this).apply { start(model.renderings) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings) }
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ class InlineRenderingActivity : AppCompatActivity() {

val model: HelloBindingModel by viewModels()
setContentView(
WorkflowLayout(this).apply {
start(renderings = model.renderings)
}
WorkflowLayout(this).apply { start(lifecycle, model.renderings) }
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ class NestedRenderingsActivity : AppCompatActivity() {
val model: NestedRenderingsModel by viewModels()
setContentView(
WorkflowLayout(this).apply {
start(
renderings = model.renderings,
environment = viewEnvironment
)
start(lifecycle, model.renderings, viewEnvironment)
}
)
}
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ import com.squareup.workflow1.ui.WorkflowUiExperimentalApi

@OptIn(WorkflowUiExperimentalApi::class)
val SampleContainers = ViewRegistry(
BackButtonViewFactory, OverviewDetailContainer, PanelContainer, ScrimContainer
OverviewDetailContainer, PanelContainer, ScrimContainer
)
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class PoetryActivity : AppCompatActivity() {

val model: PoetryModel by viewModels()
setContentView(
WorkflowLayout(this).apply { start(model.renderings, viewRegistry) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings, viewRegistry) }
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class RavenActivity : AppCompatActivity() {

val model: RavenModel by viewModels()
setContentView(
WorkflowLayout(this).apply { start(model.renderings, viewRegistry) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings, viewRegistry) }
)

lifecycleScope.launch {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.squareup.sample.hellobackbutton

import android.os.Parcelable
import com.squareup.sample.container.BackButtonScreen
import com.squareup.sample.hellobackbutton.AreYouSureWorkflow.Finished
import com.squareup.sample.hellobackbutton.AreYouSureWorkflow.State
import com.squareup.sample.hellobackbutton.AreYouSureWorkflow.State.Quitting
Expand All @@ -10,6 +9,7 @@ import com.squareup.workflow1.Snapshot
import com.squareup.workflow1.StatefulWorkflow
import com.squareup.workflow1.WorkflowAction.Companion.noAction
import com.squareup.workflow1.action
import com.squareup.workflow1.ui.BackButtonScreen
import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
import com.squareup.workflow1.ui.modal.AlertContainerScreen
import com.squareup.workflow1.ui.modal.AlertScreen
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class HelloBackButtonActivity : AppCompatActivity() {

val model: HelloBackButtonModel by viewModels()
setContentView(
WorkflowLayout(this).apply { start(model.renderings, viewRegistry) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings, viewRegistry) }
)

lifecycleScope.launch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class DungeonActivity : AppCompatActivity() {
val model: TimeMachineModel by viewModels { component.timeMachineModelFactory }

setContentView(
WorkflowLayout(this).apply { start(model.renderings, component.viewRegistry) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings, component.viewRegistry) }
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ class HelloWorkflowFragment : Fragment() {
// This ViewModel will survive configuration changes. It's instantiated
// by the first call to ViewModelProvider.get(), and that original instance is returned by
// succeeding calls, until this Fragment session ends.
val model: HelloViewModel = ViewModelProvider(this).get(HelloViewModel::class.java)
val model: HelloViewModel = ViewModelProvider(this)[HelloViewModel::class.java]

return WorkflowLayout(inflater.context).apply {
start(model.renderings)
start(lifecycle, model.renderings)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class HelloWorkflowActivity : AppCompatActivity() {
// succeeding calls.
val model: HelloViewModel by viewModels()
setContentView(
WorkflowLayout(this).apply { start(model.renderings) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings) }
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class StubVisibilityActivity : AppCompatActivity() {

val model: StubVisibilityModel by viewModels()
setContentView(
WorkflowLayout(this).apply { start(model.renderings) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings) }
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class TicTacToeActivity : AppCompatActivity() {
idlingResource = component.idlingResource

setContentView(
WorkflowLayout(this).apply { start(model.renderings, viewRegistry) }
WorkflowLayout(this).apply { start(lifecycle, model.renderings, viewRegistry) }
)

lifecycleScope.launch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class ToDoActivity : AppCompatActivity() {

setContentView(
WorkflowLayout(this).apply {
start(model.ensureWorkflow(traceFilesDir = filesDir), viewRegistry)
start(lifecycle, model.ensureWorkflow(traceFilesDir = filesDir), viewRegistry)
}
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ import android.view.ViewGroup
import android.view.ViewGroup.LayoutParams.MATCH_PARENT
import android.view.ViewGroup.LayoutParams.WRAP_CONTENT
import androidx.annotation.IdRes
import com.squareup.workflow1.ui.BackButtonScreen
import com.squareup.workflow1.ui.BuilderViewFactory
import com.squareup.workflow1.ui.ViewEnvironment
import com.squareup.workflow1.ui.ViewRegistry
import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
import com.squareup.workflow1.ui.backPressedHandler
import com.squareup.workflow1.ui.bindShowRendering
import com.squareup.workflow1.ui.buildView
import com.squareup.workflow1.ui.modal.ModalViewContainer.Companion.binding
Expand Down Expand Up @@ -62,22 +62,23 @@ public open class ModalViewContainer @JvmOverloads constructor(
initialModalRendering: Any,
initialViewEnvironment: ViewEnvironment
): DialogRef<Any> {
// Put a no-op backPressedHandler behind the given rendering, to
// ensure that the `onBackPressed` call below will not leak up to handlers
// that should be blocked by this modal session.
val wrappedRendering = BackButtonScreen(initialModalRendering) { }

val view = initialViewEnvironment[ViewRegistry]
// Notice that we don't pass a custom initializeView function to set the
// WorkflowLifecycleOwner here. ModalContainer will do that itself, on the parent of the view
// created here.
.buildView(
initialRendering = initialModalRendering,
initialRendering = wrappedRendering,
initialViewEnvironment = initialViewEnvironment,
contextForNewView = this.context,
container = this
)
.apply {
start()
// If the modal's root view has no backPressedHandler, add a no-op one to
// ensure that the `onBackPressed` call below will not leak up to handlers
// that should be blocked by this modal session.
if (backPressedHandler == null) backPressedHandler = { }
}

return buildDialogForView(view)
Expand Down Expand Up @@ -109,7 +110,11 @@ public open class ModalViewContainer @JvmOverloads constructor(
}

override fun updateDialog(dialogRef: DialogRef<Any>) {
with(dialogRef) { (extra as View).showRendering(modalRendering, viewEnvironment) }
with(dialogRef) {
// Have to preserve the wrapping done in buildDialog
val wrappedRendering = BackButtonScreen(modalRendering) { }
(extra as View).showRendering(wrappedRendering, viewEnvironment)
}
}

@PublishedApi
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import org.junit.Test
import kotlin.test.assertFailsWith

@OptIn(WorkflowUiExperimentalApi::class)
class BackStackScreenTest {
internal class BackStackScreenTest {
@Test fun `top is last`() {
assertThat(BackStackScreen(1, 2, 3, 4).top).isEqualTo(4)
}
Expand Down
12 changes: 12 additions & 0 deletions workflow-ui/core-android/api/core-android.api
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ public abstract interface class com/squareup/workflow1/ui/AndroidViewRendering {
public abstract fun getViewFactory ()Lcom/squareup/workflow1/ui/ViewFactory;
}

public final class com/squareup/workflow1/ui/BackButtonScreen : com/squareup/workflow1/ui/AndroidViewRendering {
public fun <init> (Ljava/lang/Object;ZLkotlin/jvm/functions/Function0;)V
public synthetic fun <init> (Ljava/lang/Object;ZLkotlin/jvm/functions/Function0;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun getOnBackPressed ()Lkotlin/jvm/functions/Function0;
public final fun getOverride ()Z
public fun getViewFactory ()Lcom/squareup/workflow1/ui/ViewFactory;
public final fun getWrapped ()Ljava/lang/Object;
}

public final class com/squareup/workflow1/ui/BackPressHandlerKt {
public static final fun getBackPressedHandler (Landroid/view/View;)Lkotlin/jvm/functions/Function0;
public static final fun onBackPressedDispatcherOwnerOrNull (Landroid/content/Context;)Landroidx/activity/OnBackPressedDispatcherOwner;
Expand Down Expand Up @@ -203,8 +212,11 @@ public abstract interface class com/squareup/workflow1/ui/ViewStarter {
public final class com/squareup/workflow1/ui/WorkflowLayout : android/widget/FrameLayout {
public fun <init> (Landroid/content/Context;Landroid/util/AttributeSet;)V
public synthetic fun <init> (Landroid/content/Context;Landroid/util/AttributeSet;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun start (Landroidx/lifecycle/Lifecycle;Lkotlinx/coroutines/flow/Flow;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
public final fun start (Landroidx/lifecycle/Lifecycle;Lkotlinx/coroutines/flow/Flow;Lcom/squareup/workflow1/ui/ViewRegistry;)V
public final fun start (Lkotlinx/coroutines/flow/Flow;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
public final fun start (Lkotlinx/coroutines/flow/Flow;Lcom/squareup/workflow1/ui/ViewRegistry;)V
public static synthetic fun start$default (Lcom/squareup/workflow1/ui/WorkflowLayout;Landroidx/lifecycle/Lifecycle;Lkotlinx/coroutines/flow/Flow;Lcom/squareup/workflow1/ui/ViewEnvironment;ILjava/lang/Object;)V
public static synthetic fun start$default (Lcom/squareup/workflow1/ui/WorkflowLayout;Lkotlinx/coroutines/flow/Flow;Lcom/squareup/workflow1/ui/ViewEnvironment;ILjava/lang/Object;)V
public final fun update (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package com.squareup.workflow1.ui

/**
* Adds optional back button handling to a [wrapped] rendering, possibly overriding that
* the wrapped rendering's own back button handler.
*
* @param override If `true`, [onBackPressed] is set as the
* [backPressedHandler][android.view.View.backPressedHandler] after
* the [wrapped] rendering's view is built / updated, effectively overriding it.
* If false, [onBackPressed] is set afterward, to allow the wrapped rendering to
* take precedence if it sets a `backPressedHandler` of its own .
* Defaults to `false`.
*
* @param onBackPressed The function to fire when the device back button
* is pressed, or null to set no handler -- or clear a handler that was set previously.
* Defaults to `null`.
*/
@WorkflowUiExperimentalApi
public class BackButtonScreen<W : Any>(
public val wrapped: W,
public val override: Boolean = false,
public val onBackPressed: (() -> Unit)? = null
) : AndroidViewRendering<BackButtonScreen<*>> {
override val viewFactory: ViewFactory<BackButtonScreen<*>> = DecorativeViewFactory(
type = BackButtonScreen::class,
map = { outer -> outer.wrapped },
doShowRendering = { view, innerShowRendering, outerRendering, viewEnvironment ->
if (!outerRendering.override) {
// Place our handler before invoking innerShowRendering, so that
// its later calls to view.backPressedHandler will take precedence
// over ours.
view.backPressedHandler = outerRendering.onBackPressed
}

innerShowRendering.invoke(outerRendering.wrapped, viewEnvironment)

if (outerRendering.override) {
// Place our handler after invoking innerShowRendering, so that ours wins.
view.backPressedHandler = outerRendering.onBackPressed
}
}
)
}
Loading

0 comments on commit 0176bb4

Please sign in to comment.