Skip to content

Commit a301ef5

Browse files
committed
bindShowRendering no longer calls showRendering.
That was always a weird coupling, and it caused the nasty compounding double update behavior in `DecorativeViewFactory`. To prevent `ViewFactory` implementations from having to remember to call `showRendering`, we make `ViewRegistry.buildView` do so. And to allow more sophisticiated `ViewFactory` implementations to prevent that call from happening prematurely, we build it into the default value for a new `initializeView` method on `ViewRegistry.buildView`. `DecorativeViewFactory` takes advantage by passing a no-op function in for `initializeView` when the view is built, and then making its own explicit call to `showRendering` after it's done playing wrapping games. Another interesting change is that the `showRendering` function now gets the `View` as its receiver. Made it that much easier for `ViewRegistry.buildView` to invoke it, and I suspect it will come in handy in other places too. Closes #397
1 parent c787931 commit a301ef5

File tree

18 files changed

+200
-167
lines changed

18 files changed

+200
-167
lines changed

samples/containers/android/src/main/java/com/squareup/sample/container/BackButtonViewFactory.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ by DecorativeViewFactory(
2121
view.backPressedHandler = outerRendering.onBackPressed
2222
}
2323

24-
innerShowRendering.invoke(outerRendering.wrapped, viewEnvironment)
24+
innerShowRendering.invoke(view, outerRendering.wrapped, viewEnvironment)
2525

2626
if (outerRendering.override) {
2727
// Place our handler after invoking innerShowRendering, so that ours wins.

samples/containers/android/src/main/java/com/squareup/sample/container/panel/PanelContainer.kt

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,15 @@ class PanelContainer @JvmOverloads constructor(
5959
}
6060

6161
companion object : ViewFactory<PanelContainerScreen<*, *>> by BuilderViewFactory(
62-
type = PanelContainerScreen::class,
63-
viewConstructor = { initialRendering, initialEnv, contextForNewView, _ ->
64-
PanelContainer(contextForNewView).apply {
65-
id = R.id.panel_container
66-
layoutParams = ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT)
67-
bindShowRendering(initialRendering, initialEnv, ::update)
62+
type = PanelContainerScreen::class,
63+
viewConstructor = { initialRendering, initialEnv, contextForNewView, _ ->
64+
PanelContainer(contextForNewView).apply {
65+
id = R.id.panel_container
66+
layoutParams = ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT)
67+
bindShowRendering(initialRendering, initialEnv) { rendering, env ->
68+
update(rendering, env)
6869
}
6970
}
71+
}
7072
)
7173
}

samples/containers/android/src/main/java/com/squareup/sample/container/panel/ScrimContainer.kt

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ import android.view.View
77
import android.view.ViewGroup
88
import com.squareup.sample.container.R
99
import com.squareup.workflow1.ui.BuilderViewFactory
10-
import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
1110
import com.squareup.workflow1.ui.ViewFactory
11+
import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
1212
import com.squareup.workflow1.ui.WorkflowViewStub
1313
import com.squareup.workflow1.ui.bindShowRendering
1414

@@ -33,7 +33,7 @@ class ScrimContainer @JvmOverloads constructor(
3333

3434
private val child: View
3535
get() = getChildAt(0)
36-
?: error("Child must be set immediately upon creation.")
36+
?: error("Child must be set immediately upon creation.")
3737

3838
var isDimmed: Boolean = false
3939
set(value) {
@@ -84,30 +84,28 @@ class ScrimContainer @JvmOverloads constructor(
8484
ValueAnimator.ofFloat(1f, 0f)
8585
}.apply {
8686
duration = resources.getInteger(android.R.integer.config_shortAnimTime)
87-
.toLong()
87+
.toLong()
8888
addUpdateListener { animation -> scrim.alpha = animation.animatedValue as Float }
8989
start()
9090
}
9191
}
9292

9393
@OptIn(WorkflowUiExperimentalApi::class)
9494
companion object : ViewFactory<ScrimContainerScreen<*>> by BuilderViewFactory(
95-
type = ScrimContainerScreen::class,
96-
viewConstructor = { initialRendering, initialViewEnvironment, contextForNewView, _ ->
97-
val stub = WorkflowViewStub(contextForNewView)
95+
type = ScrimContainerScreen::class,
96+
viewConstructor = { initialRendering, initialViewEnvironment, contextForNewView, _ ->
97+
val stub = WorkflowViewStub(contextForNewView)
9898

99-
ScrimContainer(contextForNewView)
100-
.apply {
101-
layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT)
102-
addView(stub)
99+
ScrimContainer(contextForNewView)
100+
.apply {
101+
layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT)
102+
addView(stub)
103103

104-
bindShowRendering(
105-
initialRendering, initialViewEnvironment
106-
) { rendering, environment ->
107-
stub.update(rendering.wrapped, environment)
108-
isDimmed = rendering.dimmed
109-
}
110-
}
111-
}
104+
bindShowRendering(initialRendering, initialViewEnvironment) { rendering, environment ->
105+
stub.update(rendering.wrapped, environment)
106+
isDimmed = rendering.dimmed
107+
}
108+
}
109+
}
112110
)
113111
}

workflow-ui/backstack-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/BackStackContainerTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import org.junit.runner.RunWith
2121

2222
@OptIn(WorkflowUiExperimentalApi::class)
2323
@RunWith(AndroidJUnit4::class)
24-
class BackStackContainerTest {
24+
internal class BackStackContainerTest {
2525

2626
@Rule @JvmField val scenarioRule = ActivityScenarioRule(BackStackTestActivity::class.java)
2727
private val scenario get() = scenarioRule.scenario

workflow-ui/backstack-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/fixtures/BackStackTestActivity.kt

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import com.squareup.workflow1.ui.showRendering
2525
* [onRetainNonConfigurationInstance].
2626
*/
2727
@OptIn(WorkflowUiExperimentalApi::class)
28-
class BackStackTestActivity : Activity() {
28+
internal class BackStackTestActivity : Activity() {
2929

3030
/**
3131
* A simple string holder that creates [ViewStateTestView]s with their ID and tag derived from
@@ -35,7 +35,7 @@ class BackStackTestActivity : Activity() {
3535
* @param onViewCreated An optional function that will be called by the view factory after the
3636
* view is created but before [bindShowRendering].
3737
*/
38-
class TestRendering(
38+
internal class TestRendering(
3939
val name: String,
4040
val onViewCreated: (ViewStateTestView) -> Unit = {},
4141
val onShowRendering: (ViewStateTestView) -> Unit = {},
@@ -58,7 +58,7 @@ class BackStackTestActivity : Activity() {
5858
viewHooks = initialRendering.viewHooks
5959
initialRendering.onViewCreated(this)
6060
bindShowRendering(initialRendering, initialViewEnvironment) { rendering, _ ->
61-
rendering.onShowRendering(this)
61+
rendering.onShowRendering(this as ViewStateTestView)
6262
viewHooks = rendering.viewHooks
6363
}
6464
}
@@ -96,6 +96,7 @@ class BackStackTestActivity : Activity() {
9696
check(backstackContainer == null)
9797
backstackContainer =
9898
NoTransitionBackStackContainer.buildView(backstack!!, viewEnvironment, this)
99+
backstackContainer!!.showRendering(backstack!!, viewEnvironment)
99100
setContentView(backstackContainer)
100101
}
101102

workflow-ui/backstack-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/fixtures/NoTransitionBackStackContainer.kt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import com.squareup.workflow1.ui.bindShowRendering
1616
* actual backstack logic. Views are just swapped instantly.
1717
*/
1818
@OptIn(WorkflowUiExperimentalApi::class)
19-
class NoTransitionBackStackContainer(context: Context) : BackStackContainer(context) {
19+
internal class NoTransitionBackStackContainer(context: Context) : BackStackContainer(context) {
2020

2121
override fun performTransition(oldViewMaybe: View?, newView: View, popped: Boolean) {
2222
oldViewMaybe?.let(::removeView)
@@ -31,7 +31,9 @@ class NoTransitionBackStackContainer(context: Context) : BackStackContainer(cont
3131
.apply {
3232
id = R.id.workflow_back_stack_container
3333
layoutParams = LayoutParams(MATCH_PARENT, MATCH_PARENT)
34-
bindShowRendering(initialRendering, initialEnv, ::update)
34+
bindShowRendering(initialRendering, initialEnv) { rendering, env ->
35+
update(rendering, env)
36+
}
3537
}
3638
}
3739
)

workflow-ui/backstack-android/src/main/java/com/squareup/workflow1/ui/backstack/BackStackContainer.kt

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,12 @@ public open class BackStackContainer @JvmOverloads constructor(
6666
return
6767
}
6868

69-
val newView = environment[ViewRegistry].buildView(named.top, environment, this)
69+
val newView = environment[ViewRegistry].buildView(
70+
initialRendering = named.top,
71+
initialViewEnvironment = environment,
72+
contextForNewView = this.context,
73+
container = this
74+
)
7075
viewStateCache.update(named.backStack, oldViewMaybe, newView)
7176

7277
val popped = currentRendering?.backStack?.any { compatible(it, named.top) } == true
@@ -146,7 +151,9 @@ public open class BackStackContainer @JvmOverloads constructor(
146151
.apply {
147152
id = R.id.workflow_back_stack_container
148153
layoutParams = (ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT))
149-
bindShowRendering(initialRendering, initialEnv, ::update)
154+
bindShowRendering(initialRendering, initialEnv) { rendering, env ->
155+
update(rendering, env)
156+
}
150157
}
151158
}
152159
)

workflow-ui/core-android/api/core-android.api

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,15 @@ public final class com/squareup/workflow1/ui/NamedViewFactory : com/squareup/wor
5959
}
6060

6161
public final class com/squareup/workflow1/ui/ShowRenderingTag {
62-
public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
62+
public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;)V
6363
public final fun component1 ()Ljava/lang/Object;
6464
public final fun component2 ()Lcom/squareup/workflow1/ui/ViewEnvironment;
65-
public final fun component3 ()Lkotlin/jvm/functions/Function2;
66-
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
67-
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;
65+
public final fun component3 ()Lkotlin/jvm/functions/Function3;
66+
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
67+
public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/ShowRenderingTag;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/ShowRenderingTag;
6868
public fun equals (Ljava/lang/Object;)Z
6969
public final fun getEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
70-
public final fun getShowRendering ()Lkotlin/jvm/functions/Function2;
70+
public final fun getShowRendering ()Lkotlin/jvm/functions/Function3;
7171
public final fun getShowing ()Ljava/lang/Object;
7272
public fun hashCode ()I
7373
public fun toString ()Ljava/lang/String;
@@ -127,19 +127,19 @@ public final class com/squareup/workflow1/ui/ViewRegistry$Companion : com/square
127127
public final class com/squareup/workflow1/ui/ViewRegistryKt {
128128
public static final fun ViewRegistry ()Lcom/squareup/workflow1/ui/ViewRegistry;
129129
public static final fun ViewRegistry ([Lcom/squareup/workflow1/ui/ViewFactory;)Lcom/squareup/workflow1/ui/ViewRegistry;
130-
public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/content/Context;Landroid/view/ViewGroup;)Landroid/view/View;
131-
public static final fun buildView (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/view/ViewGroup;)Landroid/view/View;
132-
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;ILjava/lang/Object;)Landroid/view/View;
130+
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/Function3;)Landroid/view/View;
131+
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/Function3;ILjava/lang/Object;)Landroid/view/View;
132+
public static final fun getFactoryForRendering (Lcom/squareup/workflow1/ui/ViewRegistry;Ljava/lang/Object;)Lcom/squareup/workflow1/ui/ViewFactory;
133133
public static final fun plus (Lcom/squareup/workflow1/ui/ViewRegistry;Lcom/squareup/workflow1/ui/ViewFactory;)Lcom/squareup/workflow1/ui/ViewRegistry;
134134
public static final fun plus (Lcom/squareup/workflow1/ui/ViewRegistry;Lcom/squareup/workflow1/ui/ViewRegistry;)Lcom/squareup/workflow1/ui/ViewRegistry;
135135
}
136136

137137
public final class com/squareup/workflow1/ui/ViewShowRenderingKt {
138-
public static final fun bindShowRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function2;)V
138+
public static final fun bindShowRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Lkotlin/jvm/functions/Function3;)V
139139
public static final fun canShowRendering (Landroid/view/View;Ljava/lang/Object;)Z
140140
public static final fun getEnvironment (Landroid/view/View;)Lcom/squareup/workflow1/ui/ViewEnvironment;
141141
public static final fun getRendering (Landroid/view/View;)Ljava/lang/Object;
142-
public static final fun getShowRendering (Landroid/view/View;)Lkotlin/jvm/functions/Function2;
142+
public static final fun getShowRendering (Landroid/view/View;)Lkotlin/jvm/functions/Function3;
143143
public static final fun showRendering (Landroid/view/View;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;)V
144144
}
145145

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import com.google.common.truth.Truth.assertThat
88
import org.junit.Test
99

1010
@OptIn(WorkflowUiExperimentalApi::class)
11-
class DecorativeViewFactoryTest {
11+
internal class DecorativeViewFactoryTest {
1212

1313
private val instrumentation = InstrumentationRegistry.getInstrumentation()
1414

@@ -28,11 +28,19 @@ class DecorativeViewFactoryTest {
2828
}
2929
}
3030
}
31+
32+
val envString = object : ViewEnvironmentKey<String>(String::class) {
33+
override val default: String get() = "Not set"
34+
}
35+
3136
val outerViewFactory = DecorativeViewFactory(
3237
type = OuterRendering::class,
33-
map = { outer -> outer.wrapped },
34-
initView = { outerRendering, _ ->
35-
events += "initView $outerRendering"
38+
map = { outer, env ->
39+
val enhancedEnv = env + (envString to "Updated environment")
40+
Pair(outer.wrapped, enhancedEnv)
41+
},
42+
initView = { outerRendering, view ->
43+
events += "initView $outerRendering ${view.environment!![envString]}"
3644
}
3745
)
3846
val viewRegistry = ViewRegistry(innerViewFactory)
@@ -44,11 +52,9 @@ class DecorativeViewFactoryTest {
4452
instrumentation.context
4553
)
4654

47-
// Note that showRendering is called twice. Technically this behavior is not incorrect, although
48-
// it's not necessary. Fix coming soon.
4955
assertThat(events).containsExactly(
50-
"inner showRendering InnerRendering(innerData=inner)",
51-
"initView OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner))",
56+
"initView OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner)) " +
57+
"Updated environment",
5258
"inner showRendering InnerRendering(innerData=inner)"
5359
)
5460
}
@@ -72,9 +78,9 @@ class DecorativeViewFactoryTest {
7278
val outerViewFactory = DecorativeViewFactory(
7379
type = OuterRendering::class,
7480
map = { outer -> outer.wrapped },
75-
doShowRendering = { _, innerShowRendering, outerRendering, env ->
81+
doShowRendering = { view, innerShowRendering, outerRendering, env ->
7682
events += "doShowRendering $outerRendering"
77-
innerShowRendering(outerRendering.wrapped, env)
83+
view.innerShowRendering(outerRendering.wrapped, env)
7884
}
7985
)
8086
val viewRegistry = ViewRegistry(innerViewFactory)
@@ -86,10 +92,7 @@ class DecorativeViewFactoryTest {
8692
instrumentation.context
8793
)
8894

89-
// Note that showRendering is called twice. Technically this behavior is not incorrect, although
90-
// it's not necessary. Fix coming soon.
9195
assertThat(events).containsExactly(
92-
"inner showRendering InnerRendering(innerData=inner)",
9396
"doShowRendering OuterRendering(outerData=outer, wrapped=InnerRendering(innerData=inner))",
9497
"inner showRendering InnerRendering(innerData=inner)"
9598
)
@@ -114,9 +117,9 @@ class DecorativeViewFactoryTest {
114117
val outerViewFactory = DecorativeViewFactory(
115118
type = OuterRendering::class,
116119
map = { outer -> outer.wrapped },
117-
doShowRendering = { _, innerShowRendering, outerRendering, env ->
120+
doShowRendering = { view, innerShowRendering, outerRendering, env ->
118121
events += "doShowRendering $outerRendering"
119-
innerShowRendering(outerRendering.wrapped, env)
122+
view.innerShowRendering(outerRendering.wrapped, env)
120123
}
121124
)
122125
val viewRegistry = ViewRegistry(innerViewFactory)

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

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -116,9 +116,9 @@ public class DecorativeViewFactory<OuterT : Any, InnerT : Any>(
116116
innerShowRendering: ViewShowRendering<InnerT>,
117117
outerRendering: OuterT,
118118
env: ViewEnvironment
119-
) -> Unit = { _, innerShowRendering, outerRendering, viewEnvironment ->
119+
) -> Unit = { view, innerShowRendering, outerRendering, viewEnvironment ->
120120
val (innerRendering, processedEnv) = map(outerRendering, viewEnvironment)
121-
innerShowRendering(innerRendering, processedEnv)
121+
view.innerShowRendering(innerRendering, processedEnv)
122122
}
123123
) : ViewFactory<OuterT> {
124124
/**
@@ -133,14 +133,14 @@ public class DecorativeViewFactory<OuterT : Any, InnerT : Any>(
133133
innerShowRendering: ViewShowRendering<InnerT>,
134134
outerRendering: OuterT,
135135
env: ViewEnvironment
136-
) -> Unit = { _, innerShowRendering, outerRendering, viewEnvironment ->
137-
innerShowRendering(map(outerRendering), viewEnvironment)
136+
) -> Unit = { view, innerShowRendering, outerRendering, viewEnvironment ->
137+
view.innerShowRendering(map(outerRendering), viewEnvironment)
138138
}
139139
) : this(
140-
type,
141-
map = { outer, viewEnvironment -> Pair(map(outer), viewEnvironment) },
142-
initView = initView,
143-
doShowRendering = doShowRendering
140+
type,
141+
map = { outer, viewEnvironment -> Pair(map(outer), viewEnvironment) },
142+
initView = initView,
143+
doShowRendering = doShowRendering
144144
)
145145

146146
override fun buildView(
@@ -152,18 +152,27 @@ public class DecorativeViewFactory<OuterT : Any, InnerT : Any>(
152152
val (innerInitialRendering, processedInitialEnv) = map(initialRendering, initialViewEnvironment)
153153

154154
return processedInitialEnv[ViewRegistry]
155-
.buildView(
156-
innerInitialRendering,
157-
processedInitialEnv,
158-
contextForNewView,
159-
container
160-
)
161-
.also { view ->
162-
val innerShowRendering: ViewShowRendering<InnerT> = view.getShowRendering()!!
163-
initView(initialRendering, view)
164-
view.bindShowRendering(initialRendering, processedInitialEnv) { rendering, env ->
165-
doShowRendering(view, innerShowRendering, rendering, env)
166-
}
167-
}
155+
.buildView(
156+
innerInitialRendering,
157+
processedInitialEnv,
158+
contextForNewView,
159+
container,
160+
// Don't call showRendering yet, we need to wrap the function first.
161+
initializeView = { _, _ -> }
162+
)
163+
.also { view ->
164+
val innerShowRendering: ViewShowRendering<InnerT> = view.getShowRendering()!!
165+
view.bindShowRendering(
166+
initialRendering,
167+
processedInitialEnv
168+
) { rendering, env -> doShowRendering(view, innerShowRendering, rendering, env) }
169+
170+
// Call this after we fuss with the bindings, to ensure it can pull the updated
171+
// ViewEnvironment.
172+
initView(initialRendering, view)
173+
174+
// Our showRendering wrapper is in place, now call it.
175+
view.showRendering(initialRendering, processedInitialEnv)
176+
}
168177
}
169178
}

0 commit comments

Comments
 (0)