Skip to content

Commit 4626f96

Browse files
emilkclaude
andcommitted
keep a tile's TileId when it gets wrapped in a container
Dropping a tile onto another tile wraps them both in a new container. That wrapper used to take over the *target's* `TileId`, with the target itself moved to a freshly allocated one. So a `TileId` did not keep referring to the same tile: an id that meant "pane_a" one frame meant "the container holding pane_a" the next. That is a trap for any application that keys its own state off `TileId`s -- which Rerun does, and which `examples/tree_recreated_every_frame.rs` demonstrates. Such an app maps the id back to its own model, gets told a container now lives where a pane used to, and reuses the pane's identity for it. Next frame it builds a tree with a pane and a container at the same id, one overwrites the other, and a pane disappears. Now the wrapped tile keeps its id and the new container gets the fresh one. The wrapper also inherits what the wrapped tile had by virtue of its position: its share of a linear container's space, its cell in a grid, and whether it was the open tab. Whoever referenced the wrapped tile is re-pointed at the wrapper, and since `Tiles` does not know the root, `insert_at` hands the new container back so `Tree` can re-point the root when it was the root that got wrapped. Verified by restoring the naive version of the example's model-sync -- the one that lost panes -- and confirming the full drag sweep now passes with it. Kept the tidier version regardless, since panes never needed to be in that map. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9d2fc66 commit 4626f96

8 files changed

Lines changed: 390 additions & 86 deletions

File tree

examples/tree_recreated_every_frame.rs

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,8 @@ impl Blueprint {
117117
Node::Pane { id } => {
118118
let id_ = tile_id(id);
119119
tiles.insert(id_, Tile::Pane(id.clone()));
120-
// Deliberately NOT in `reverse`: a pane carries its own app id as its
121-
// payload, and `reverse` is only consulted for containers. Putting panes in
122-
// here is a trap -- see the note on `sync_from_tree`.
120+
// Not in `reverse`: a pane carries its own app id as its payload, and
121+
// `reverse` is only consulted for containers.
123122
id_
124123
}
125124
Node::Container { id, kind, children } => {
@@ -140,19 +139,10 @@ impl Blueprint {
140139
/// Fold an edited tree back into the blueprint, minting fresh ids for any newly-created
141140
/// containers (tiles whose id isn't in `reverse`) — exactly what Rerun does on a drop.
142141
///
143-
/// ## Watch out
144-
///
145-
/// `reverse` maps a `TileId` back to an app id, and it deliberately contains **containers
146-
/// only**. Panes are looked up from their own payload instead.
147-
///
148-
/// That matters because a `TileId` does not keep referring to the same kind of tile. Drop a
149-
/// pane onto another pane and `egui_tiles` reuses the *target's* id for the new container it
150-
/// wraps them both in, moving the target pane itself to a freshly allocated id. So a tile id
151-
/// that meant `"pane_a"` one frame can mean `"the container holding pane_a"` the next.
152-
///
153-
/// If panes were in `reverse`, that new container would be handed the app id `"pane_a"`, and
154-
/// the next `to_tree()` would insert both a pane and a container at `tile_id("pane_a")` — the
155-
/// second overwriting the first, silently losing a pane.
142+
/// `reverse` maps a `TileId` back to an app id, and holds **containers only** — a pane is
143+
/// identified by its own payload instead. A container appearing somewhere the app has never
144+
/// seen before therefore gets a freshly minted id, which is exactly what should happen when
145+
/// `egui_tiles` wraps tiles in a new container on drop.
156146
fn sync_from_tree(&mut self, tree: &Tree<String>, reverse: &HashMap<TileId, String>) {
157147
fn rebuild(
158148
tile_id: TileId,

src/container/grid.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,17 @@ impl Grid {
111111

112112
/// Returns the child already at the given index, if any.
113113
#[must_use]
114+
/// Swap out one child for another, keeping its cell.
115+
///
116+
/// The column and row shares are positional, so they need no fixing up.
117+
pub(super) fn replace_child(&mut self, old: TileId, new: TileId) -> bool {
118+
let Some(slot) = self.children.iter_mut().find(|child| **child == Some(old)) else {
119+
return false;
120+
};
121+
*slot = Some(new);
122+
true
123+
}
124+
114125
pub fn replace_at(&mut self, index: usize, child: TileId) -> Option<TileId> {
115126
if let Some(slot) = self.children.get_mut(index) {
116127
slot.replace(child)

src/container/linear.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,16 @@ impl Linear {
144144
slf
145145
}
146146

147+
/// Swap out one child for another, keeping its position and its share of the space.
148+
pub(super) fn replace_child(&mut self, old: TileId, new: TileId) -> bool {
149+
let Some(slot) = self.children.iter_mut().find(|child| **child == old) else {
150+
return false;
151+
};
152+
*slot = new;
153+
self.shares.replace_with(old, new);
154+
true
155+
}
156+
147157
pub fn add_child(&mut self, child: TileId) {
148158
self.children.push(child);
149159
}

src/container/mod.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,17 @@ impl Container {
176176
}
177177
}
178178

179+
/// Swap out one child for another, keeping its place and its share of the space.
180+
///
181+
/// Returns `false` if `old` was not a child of this container.
182+
pub fn replace_child(&mut self, old: TileId, new: TileId) -> bool {
183+
match self {
184+
Self::Tabs(tabs) => tabs.replace_child(old, new),
185+
Self::Linear(linear) => linear.replace_child(old, new),
186+
Self::Grid(grid) => grid.replace_child(old, new),
187+
}
188+
}
189+
179190
/// Returns child index, if found.
180191
pub fn remove_child(&mut self, child: TileId) -> Option<usize> {
181192
match self {

src/container/tabs.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,18 @@ impl Tabs {
152152
self.children.push(child);
153153
}
154154

155+
/// Swap out one tab for another, keeping its position and whether it was the open one.
156+
pub(super) fn replace_child(&mut self, old: TileId, new: TileId) -> bool {
157+
let Some(slot) = self.children.iter_mut().find(|child| **child == old) else {
158+
return false;
159+
};
160+
*slot = new;
161+
if self.active == Some(old) {
162+
self.active = Some(new);
163+
}
164+
true
165+
}
166+
155167
pub fn set_active(&mut self, child: TileId) {
156168
self.active = Some(child);
157169
}

0 commit comments

Comments
 (0)