Skip to content

Commit

Permalink
Replaces *.initializeView with *.viewStarter
Browse files Browse the repository at this point in the history 
Fixes #597.

It used to be the case that every `DecorativeViewFactory` called
`showRendering()` twice (#397). We fixed that (we thought) by introducing the
`initializeView` lambda to `ViewRegistry.buildView` and
`DecorativeViewFactory` (#408).

Unfortunately, that fix botched recursion. Individual `DecorativeViewFactory`
instances work fine, but if you wrap them you still get one `showRendering`
call from each. Worse, upstream `initializeView` lambdas are clobbered by
immediately downstream ones. e.g., when a `WorkflowViewStub` shows a
`DecorativeViewFactory`, the `WorkflowLifecycleRunner.installOn` call in the
former is clobbered.

The fix is to completely decouple building a view from from this kind of
initialization. `ViewRegistry.buildView` and its wrappers no longer try to
call `showRendering` at all.

Instead the caller of `buildView` (mostly `WorkflowViewStub`) is reponsible
for immediately calling `View.start` on the new `View`. `View.start` makes
the initial `showRendering` call that formerly was the job of
`ViewFactory.buildView` -- the factory builds the view, and the container
turns the key. Since `View.start` is called only after all wrapped
`ViewFactory.buildView` functions have executed, we're certain it will only
happen once.

Of course we still need the ability to customize view initialization via
wrapping, especially to invoke `WorkflowLifecycleOwner.installOn`. To
accomodate that, the function that `View.start` executes can be wrapped via
the new `viewStarter` argument to `ViewRegistry.buildView` and
`DecorativeViewFactory`, which replaces `initializeView`.

This required a pretty thorough overhaul of `ViewShowRendering.kt`
The `ViewShowRenderingTag` that it hangs off of a view tag is renamed
`WorkflowViewState`, and extracted to a separate file.

`WorkflowViewState` is a sealed class with two implementations (`New` and
`Started`) to help us enforce the order of the `ViewRegistry.buildView`,
`View.bindShowRendering`, `View.start` and `View.showRendering` calls.
  • Loading branch information
@rjrjr
rjrjr committed Dec 10, 2021
1 parent e50052d commit 0320832
Show file tree
Hide file tree
Showing 13 changed files with 285 additions and 192 deletions.
3 changes: 3 additions & 0 deletions workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/WorkflowRendering.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import com.squareup.workflow1.ui.WorkflowViewStub
import com.squareup.workflow1.ui.getFactoryForRendering
import com.squareup.workflow1.ui.getShowRendering
import com.squareup.workflow1.ui.showRendering
import com.squareup.workflow1.ui.start
import kotlin.reflect.KClass

/**
Expand Down Expand Up @@ -184,6 +185,8 @@ private fun <R : Any> ViewFactory<R>.asComposeViewFactory() =
// we don't have access to that.
originalFactory.buildView(rendering, viewEnvironment, context, container = null)
.also { view ->
view.start()

// Mirrors the check done in ViewRegistry.buildView.
checkNotNull(view.getShowRendering<Any>()) {
"View.bindShowRendering should have been called for $view, typically by the " +
Expand Down
9 changes: 5 additions & 4 deletions ...container-android/src/main/java/com/squareup/workflow1/ui/backstack/BackStackContainer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ import com.squareup.workflow1.ui.canShowRendering
import com.squareup.workflow1.ui.compatible
import com.squareup.workflow1.ui.container.R
import com.squareup.workflow1.ui.getRendering
import com.squareup.workflow1.ui.showFirstRendering
import com.squareup.workflow1.ui.showRendering
import com.squareup.workflow1.ui.start

/**
* A container view that can display a stream of [BackStackScreen] instances.
Expand Down Expand Up @@ -103,11 +103,12 @@ public open class BackStackContainer @JvmOverloads constructor(
initialViewEnvironment = environment,
contextForNewView = this.context,
container = this,
initializeView = {
WorkflowLifecycleOwner.installOn(this)
showFirstRendering()
viewStarter = { view, doStart ->
WorkflowLifecycleOwner.installOn(view)
doStart()
}
)
newView.start()
viewStateCache.update(named.backStack, oldViewMaybe, newView)

val popped = currentRendering?.backStack?.any { compatible(it, named.top) } == true
Expand Down
2 changes: 2 additions & 0 deletions ...-ui/container-android/src/main/java/com/squareup/workflow1/ui/modal/ModalViewContainer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.squareup.workflow1.ui.buildView
import com.squareup.workflow1.ui.modal.ModalViewContainer.Companion.binding
import com.squareup.workflow1.ui.onBackPressedDispatcherOwnerOrNull
import com.squareup.workflow1.ui.showRendering
import com.squareup.workflow1.ui.start
import kotlin.reflect.KClass

/**
Expand Down Expand Up @@ -72,6 +73,7 @@ public open class ModalViewContainer @JvmOverloads constructor(
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.
Expand Down
70 changes: 47 additions & 23 deletions workflow-ui/core-android/api/core-android.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ public final class com/squareup/workflow1/ui/BuilderViewFactory : com/squareup/w
}

public final class com/squareup/workflow1/ui/DecorativeViewFactory : com/squareup/workflow1/ui/ViewFactory {
public fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function4;)V
public synthetic fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function4;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function4;)V
public synthetic fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function4;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function1;Lcom/squareup/workflow1/ui/ViewStarter;Lkotlin/jvm/functions/Function4;)V
public synthetic fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function1;Lcom/squareup/workflow1/ui/ViewStarter;Lkotlin/jvm/functions/Function4;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function2;Lcom/squareup/workflow1/ui/ViewStarter;Lkotlin/jvm/functions/Function4;)V
public synthetic fun <init> (Lkotlin/reflect/KClass;Lkotlin/jvm/functions/Function2;Lcom/squareup/workflow1/ui/ViewStarter;Lkotlin/jvm/functions/Function4;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun buildView (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
public fun getType ()Lkotlin/reflect/KClass;
}
Expand All @@ -51,21 +51,6 @@ public final class com/squareup/workflow1/ui/LayoutRunnerViewFactory : com/squar
public fun getType ()Lkotlin/reflect/KClass;
}

public final class com/squareup/workflow1/ui/ShowRenderingTag {
public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
public final fun component1 ()Ljava/lang/Object;
public final fun component2 ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public final fun component3 ()Lkotlin/jvm/functions/Function2;
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/ShowRenderingTag;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
public fun equals (Ljava/lang/Object;)Z
public final fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public final fun getShowRendering ()Lkotlin/jvm/functions/Function2;
public final fun getShowing ()Ljava/lang/Object;
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

public final class com/squareup/workflow1/ui/SnapshotParcelsKt {
public static final fun toSnapshot (Landroid/os/Parcelable;)Lcom/squareup/workflow1/Snapshot;
}
Expand Down Expand Up @@ -124,21 +109,24 @@ public final class com/squareup/workflow1/ui/ViewRegistry$Companion : com/square
public final class com/squareup/workflow1/ui/ViewRegistryKt {
public static final fun ViewRegistry ()Lcom/squareup/workflow1/ui/ViewRegistry;
public static final fun ViewRegistry ([Lcom/squareup/workflow1/ui/ViewFactory;)Lcom/squareup/workflow1/ui/ViewRegistry;
public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lkotlin/jvm/functions/Function1;)Landroid/view/View;
public static synthetic fun buildView$default (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Landroid/view/View;
public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lcom/squareup/workflow1/ui/ViewStarter;)Landroid/view/View;
public static synthetic fun buildView$default (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;Lcom/squareup/workflow1/ui/ViewStarter;ILjava/lang/Object;)Landroid/view/View;
public static final fun getFactoryForRendering (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;)Lcom/squareup/workflow1/ui/ViewFactory;
public static final fun plus (Lcom/squareup/workflow1/ui/ViewRegistry;Lcom/squareup/workflow1/ui/ViewFactory;)Lcom/squareup/workflow1/ui/ViewRegistry;
public static final fun plus (Lcom/squareup/workflow1/ui/ViewRegistry;Lcom/squareup/workflow1/ui/ViewRegistry;)Lcom/squareup/workflow1/ui/ViewRegistry;
public static final fun showFirstRendering (Landroid/view/View;)V
}

public final class com/squareup/workflow1/ui/ViewShowRenderingKt {
public static final fun bindShowRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
public static final fun canShowRendering (Landroid/view/View;Ljava/lang/Object;)Z
public static final fun getEnvironment (Landroid/view/View;)Lcom/squareup/workflow1/ui/ViewEnvironment;
public static final fun getRendering (Landroid/view/View;)Ljava/lang/Object;
public static final fun getShowRendering (Landroid/view/View;)Lkotlin/jvm/functions/Function2;
public static final fun showRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
public static final fun start (Landroid/view/View;)V
}

public abstract interface class com/squareup/workflow1/ui/ViewStarter {
public abstract fun startView (Landroid/view/View;Lkotlin/jvm/functions/Function0;)V
}

public final class com/squareup/workflow1/ui/WorkflowLayout : android/widget/FrameLayout {
Expand All @@ -149,6 +137,42 @@ public final class com/squareup/workflow1/ui/WorkflowLayout : android/widget/Fra
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 abstract class com/squareup/workflow1/ui/WorkflowViewState {
public abstract fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public abstract fun getShowRendering ()Lkotlin/jvm/functions/Function2;
}

public final class com/squareup/workflow1/ui/WorkflowViewState$New : com/squareup/workflow1/ui/WorkflowViewState {
public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function1;)V
public synthetic fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function1;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun component2 ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public final fun component3 ()Lkotlin/jvm/functions/Function2;
public final fun component4 ()Lkotlin/jvm/functions/Function1;
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function1;)Lcom/squareup/workflow1/ui/WorkflowViewState$New;
public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/WorkflowViewState$New;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/WorkflowViewState$New;
public fun equals (Ljava/lang/Object;)Z
public fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public fun getShowRendering ()Lkotlin/jvm/functions/Function2;
public synthetic fun getShowing ()Ljava/lang/Object;
public final fun getStarter ()Lkotlin/jvm/functions/Function1;
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

public final class com/squareup/workflow1/ui/WorkflowViewState$Started : com/squareup/workflow1/ui/WorkflowViewState {
public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
public final fun component2 ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public final fun component3 ()Lkotlin/jvm/functions/Function2;
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)Lcom/squareup/workflow1/ui/WorkflowViewState$Started;
public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/WorkflowViewState$Started;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/WorkflowViewState$Started;
public fun equals (Ljava/lang/Object;)Z
public fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
public fun getShowRendering ()Lkotlin/jvm/functions/Function2;
public synthetic fun getShowing ()Ljava/lang/Object;
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

public final class com/squareup/workflow1/ui/WorkflowViewStub : android/view/View {
public fun <init> (Landroid/content/Context;)V
public fun <init> (Landroid/content/Context;Landroid/util/AttributeSet;)V
Expand Down
78 changes: 41 additions & 37 deletions .../core-android/src/androidTest/java/com/squareup/workflow1/ui/DecorativeViewFactoryTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import org.junit.Test
internal class DecorativeViewFactoryTest {
private val instrumentation = InstrumentationRegistry.getInstrumentation()

@Test fun initializeView_is_only_call_to_showRendering() {
@Test fun viewStarter_is_only_call_to_showRendering() {
val events = mutableListOf<String>()

val innerViewFactory = object : ViewFactory<InnerRendering> {
Expand All @@ -38,27 +38,26 @@ internal class DecorativeViewFactoryTest {
val enhancedEnv = env + (envString to "Updated environment")
Pair(outer.wrapped, enhancedEnv)
},
initializeView = {
val outerRendering = getRendering<OuterRendering>()
events += "initializeView $outerRendering ${environment!![envString]}"
showFirstRendering()
events += "exit initializeView"
viewStarter = { view, doStart ->
events += "viewStarter ${view.getRendering<Any>()} ${view.environment!![envString]}"
doStart()
events += "exit viewStarter"
}
)
val viewRegistry = ViewRegistry(innerViewFactory)
val viewRegistry = ViewRegistry(innerViewFactory, outerViewFactory)
val viewEnvironment = ViewEnvironment(mapOf(ViewRegistry to viewRegistry))

outerViewFactory.buildView(
viewRegistry.buildView(
OuterRendering("outer", InnerRendering("inner")),
viewEnvironment,
instrumentation.context
)
).start()

assertThat(events).containsExactly(
"initializeView OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner)) " +
"viewStarter OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner)) " +
"Updated environment",
"inner showRendering InnerRendering(innerData=inner)",
"exit initializeView"
"exit viewStarter"
)
}

Expand Down Expand Up @@ -86,14 +85,14 @@ internal class DecorativeViewFactoryTest {
innerShowRendering(outerRendering.wrapped, env)
}
)
val viewRegistry = ViewRegistry(innerViewFactory)
val viewRegistry = ViewRegistry(innerViewFactory, outerViewFactory)
val viewEnvironment = ViewEnvironment(mapOf(ViewRegistry to viewRegistry))

outerViewFactory.buildView(
viewRegistry.buildView(
OuterRendering("outer", InnerRendering("inner")),
viewEnvironment,
instrumentation.context
)
).start()

assertThat(events).containsExactly(
"doShowRendering OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner))",
Expand Down Expand Up @@ -126,28 +125,27 @@ internal class DecorativeViewFactoryTest {
val outerViewFactory = DecorativeViewFactory(
type = OuterRendering::class,
map = { outer, env ->
val enhancedEnv = env + (envString to "Outer Updated environment")
val enhancedEnv = env + (envString to "Outer Updated environment" +
" SHOULD NOT SEE THIS! It will be clobbered by WayOutRendering")
Pair(outer.wrapped, enhancedEnv)
},
initializeView = {
val outerRendering = getRendering<OuterRendering>()
events += "outer initializeView $outerRendering ${environment!![envString]}"
showFirstRendering()
events += "exit outer initializeView"
viewStarter = { view, doStart ->
events += "outer viewStarter ${view.getRendering<Any>()} ${view.environment!![envString]}"
doStart()
events += "exit outer viewStarter"
}
)

val wayOutViewFactory = DecorativeViewFactory(
type = WayOutRendering::class,
map = { wayOut, env ->
val enhancedEnv = env + (envString to "Way Out Updated environment")
val enhancedEnv = env + (envString to "Way Out Updated environment triumphs over all")
Pair(wayOut.wrapped, enhancedEnv)
},
initializeView = {
val wayOutRendering = getRendering<WayOutRendering>()
events += "way out initializeView $wayOutRendering ${environment!![envString]}"
showFirstRendering()
events += "exit way out initializeView"
viewStarter = { view, doStart ->
events += "way out viewStarter ${view.getRendering<Any>()} ${view.environment!![envString]}"
doStart()
events += "exit way out viewStarter"
}
)
val viewRegistry = ViewRegistry(innerViewFactory, outerViewFactory, wayOutViewFactory)
Expand All @@ -157,21 +155,26 @@ internal class DecorativeViewFactoryTest {
WayOutRendering("way out", OuterRendering("outer", InnerRendering("inner"))),
viewEnvironment,
instrumentation.context
)
).start()

assertThat(events).containsExactly(
"way out initializeView " +
"way out viewStarter " +
"WayOutRendering(wayOutData=way out, wrapped=" +
"OuterRendering(outerData=outer, wrapped=" +
"InnerRendering(innerData=inner))) " +
"Way Out Updated environment",
"outer initializeView " +
"Way Out Updated environment triumphs over all",
"outer viewStarter " +
// Notice that both the initial rendering and the ViewEnvironment are stomped by
// the outermost wrapper before inners are invoked. Could try to give
// the inner wrapper access to the rendering it expected, but there are no
// use cases and it trashes the API.
"WayOutRendering(wayOutData=way out, wrapped=" +
"OuterRendering(outerData=outer, wrapped=" +
"InnerRendering(innerData=inner)) " +
"Outer Updated environment",
"InnerRendering(innerData=inner))) " +
"Way Out Updated environment triumphs over all",
"inner showRendering InnerRendering(innerData=inner)",
"exit outer initializeView",
"exit way out initializeView"
"exit outer viewStarter",
"exit way out viewStarter"
)
}

Expand Down Expand Up @@ -199,14 +202,14 @@ internal class DecorativeViewFactoryTest {
innerShowRendering(outerRendering.wrapped, env)
}
)
val viewRegistry = ViewRegistry(innerViewFactory)
val viewRegistry = ViewRegistry(innerViewFactory, outerViewFactory)
val viewEnvironment = ViewEnvironment(mapOf(ViewRegistry to viewRegistry))

val view = outerViewFactory.buildView(
val view = viewRegistry.buildView(
OuterRendering("out1", InnerRendering("in1")),
viewEnvironment,
instrumentation.context
)
).apply { start() }
events.clear()

view.showRendering(OuterRendering("out2", InnerRendering("in2")), viewEnvironment)
Expand All @@ -222,6 +225,7 @@ internal class DecorativeViewFactoryTest {
val outerData: String,
val wrapped: InnerRendering
)

private data class WayOutRendering(
val wayOutData: String,
val wrapped: OuterRendering
Expand Down

0 comments on commit 0320832

Please sign in to comment.