Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update re-attached widget removes to use detach #2510

Merged
merged 3 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Fixed:
- Add support for the Height modifier in `ComposeUiBox`.
- Add support for the Width modifier in `ComposeUiBox`.
- Call `DisposableEffect` when a screen is unbound. We were only calling these when the effect was removed from the composition.
- Support `movableContentOf` in Treehouse (and generally in the Redwood protocol). Note: this requires the host be running version 0.17.0 or newer.


## [0.16.0] - 2024-11-19
Expand Down
3 changes: 2 additions & 1 deletion redwood-protocol-guest/api/redwood-protocol-guest.api
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public final class app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter :
public fun appendPropertyChange-13Ob0Yo (IIILkotlinx/serialization/KSerializer;Ljava/lang/Object;)V
public fun appendPropertyChange-ITsWdOQ (IIIZ)V
public fun appendPropertyChange-hzhmVHk (IIII)V
public fun appendRemove-hx81-Ec (IIII)V
public fun appendRemove-ARs5Qwk (IIILapp/cash/redwood/protocol/guest/ProtocolWidget;)V
public fun emitChanges ()V
public fun getJson ()Lkotlinx/serialization/json/Json;
public fun getRoot ()Lapp/cash/redwood/widget/Widget$Children;
Expand All @@ -24,6 +24,7 @@ public abstract class app/cash/redwood/protocol/guest/GuestProtocolAdapter : app
public static final field $stable I
public synthetic fun <init> (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
public abstract fun emitChanges ()V
protected final fun getHostVersion-7jYel6c ()Ljava/lang/String;
public abstract fun getRoot ()Lapp/cash/redwood/widget/Widget$Children;
public abstract fun getWidgetSystem ()Lapp/cash/redwood/widget/WidgetSystem;
public abstract fun initChangesSink (Lapp/cash/redwood/protocol/ChangesSink;)V
Expand Down
4 changes: 3 additions & 1 deletion redwood-protocol-guest/api/redwood-protocol-guest.klib.api
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ abstract class app.cash.redwood.protocol.guest/GuestProtocolAdapter : app.cash.r
abstract fun <get-root>(): app.cash.redwood.widget/Widget.Children<kotlin/Unit> // app.cash.redwood.protocol.guest/GuestProtocolAdapter.root.<get-root>|<get-root>(){}[0]
abstract val widgetSystem // app.cash.redwood.protocol.guest/GuestProtocolAdapter.widgetSystem|{}widgetSystem[0]
abstract fun <get-widgetSystem>(): app.cash.redwood.widget/WidgetSystem<kotlin/Unit> // app.cash.redwood.protocol.guest/GuestProtocolAdapter.widgetSystem.<get-widgetSystem>|<get-widgetSystem>(){}[0]
final val hostVersion // app.cash.redwood.protocol.guest/GuestProtocolAdapter.hostVersion|{}hostVersion[0]
final fun <get-hostVersion>(): app.cash.redwood.protocol/RedwoodVersion // app.cash.redwood.protocol.guest/GuestProtocolAdapter.hostVersion.<get-hostVersion>|<get-hostVersion>(){}[0]

abstract fun emitChanges() // app.cash.redwood.protocol.guest/GuestProtocolAdapter.emitChanges|emitChanges(){}[0]
abstract fun initChangesSink(app.cash.redwood.protocol/ChangesSink) // app.cash.redwood.protocol.guest/GuestProtocolAdapter.initChangesSink|initChangesSink(app.cash.redwood.protocol.ChangesSink){}[0]
Expand All @@ -49,7 +51,7 @@ final class app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter : app.ca
final fun appendMove(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/ChildrenTag, kotlin/Int, kotlin/Int, kotlin/Int) // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.appendMove|appendMove(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.ChildrenTag;kotlin.Int;kotlin.Int;kotlin.Int){}[0]
final fun appendPropertyChange(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/WidgetTag, app.cash.redwood.protocol/PropertyTag, kotlin/Boolean) // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.appendPropertyChange|appendPropertyChange(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.WidgetTag;app.cash.redwood.protocol.PropertyTag;kotlin.Boolean){}[0]
final fun appendPropertyChange(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/WidgetTag, app.cash.redwood.protocol/PropertyTag, kotlin/UInt) // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.appendPropertyChange|appendPropertyChange(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.WidgetTag;app.cash.redwood.protocol.PropertyTag;kotlin.UInt){}[0]
final fun appendRemove(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/ChildrenTag, kotlin/Int, app.cash.redwood.protocol/Id) // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.appendRemove|appendRemove(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.ChildrenTag;kotlin.Int;app.cash.redwood.protocol.Id){}[0]
final fun appendRemove(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/ChildrenTag, kotlin/Int, app.cash.redwood.protocol.guest/ProtocolWidget) // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.appendRemove|appendRemove(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.ChildrenTag;kotlin.Int;app.cash.redwood.protocol.guest.ProtocolWidget){}[0]
final fun emitChanges() // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.emitChanges|emitChanges(){}[0]
final fun initChangesSink(app.cash.redwood.protocol/ChangesSink) // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.initChangesSink|initChangesSink(app.cash.redwood.protocol.ChangesSink){}[0]
final fun nextId(): app.cash.redwood.protocol/Id // app.cash.redwood.protocol.guest/DefaultGuestProtocolAdapter.nextId|nextId(){}[0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import app.cash.redwood.protocol.PropertyChange
import app.cash.redwood.protocol.PropertyTag
import app.cash.redwood.protocol.RedwoodVersion
import app.cash.redwood.protocol.WidgetTag
import app.cash.redwood.protocol.guest.ProtocolWidget.Companion.INVALID_INDEX
import app.cash.redwood.widget.Widget
import app.cash.redwood.widget.WidgetSystem
import kotlinx.serialization.ExperimentalSerializationApi
Expand Down Expand Up @@ -136,9 +137,18 @@ public class DefaultGuestProtocolAdapter(
index: Int,
child: ProtocolWidget,
) {
val replaced = widgets.put(child.id.value, child)
check(replaced == null) {
"Attempted to add widget with ID ${child.id} but one already exists"
val childId = child.id.value
if (child.removeIndex != INVALID_INDEX) {
check(hostSupportsRemoveDetach) { "Host v$hostVersion does not support widget re-attach" }
check(childId in widgets) { "Attempted to re-attach unknown widget with ID $childId" }
removed.remove(childId)

// Update the remove change to indicate it's only a detach.
val remove = changes[child.removeIndex] as ChildrenChange.Remove
changes[child.removeIndex] = ChildrenChange.Remove(remove.id, remove.tag, remove.index, true)
JakeWharton marked this conversation as resolved.
Show resolved Hide resolved
JakeWharton marked this conversation as resolved.
Show resolved Hide resolved
} else {
val replaced = widgets.put(childId, child)
check(replaced == null) { "Attempted to add widget with existing ID $childId" }
}
changes.add(ChildrenChange.Add(id, tag, child.id, index))
}
Expand All @@ -157,9 +167,10 @@ public class DefaultGuestProtocolAdapter(
id: Id,
tag: ChildrenTag,
index: Int,
childId: Id,
child: ProtocolWidget,
) {
removed += childId.value
removed += child.id.value
child.removeIndex = changes.size
changes.add(ChildrenChange.Remove(id, tag, index, detach = false))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,20 +38,19 @@ import kotlinx.serialization.json.Json
* This interface is for generated code use only.
*/
public abstract class GuestProtocolAdapter(
hostVersion: RedwoodVersion,
protected val hostVersion: RedwoodVersion,
) : EventSink {
@RedwoodCodegenApi
public abstract val json: Json

/**
* Whether the host supports the detach parameter on
* [app.cash.redwood.protocol.ChildrenChange.Remove]. This also implies that the `count`
* parameter does not need to be sent.
* [app.cash.redwood.protocol.ChildrenChange.Remove].
*
* TODO Remove when minimum-supported host version is 1.17 or higher.
* TODO Remove when minimum-supported host version is 0.17 or higher.
*/
@RedwoodCodegenApi
protected val hostSupportsRemoveDetachAndNoCount: Boolean = hostVersion >= RedwoodVersion("0.17.0-SNAPSHOT")
protected val hostSupportsRemoveDetach: Boolean = hostVersion >= RedwoodVersion("0.17.0-SNAPSHOT")

/**
* The provider of factories of widgets which record property changes and whose children changes
Expand Down Expand Up @@ -143,6 +142,6 @@ public abstract class GuestProtocolAdapter(
id: Id,
tag: ChildrenTag,
index: Int,
childId: Id,
child: ProtocolWidget,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ public interface ProtocolWidget : Widget<Unit> {
public val id: Id
public val tag: WidgetTag

/**
* Used to detect when a removed widget is re-added later in the frame, such as when
* `movableContentOf` is used. When a widget is added with this value greater than or equal to 0,
* the original 'remove' operation should be changed to indicate that this node
* (and its entire subtree) should be retained host-side, as a future 'add' of this node
* will occur without associated 'create' events for the subtree.
*/
public var removeIndex: Int

override val value: Unit get() = Unit

public fun sendEvent(event: Event)
Expand Down Expand Up @@ -73,4 +82,9 @@ public interface ProtocolWidget : Widget<Unit> {
children: ProtocolWidgetChildren,
)
}

@RedwoodCodegenApi // https://github.com/Kotlin/binary-compatibility-validator/issues/91
public companion object {
public const val INVALID_INDEX: Int = -1
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ public class ProtocolWidgetChildren(

override fun remove(index: Int, count: Int) {
for (i in index + count - 1 downTo index) {
val widget = widgets[i]
guestAdapter.appendRemove(id, tag, index, widget.id)
guestAdapter.appendRemove(id, tag, index, widgets[i])
}
_widgets.remove(index, count)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,14 @@ package app.cash.redwood.protocol.guest

import androidx.compose.runtime.BroadcastFrameClock
import androidx.compose.runtime.getValue
import androidx.compose.runtime.movableContentOf
import androidx.compose.runtime.mutableIntStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import app.cash.redwood.compose.WidgetVersion
import app.cash.redwood.layout.compose.Box
import app.cash.redwood.layout.compose.Column
import app.cash.redwood.layout.compose.Row
import app.cash.redwood.protocol.Change
import app.cash.redwood.protocol.ChildrenChange
import app.cash.redwood.protocol.ChildrenTag
Expand All @@ -31,13 +36,15 @@ import app.cash.redwood.protocol.ModifierChange
import app.cash.redwood.protocol.PropertyChange
import app.cash.redwood.protocol.PropertyTag
import app.cash.redwood.protocol.RedwoodVersion
import app.cash.redwood.protocol.ValueChange
import app.cash.redwood.protocol.WidgetTag
import app.cash.redwood.testing.TestRedwoodComposition
import app.cash.redwood.ui.Cancellable
import app.cash.redwood.ui.OnBackPressedCallback
import app.cash.redwood.ui.OnBackPressedDispatcher
import app.cash.redwood.ui.UiConfiguration
import assertk.assertThat
import assertk.assertions.containsExactly
import assertk.assertions.isEqualTo
import com.example.redwood.testapp.compose.Button
import com.example.redwood.testapp.compose.Button2
Expand Down Expand Up @@ -288,6 +295,171 @@ class ProtocolTest {
)
}

@Test fun movableContentSameRecomposition() = runTest {
val (composition) = testProtocolComposition()

var state by mutableIntStateOf(0)
composition.setContent {
val oneTwoThree = remember {
movableContentOf {
Box {
Text("one")
Text("two")
Text("three")
}
}
}
when (state) {
0 -> Row { oneTwoThree() }
1 -> Column { oneTwoThree() }
}
}

assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful!

// Row to Root
Create(Id(1), WidgetTag(1000001)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(1), 0),
// Box
Create(Id(2), WidgetTag(1000004)),
// Text("one") to Box
Create(Id(3), WidgetTag(3)),
ChildrenChange.Add(Id(2), ChildrenTag(1), Id(3), 0),
// Text("two") to Box
Create(Id(4), WidgetTag(3)),
ChildrenChange.Add(Id(2), ChildrenTag(1), Id(4), 1),
// Text("three") to Box
Create(Id(5), WidgetTag(3)),
ChildrenChange.Add(Id(2), ChildrenTag(1), Id(5), 2),
// Box to Row
ChildrenChange.Add(Id(1), ChildrenTag(1), Id(2), 0),
)

state = 1
assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
Copy link
Contributor

Choose a reason for hiding this comment

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

The ValueChange instances all target the new Column here, right?

// Box from Row
ChildrenChange.Remove(Id(1), ChildrenTag(1), 0, detach = true),
Copy link
Contributor

Choose a reason for hiding this comment

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

YASS

// Row from Root
ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, detach = false),
// Column to Root
Create(Id(6), WidgetTag(1000002)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(6), 0),
// Box to Column
ChildrenChange.Add(Id(6), ChildrenTag(1), Id(2), 0),
)
}

@Test fun multipleMovableContentButOnlyOneReused() = runTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

So cool

val (composition) = testProtocolComposition()

var state by mutableIntStateOf(0)
composition.setContent {
val one = remember { movableContentOf { Text("one") } }
val two = remember { movableContentOf { Text("two") } }
val three = remember { movableContentOf { Text("three") } }
when (state) {
0 -> {
one()
two()
three()
}
1 -> {
two()
}
}
}

assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
Create(Id(1), WidgetTag(3)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(1), 0),
Create(Id(2), WidgetTag(3)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(2), 1),
Create(Id(3), WidgetTag(3)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(3), 2),
)

state = 1
assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, detach = false),
ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, detach = true),
ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, detach = false),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(2), 0),
)
}

@Test fun movableContentMultipleRecompositions() = runTest {
val (composition) = testProtocolComposition()

var state by mutableIntStateOf(0)
composition.setContent {
val oneTwoThree = remember {
movableContentOf {
Box {
Text("one")
Text("two")
Text("three")
}
}
}
when (state) {
0 -> Row { oneTwoThree() }
1 -> Text("hey")
2 -> Column { oneTwoThree() }
}
}

assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
// Row to Root
Create(Id(1), WidgetTag(1000001)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(1), 0),
// Box
Create(Id(2), WidgetTag(1000004)),
// Text("one") to Box
Create(Id(3), WidgetTag(3)),
ChildrenChange.Add(Id(2), ChildrenTag(1), Id(3), 0),
// Text("two") to Box
Create(Id(4), WidgetTag(3)),
ChildrenChange.Add(Id(2), ChildrenTag(1), Id(4), 1),
// Text("three") to Box
Create(Id(5), WidgetTag(3)),
ChildrenChange.Add(Id(2), ChildrenTag(1), Id(5), 2),
// Box to Row
ChildrenChange.Add(Id(1), ChildrenTag(1), Id(2), 0),
)

state = 1
assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
// Box from Row
ChildrenChange.Remove(Id(1), ChildrenTag(1), 0, detach = false),
// Row from Root
ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, detach = false),
// Text("hey") to Root
Create(Id(6), WidgetTag(3)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(6), 0),
)

state = 2
assertThat(composition.awaitSnapshot().filter { it !is ValueChange }).containsExactly(
// Text from Root
ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, detach = false),
// Column to Root
Create(Id(7), WidgetTag(1000002)),
ChildrenChange.Add(Id.Root, ChildrenTag.Root, Id(7), 0),
// Box
Create(Id(8), WidgetTag(1000004)),
// Text("one") to Box
Create(Id(9), WidgetTag(3)),
ChildrenChange.Add(Id(8), ChildrenTag(1), Id(9), 0),
// Text("two") to Box
Create(Id(10), WidgetTag(3)),
ChildrenChange.Add(Id(8), ChildrenTag(1), Id(10), 1),
// Text("three") to Box
Create(Id(11), WidgetTag(3)),
ChildrenChange.Add(Id(8), ChildrenTag(1), Id(11), 2),
// Box to Column
ChildrenChange.Add(Id(7), ChildrenTag(1), Id(8), 0),
)
}

private fun TestScope.testProtocolComposition(
hostVersion: RedwoodVersion = latestVersion,
): Pair<TestRedwoodComposition<List<Change>>, GuestProtocolAdapter> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ public value class RedwoodVersion(public val value: String) : Comparable<Redwood
}
}

override fun toString(): String = value

/**
* Compare two versions to see which is newer.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ internal class ProtocolButton(
) : ProtocolWidget, Button<Unit> {
public override val id: Id = guestAdapter.nextId()
public override val tag: WidgetTag get() = WidgetTag(3)
public override var removeIndex: Int = INVALID_INDEX

private var onClick: (() -> Unit)? = null

Expand Down Expand Up @@ -274,6 +275,12 @@ internal fun generateProtocolWidget(
)
.build(),
)
.addProperty(
PropertySpec.builder("removeIndex", INT, OVERRIDE)
.mutable(true)
.initializer("%T.INVALID_INDEX", ProtocolGuest.ProtocolWidget)
.build(),
)
.apply {
var nextSerializerId = 0
val serializerIds = mutableMapOf<TypeName, Int>()
Expand Down
Loading
Loading