Animated layout preview on tile drag-and-drop (#118 + more) - #139
Draft
emilk wants to merge 50 commits into
Draft
Conversation
The animated layout preview needs state that spans several frames: the speculative layout uses the *previous* frame's insertion point, and the tile rects are smoothed over time. Keeping that state in a `Tree` field breaks any application that re-creates its `Tree` from its own source of truth every frame (Rerun does exactly this). There, `preview.insertion` was always `None` by the time `compute_preview_rects` read it, so no preview rects were ever produced, nothing was ever animated, and the feature silently degraded to the old highlighted drop zone. Store it in `egui::Memory` keyed by the tree id instead, like `smooth_preview_rect` already does. The `Tree::preview` field remains as a frame-local scratch copy, loaded at the start of `Tree::ui` and stored back at the end, so the `&self` accessors used by the container ui code are unchanged. Adds a test that drives a real `egui::Context` through a drag while re-creating the `Tree` inside every frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adapts to egui 0.35, where `Panel::show_inside` was renamed back to `show`.
The animated drag preview used to work out the post-drop layout by applying the move to the live tree, laying it out, and then restoring the tree from a snapshot. The restore was driven by a journal of relocations recorded in `Tiles::insert_at`, but that was not the only place that relocates a tile to a fresh id: `make_all_panes_children_of_tabs` and `flatten_tabs_in_tabs` do too. With `all_panes_must_have_tabs` set (as Rerun does), merely *hovering* a drag over a pane destroyed it -- no drop required. Speculation now runs against `Tiles::skeleton()`: a structural copy in which every pane is replaced by its own `TileId`. The real tree is only ever borrowed, so no bug in the speculative pass can lose a pane. The pane payload doubles as an identity token, so a pane stays identifiable however many times the speculative edits relocate it, and a `debug_assert!` compares the pane sets before and after to catch any future relocation site. This is possible because the layout pass never looks at pane contents. The four numbers it does need are gathered up front into a pane-agnostic `LayoutContext`, which also means `Tabs::layout` no longer fires `Behavior::on_edit` from deep inside the recursion -- it reports the auto-selection back to the caller, so a speculative layout cannot emit user-visible edit events. Falling out of the above: - The state kept in `egui::Memory` shrinks from six fields to two: the pending insertion point and the smoothed rects. Everything else is derived fresh each frame and so cannot go stale. - `MoveJournal`, `lerp_t` (a tri-state `f32` never read as a number), the `next_tile_id` save/restore accessors, the four restore loops and the `pointer_released` guard are all gone. - `Tiles::layout_tile` loses two parameters. - `Linear::layout` is no longer `pub`; it could not be called usefully from outside the crate. Tests go from 2 to 7. `hovering_a_drag_never_touches_the_real_tree` sweeps 75 pointer positions across two simplification configs, asserting the tree is byte-identical throughout, and asserts it actually speculated on most of those frames so it cannot pass vacuously. Verified that it fails on the previous implementation at (450, 500) with `all_panes_must_have_tabs`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The layout pass never looks at a pane's contents. All it needs from `Behavior` are four numbers: `gap_width`, `tab_bar_height`, `min_size` (via `grid_auto_column_count`) and the column-count heuristic itself. Gathering those up front into a `LayoutContext` drops both the `style` and `behavior` arguments from every layout signature, and means the layout code no longer mentions `Pane` at all -- so the same code can lay out a `Tiles` of any pane type, not only the one the `Behavior` was written for. Also fixes a small wart: `Tabs::layout` called `Behavior::on_edit` from deep inside the layout recursion. It now reports the auto-selection up through the `LayoutContext` and the single caller emits the event, so laying out a tree is no longer a way to trigger user-visible callbacks. `Linear::layout` becomes `pub(super)`, matching the other containers. It took `&mut Tiles` and a `&mut dyn Behavior`, so it was not callable in practice. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rerun (and apps like it) do not treat the `Tree` as the source of truth. They keep their own layout model, build a fresh `Tree` from it every frame with `TileId`s derived from their own stable ids, and fold any edit back out of the tree afterwards. That pattern puts real constraints on `egui_tiles`: any state that has to outlive a frame cannot live in the `Tree`, and anything that edits the tree mid-frame and expects to restore it has to cope with the tree being discarded and rebuilt underneath it. Neither constraint is obvious from the other examples, both are easy to break, and breaking them tends to show up as a lost pane rather than a compile error. So: an example that does it, with a live pane counter that must not change while you drag things around, plus a test pinning the same invariant down in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md # examples/tree_recreated_every_frame.rs # src/behavior.rs # src/tree.rs
This was referenced Jul 27, 2026
emilk
added a commit
that referenced
this pull request
Jul 27, 2026
Pure refactor, extracted from #139. No behavior change. The layout pass never looks at a pane's contents. All it needs from `Behavior` are a few numbers, so this gathers them up front into a `LayoutContext`: ```rust pub(crate) struct LayoutContext<'a> { pub gap_width: f32, pub tab_bar_height: f32, pub grid_auto_column_count: &'a dyn Fn(usize, Rect, f32) -> usize, pub tab_auto_selected: &'a Cell<bool>, } ``` Two consequences: * Every layout signature loses both its `style` and `behavior` arguments — `Tiles::layout_tile` goes from 5 parameters to 3. * The layout code no longer mentions `Pane` at all, so it can lay out a `Tiles<T>` for any `T`, not only the one the `Behavior` was written for. #139 needs exactly that. ### Drive-by fix `Tabs::layout` called `Behavior::on_edit(TabSelected)` from deep inside the layout recursion. It now reports the auto-selection up through the `LayoutContext` and the single caller emits the event. Same event, same conditions — but laying out a tree is no longer a way to trigger user-visible callbacks, which matters once anything wants to lay out a tree speculatively. ### Breaking⚠️ `Linear::layout` is no longer `pub`, matching the other containers. It took `&mut Tiles` and a `&mut dyn Behavior`, so it was not callable in practice. ### Testing `cargo fmt --check`, `cargo clippy --all-features --all-targets` and `cargo test --all-features` all clean. No new tests: this is intended to be behavior-preserving, and the existing `test_grid_with_chaos_monkey` exercises the layout paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The default `Behavior::tab_ui` draws a tab with a bare `Ui::interact` plus manual painting. That means the tab has no accessibility information at all: a screen reader sees an unnamed, untyped blob where a tab should be, with no way to tell which one is selected. Report it as a button carrying the tab's title and its selected state. Same for the close button, which gets a "Close" label. This is done outside the `is_rect_visible` check, since a tab scrolled out of the tab bar is still a tab. Also makes tabs reachable by name from `egui_kittest`, which is what led me here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The layout pass never looks at a pane's contents. All it needs from `Behavior` are four numbers: `gap_width`, `tab_bar_height`, `min_size` (via `grid_auto_column_count`) and the column-count heuristic itself. Gathering those up front into a `LayoutContext` drops both the `style` and `behavior` arguments from every layout signature, and means the layout code no longer mentions `Pane` at all -- so the same code can lay out a `Tiles` of any pane type, not only the one the `Behavior` was written for. Also fixes a small wart: `Tabs::layout` called `Behavior::on_edit` from deep inside the layout recursion. It now reports the auto-selection up through the `LayoutContext` and the single caller emits the event, so laying out a tree is no longer a way to trigger user-visible callbacks. `Linear::layout` becomes `pub(super)`, matching the other containers. It took `&mut Tiles` and a `&mut dyn Behavior`, so it was not callable in practice. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rerun (and apps like it) do not treat the `Tree` as the source of truth. They keep their own layout model, build a fresh `Tree` from it every frame with `TileId`s derived from their own stable ids, and fold any edit back out of the tree afterwards. That pattern puts real constraints on `egui_tiles`: any state that has to outlive a frame cannot live in the `Tree`, and anything that edits the tree mid-frame and expects to restore it has to cope with the tree being discarded and rebuilt underneath it. Neither constraint is obvious from the other examples, both are easy to break, and breaking them tends to show up as a lost pane rather than a compile error. So: an example that does it, with a live pane counter that must not change while you drag panes around, plus an `egui_kittest` test pinning the same invariant down in CI. The layout is `Horizontal[ pane_a, Vertical[ pane_b, Tabs[ pane_c, pane_d ] ] ]`, so the test's three drops land somewhere structurally different each time: a pane in a sibling container, a tab bar, and an open tab. The test drives the drag through the real widgets -- press the pane's drag handle, move the pointer, release -- rather than by poking internals, so it exercises the same path a user takes. It counts committed `TileDropped` edits rather than comparing layouts, because a real drop can still leave the shape unchanged: dropping onto the tab bar wraps the column in a container that simplification then unwraps again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md # examples/tree_recreated_every_frame.rs # src/behavior.rs # src/tree.rs
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>
# Conflicts: # CHANGELOG.md # src/tiles.rs # src/tree.rs
emilk
changed the base branch from
emilk/tree-recreated-example
to
emilk/stable-tile-ids-on-wrap
July 27, 2026 18:08
The default `Behavior::tab_ui` draws a tab with a bare `Ui::interact` plus manual painting. That means the tab has no accessibility information at all: a screen reader sees an unnamed, untyped blob where a tab should be, with no way to tell which one is selected. Report it as a button carrying the tab's title and its selected state. Same for the close button, which gets a "Close" label. This is done outside the `is_rect_visible` check, since a tab scrolled out of the tab bar is still a tab. Also makes tabs reachable by name from `egui_kittest`, which is what led me here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rerun (and apps like it) do not treat the `Tree` as the source of truth. They keep their own layout model, build a fresh `Tree` from it every frame with `TileId`s derived from their own stable ids, and fold any edit back out of the tree afterwards. That pattern puts real constraints on `egui_tiles`: any state that has to outlive a frame cannot live in the `Tree`, and anything that edits the tree mid-frame and expects to restore it has to cope with the tree being discarded and rebuilt underneath it. Neither constraint is obvious from the other examples, both are easy to break, and breaking them tends to show up as a lost pane rather than a compile error. So: an example that does it, with a live pane counter that must not change while you drag panes around, plus an `egui_kittest` test pinning the same invariant down in CI. The layout is `Horizontal[ pane_a, Vertical[ pane_b, Tabs[ pane_c, pane_d ] ] ]`, so the test's three drops land somewhere structurally different each time: a pane in a sibling container, a tab bar, and an open tab. The test drives the drag through the real widgets -- press the pane's drag handle, move the pointer, release -- rather than by poking internals, so it exercises the same path a user takes. It counts committed `TileDropped` edits rather than comparing layouts, because a real drop can still leave the shape unchanged: dropping onto the tab bar wraps the column in a container that simplification then unwraps again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dragging a pane onto another pane in the example made the *target* pane vanish:
Horizontal[pane_a, Vertical[pane_b, Tabs[pane_c, pane_d]]]
-> Horizontal[pane_b, Tabs[pane_c, pane_d]] # pane_a gone
A `TileId` does not keep referring to the same kind of tile. When you drop onto a
pane, `Tiles::insert_at` reuses the *target's* id for the new container it wraps
them both in, and moves the target pane itself to a freshly allocated id. So an
id that meant "pane_a" one frame means "the container holding pane_a" the next.
The example put panes in its `TileId -> app id` map, so that new container was
handed the app id "pane_a". The next `to_tree()` then inserted both a pane and a
container at `tile_id("pane_a")`, the second overwriting the first.
Panes carry their app id as their payload, so they never needed to be in that map
at all. Now only containers are, and a container appearing where a pane used to
be correctly gets a freshly minted id.
This is a sharp edge worth knowing about for anyone driving egui_tiles this way,
so it is now written up on `Blueprint::sync_from_tree`.
The two tests are also much stronger than what they replace. They cover every
way of dropping one pane onto another -- aiming both at the pane and at its tab
-- both on a fresh tree and on one long-lived app where each drop's leftovers
feed into the next. Each drag additionally asserts that a drop really was
committed, so a pane-count check can never pass by quietly doing nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
# Conflicts: # src/tree.rs
emilk
force-pushed
the
emilk/stable-tile-ids-on-wrap
branch
from
July 27, 2026 18:22
a6316b4 to
4626f96
Compare
emilk
added a commit
that referenced
this pull request
Jul 27, 2026
Extracted from #139. Example + test only, no library changes. Stacked on #143. Read [this PR's own commits](https://github.com/rerun-io/egui_tiles/pull/142/commits) for just the new material. ### Why Rerun (and apps like it) don't treat the `Tree` as the source of truth. They keep their own layout model, build a fresh `Tree` from it every frame with `TileId`s derived from their own stable ids, and fold any edit back out of the tree afterwards. That puts constraints on `egui_tiles` that none of the existing examples show, and they are easy to get wrong in ways that silently lose a pane rather than failing to compile. ### What **`examples/tree_recreated_every_frame.rs`** — a runnable app in that shape, with a live pane counter in the top bar that must not change while you drag things around: ``` Horizontal[ pane_a, Vertical[ pane_b, Tabs[ pane_c, pane_d ] ] ] ``` **`tests/tree_recreated_every_frame.rs`** — the same setup driven headlessly by `egui_kittest`, covering every way of dropping one pane onto another (aiming both at the pane and at its tab), both on a fresh tree and on one long-lived app where each drop's leftovers feed into the next. ### A sharp edge this turned up The first version of the example lost the pane you dropped *onto*: ``` Horizontal[pane_a, Vertical[pane_b, Tabs[pane_c, pane_d]]] -> Horizontal[pane_b, Tabs[pane_c, pane_d]] # pane_a gone ``` **A `TileId` does not keep referring to the same kind of tile.** When you drop onto a pane, `Tiles::insert_at` reuses the *target's* id for the new container it wraps them both in, and moves the target pane itself to a freshly allocated id. So an id that meant `"pane_a"` one frame means "the container holding `pane_a`" the next. The example was putting panes in its `TileId -> app id` map, so that new container got handed the app id `"pane_a"`, and the next `to_tree()` inserted both a pane and a container at `tile_id("pane_a")` — the second overwriting the first. Panes carry their app id as their payload and never needed to be in that map. Now only containers are, and this is written up on `Blueprint::sync_from_tree`, since anyone driving `egui_tiles` this way can hit it. Arguably `egui_tiles` should not reuse the target's id like this — see the follow-up noted at the bottom of #139. That is a breaking change to tile identity across a drop, so it wants its own PR; this one just makes the example correct and documents the trap. ### Notes on the tests * The drag goes through the **real widgets** — find the pane's drag handle by its accessibility label, press it, move the pointer, release — so it exercises the path a user takes and needs nothing beyond the public API. Finding a *tab* by name is what #143 makes possible. * Each drag asserts that a drop really was committed, so a pane-count check can never pass by quietly doing nothing. Verified by aiming a drag at empty space: `committed no drop`. * Skipped targets (an inactive tab's contents, say) are counted rather than silently ignored. ### Testing `cargo fmt --check`, `cargo clippy --all-features --all-targets`, `cargo test --all-features` all clean. Adds `egui_kittest` as a dev-dependency. The example builds; I have **not** run it in a window, so the drag *feel* is unverified — but the pane-loss bug you hit is now covered by CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. 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 tile 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. Note that this does not make every `TileId` stable across every edit: `make_all_panes_children_of_tabs` still takes over a pane's id for the tab container it wraps it in. Applications must still identify panes by their payload rather than by tile id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
emilk
force-pushed
the
emilk/stable-tile-ids-on-wrap
branch
from
July 27, 2026 18:31
4626f96 to
f470cce
Compare
# Conflicts: # src/tree.rs
# Conflicts: # src/container/linear.rs # src/container/mod.rs # src/container/tabs.rs # src/tiles.rs # src/tree.rs
The speculative pass runs on a `Tiles::skeleton`, then has to map the resulting rects back onto the real tree. That used to need a log of every id a tile was relocated to, because a move re-used the target's id for the container wrapping it. Since #144 it does not, so the mapping falls out of the ids themselves: - A pane carries its real id as its skeleton payload, so it stays identifiable however many times it moves. - A container is the real container with the same id -- but only where the real tree has a container there too. Simplification still invents containers at ids that hold a pane in the real tree (`all_panes_must_have_tabs` wraps every pane in a `Tabs` at the pane's own id); those have no counterpart to animate, and the kind check already excludes them. So `Tiles::renames`, `insert_new_replacing` and the `renamed_from`/`vacated` maps in `harvest` all go away: -62 lines, +18. Confirmed this loses no preview fidelity rather than assuming it: instrumented the `hovering_a_drag_never_touches_the_real_tree` sweep to record how many tiles get a preview rect on each of its 150 frames, and the distribution is identical before and after -- 7 of 7 tiles with `all_panes_must_have_tabs`, 4 of 4 without. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Speculation` and `PreviewTabs` existed only to distill the speculative tree into something addressable by real tile ids, because the speculative pass could move a tile to a different id. `harvest` did that translation. Two changes make the ids stable everywhere, so the translation is unnecessary: - `make_all_panes_children_of_tabs` now keeps the pane's id and gives the new tab container a fresh one, the same way `insert_at` does since the previous PR. It reports the new id upwards so the parent -- or `Tree::simplify`, for the root -- can point at it. - `flatten_tabs_in_tabs` mutates the container in place rather than minting a replacement. Flattening changes what a container holds, not which container it is. With that, a laid-out `Tiles::skeleton` can be read with the real tree's ids: `tiles.rect(id)` is the target rect, `tiles.get_container(id)` is how a `Tabs` would look. So `Speculation`, `PreviewTabs` and `harvest` all go, and `speculate` is reduced to build-move-simplify-layout. `PreviewTabs` turned out to be a field-for-field copy of `Tabs`, so `Tabs::is_active` now does what a hand-written comparison used to. Note this holds `Tiles`, not a whole `Tree`: a `Tree` owns a `Preview`, so a `Preview` owning a `Tree` is infinitely recursive, and nothing needs the speculative root. A `debug_assert!` now checks that every skeleton pane still sits at its own id, so a future edit that relocates one is caught immediately rather than silently breaking every id-based lookup. `harvest` also used to synthesise a tab bar for containers that the drop collapses, to hide the tab on its way out. That is now a small fallback in `Tabs::tab_bar_ui`, covered by the new `tab_bar_previews_a_tab_leaving` test -- which I confirmed fails without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`smoothness` and `smooth_duration_sec` were passed straight to `emath::exponential_smooth_factor` as `reach_this_fraction` and `in_this_many_seconds`, so use those names. The pair then reads as what it does: reach 90% of the way there in 0.05 seconds. The doc on the second one also claimed it was the `half_time` parameter, which that function does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`PreviewOptions::in_this_many_seconds` duplicated something egui already has, and in a place a user would not think to look: the drag preview animated over its own timescale while everything else in the ui followed `Style::animation_time`. Now it follows that too, leaving `reach_this_fraction` as the only knob. Note this slows the default animation: `Style::animation_time` is 0.2s where the old hard-coded duration was 0.05s. Reducing `Style::animation_time` speeds up the whole ui, which is probably what someone who wants a snappier preview wants anyway. The lerp factor was being computed identically in two places, so it is now one `smoothing_factor` helper. Both callers take `&Ui` rather than `&Context` -- `Ui` derefs to `Context`, and this way they can reach the style too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
My merge resolutions kept this branch's side wholesale, which quietly undid the wording main settled on in review: `LayoutContext` and `layout_tiles` had their general descriptions replaced by ones that tie them to the drag preview, and the generic parameter went back from `TilesPane` to `SkeletonPane`. `layout_tiles` serves any `Tiles`, so main's wording is the right one. Also fills in the `X` placeholder in the `reach_this_fraction` comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mid-drag the tab bar draws the tabs the container _would_ have, and the hover-to-expand logic sets `next_active` from whatever it draws. That could name a tile on its way in but not a child yet, which then got committed to the real container's `active`, leaving it pointing at a tile it does not contain. Caught by `hovering_a_drag_never_touches_the_real_tree`, which compares the whole tree rather than just its shape -- the two trees printed identically, since `Debug` walks from the root and the bad `active` was only visible in the tiles map. Only reachable now because the previous commit slowed the animation to `Style::animation_time`: tiles stay visibly displaced for far longer, so a container's tab bar spends more frames under the pointer while it slides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#144 stopped a move from taking over the target's `TileId`. Simplification still did it in two places: - `make_all_panes_children_of_tabs` re-used the pane's id for the tab container it wraps it in, moving the pane to a fresh id. Now the pane keeps its id and the container gets the fresh one; the new id is reported upwards so the parent -- or `Tree::simplify`, when it is the root that got wrapped -- can point at it. - `flatten_tabs_in_tabs` minted a replacement container. Flattening changes what a container holds, not which container it is, so it now mutates in place. That matters for the same reason as #144: `all_panes_must_have_tabs` is on in Rerun, so under the old behaviour every view's tile id denoted a container rather than the view, and an application mapping ids back to its own model has to know that. With this, a tile keeps its id for as long as it exists, through both moves and simplification. Also fixes the crate docs, which described a `TileId` as random. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Overwriting the container with `Container::new_tabs(..)` reset `Tabs::active` to the first child. The old code did that too, by building a whole new container, but now that flattening keeps the container's identity it should keep what it was showing as well. Two cases: - the open tab survives the flattening: keep it open. - the open tab _is_ the inner container being flattened away: the user is really looking at whichever of its tabs was open, so that one stays open. Reported by Copilot on #147. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
emilk
added a commit
that referenced
this pull request
Jul 28, 2026
Extracted from #139. Finishes what #144 started. #144 stopped a *move* from taking over the target's `TileId`. Simplification still did it in two places: - **`make_all_panes_children_of_tabs`** re-used the pane's id for the tab container it wraps it in, moving the pane to a fresh id. Now the pane keeps its id and the container gets the fresh one. The new id is reported upwards so the parent — or `Tree::simplify`, when it is the root that got wrapped — can point at it. - **`flatten_tabs_in_tabs`** minted a replacement container. Flattening changes what a container *holds*, not which container it *is*, so it now mutates in place. With this, a tile keeps its id for as long as it exists, through both moves and simplification. ### Why it matters Same reason as #144, and it bites harder here: `all_panes_must_have_tabs` is on in Rerun, so under the old behaviour **every view's tile id denoted a container rather than the view**. An application mapping tile ids back to its own model has to know that — Rerun does, via its "Trivial Tab" special case in `save_tree_as_containers`, which stays correct either way. ###⚠️ Breaking Container `TileId`s after simplification differ from before. Panes are strictly better off — they now keep their ids where previously they did not. ### Testing Three new tests: the pane keeps its id and gains a `Tabs` parent, the root-pane case where the new container becomes the root, and flattening keeping the container's id. The first also runs `simplify` twice and asserts the tree is unchanged, since a wrap-forever bug is the obvious way to get this wrong. I checked all three fail against `main`'s behaviour — swapped the two functions back while keeping the tests, and got 3 failures — so they are real regression tests rather than restatements of the new code. Also fixes the crate docs, which described a `TileId` as random. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@stmio's #118 with a rewrite of how the post-drop layout is worked out.
Rebased on
mainnow that #141, #143, #144, #145 and #146 have landed.How it works
To animate, the preview needs to know where every tile would end up if the drag were dropped now. It used to compute that by applying the move to the live tree, laying it out, and restoring the tree from a snapshot afterwards. That restore was driven by a journal of relocations, and it missed some — with
all_panes_must_have_tabs(as Rerun sets it), merely hovering a drag over a pane destroyed it:Speculation now runs against
Tiles::skeleton()— a structural copy where each pane is replaced by its ownTileId. The real tree is only ever borrowed:so no bug in there can lose a pane. #141 is what makes this possible: the layout pass no longer mentions
Pane, so it can lay out the skeleton.Mapping the result back onto the real tree is now just the ids themselves — a pane carries its real id as its skeleton payload, and thanks to #144 a container keeps its id across a wrap. A
debug_assert!compares the pane sets before and after, so a future edit that drops or duplicates a pane is caught here rather than by a puzzled user.The speculation is never stored: it's derived fresh each frame from (real tree, insertion point), so it can't go stale. Only two genuinely cross-frame values live in
egui::Memory— the pending insertion point and the smoothed rects.Fallout
MoveJournal,lerp_t(a tri-statef32never read as a number), thenext_tile_idsave/restore accessors, the four restore loops, thepointer_releasedguard and the id-remapping bookkeeping are all gone.PreviewOptionsmoved next toSimplificationOptions.Testing
hovering_a_drag_never_touches_the_real_treesweeps 75 pointer positions across two simplification configs, asserting the tree is byte-identical throughout — and asserting it actually speculated on most of those frames, so it can't pass vacuously. Checked against the previous implementation in a scratch worktree, where it fails:tab_bar_previews_the_incoming_tabcovers the id mapping via what the user sees: it recordsBehavior::tab_uicalls and asserts the incoming tab shows up selected while the real tree is untouched.The example and drag sweep from #145/#146 also run against this, so the preview is exercised by an app that rebuilds its tree every frame.
fmt --check,clippy --all-features --all-targetsandcargo test --all-featuresclean in debug and release.Still to do
egui::Context::run_ui, which cannot judge whether it feels right. Worth a manual pass before this leaves draft.🤖 Generated with Claude Code