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.

Sign up for GitHub

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

Switch back to MutableMap to avoid scatter map bug #2438

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Switch back to MutableMap to avoid scatter map bug #2438

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 10 additions & 9 deletions ...protocol-host/src/commonMain/kotlin/app/cash/redwood/protocol/host/HostProtocolAdapter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import app.cash.redwood.protocol.RedwoodVersion
import app.cash.redwood.protocol.WidgetTag
import app.cash.redwood.widget.ChangeListener
import app.cash.redwood.widget.Widget
import kotlin.collections.mutableMapOf
import kotlin.native.ObjCName

/**
Expand All @@ -60,8 +61,9 @@ public class HostProtocolAdapter<W : Any>(
is GeneratedHostProtocol -> factory
}

// TODO Use MutableIntObjectMap once https://issuetracker.google.com/issues/378077858 is fixed.
private val nodes =
mutableIntObjectMapOf<ProtocolNode<W>>(Id.Root.value, RootProtocolNode(container))
mutableMapOf<Int, ProtocolNode<W>>(Id.Root.value to RootProtocolNode(container))

private val removeNodeById = IdVisitor { nodes.remove(it.value) }

Expand Down Expand Up @@ -173,7 +175,7 @@ public class HostProtocolAdapter<W : Any>(
public fun close() {
closed = true

nodes.forEachValue { node ->
nodes.values.forEach { node ->
node.detach()
}
nodes.clear()
Expand Down Expand Up @@ -235,9 +237,10 @@ public class HostProtocolAdapter<W : Any>(
// Must have a Create node that precedes it.
if (lastCreatedId != change.id) continue

idToNode[change.id.value] = ReuseNode(
widgetId = change.id,
childrenTag = ChildrenTag.Root,
idToNode[lastCreatedId.value] = ReuseNode(
widgetId = lastCreatedId,
Comment on lines +240 to +241
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switch to local which already has the same value as change.id

// This is the root of the reuse tree, so it will never have a children tag from a parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

childrenTag = ChildrenTag(-1),
)
}

Expand Down Expand Up @@ -359,19 +362,17 @@ public class HostProtocolAdapter<W : Any>(
* descendants into the nodes map.
*/
fun assignPooledNodeRecursive(
nodes: MutableIntObjectMap<ProtocolNode<W>>,
nodes: MutableMap<Int, ProtocolNode<W>>,
changesAndNulls: Array<Change?>,
pooled: ProtocolNode<W>,
) {
// Reuse the node.
pooled.id = widgetId
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to lookup the Create, we already have a member with the new ID.

val old = nodes.put(widgetId.value, pooled)
require(old == null) {
"Insert attempted to replace existing widget with ID ${widgetId.value}"
}

val skippedCreate = changesAndNulls[changeIndexForCreate] as Create
pooled.id = skippedCreate.id

// Remove the corresponding changes that we avoided by node reuse. We don't clear the 'Add'
// that adds the node to its new parent.
changesAndNulls[changeIndexForCreate] = null
Expand Down