Improve encapsulation of moxie_ui widgets/elems - #130
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (28)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change reorganizes ChangesMoxie UI and editor integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant UiPlugin
participant WindowRegistry
participant DockTree
participant TimelinePanel
Editor->>UiPlugin: initialize editor UI
UiPlugin->>WindowRegistry: register editor windows
UiPlugin->>DockTree: build dock layout
DockTree->>TimelinePanel: create timeline window
TimelinePanel->>Editor: update playback and timeline state
sequenceDiagram
participant User
participant DockDragPlugin
participant DockTree
participant DockView
User->>DockDragPlugin: drag dock tab
DockDragPlugin->>DockView: display ghost and drop overlay
DockDragPlugin->>DockTree: apply tab move or split
DockTree->>DockView: reconcile dock components
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7e64923 to
049fd98
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
editor/moxie_ui/src/lib.rs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueList
iconsin the crate doc.Line 18 declares
pub mod icons. The crate doc enumerates the public modules but omitsicons. Add it so the enumeration matches the public surface.📝 Proposed doc update
//! Reusable `bevy_ui` widgets for the MotionGfx editor: [`elements`] //! (pure `bsn!` building blocks), [`widgets`] (kernel-composed trees: //! docking engine, reflect inspector), [`glass`] (frosted-glass -//! material), [`reactive`] (kernel adapter), and [`theme`]. +//! material), [`reactive`] (kernel adapter), [`icons`] (shared icon +//! asset paths), and [`theme`].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/lib.rs` around lines 1 - 4, Update the crate-level documentation module list to include the public icons module declared by pub mod icons, alongside elements, widgets, glass, reactive, and theme.editor/moxie_ui/src/elements/timeline_track.rs (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that
PIXELS_PER_SECONDbelongs to the consuming app.
PIXELS_PER_SECONDis not defined inmoxie_ui. The editor crate owns the pixels-per-second scale. This element only receives a resolved pixel width throughTimelineTrackProps::width. State that ownership so the doc stays correct for other consumers of this crate.📝 Proposed doc wording
/// The scrubbable timeline track: a plain node sized to the track's -/// duration (`PIXELS_PER_SECOND` per second), so a clip at time `t` -/// sits at `t * PIXELS_PER_SECOND` from its left edge. +/// duration. The consuming app resolves its own pixels-per-second +/// scale and passes the result as `width`, so a clip at time `t` +/// sits at `t * pixels_per_second` from the track's left edge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/elements/timeline_track.rs` around lines 3 - 11, Update the documentation for the timeline track to state that the consuming application owns the PIXELS_PER_SECOND scale, while this element receives only the resolved pixel width through TimelineTrackProps::width. Keep the existing description of sizing and pointer-based scrubbing intact.editor/moxie_ui/src/widgets/dock/drag.rs (3)
450-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated drag-teardown into one helper.
on_drag_end(lines 450-460) andcancel_drag_on_escape(lines 516-523) run the identical four-step teardown: clear the grab cursor, despawn the ghost, restoresource_tabvisibility, and despawn the overlay. Only the drop handling differs. Two copies of teardown will drift as the drag state grows fields.♻️ Suggested helper
/// Tear down the transient drag entities and the grab cursor. fn teardown_drag( commands: &mut Commands, override_cursor: &mut OverrideCursor, source_tab: Entity, ghost_entity: Entity, overlay_entity: Option<Entity>, ) { clear_grab_cursor(override_cursor); commands.entity(ghost_entity).despawn(); // A consumed drop rebuilds the leaf and despawns the tab anyway, // so restoring visibility is best-effort. commands.entity(source_tab).try_insert(Visibility::Inherited); if let Some(overlay) = overlay_entity { commands.entity(overlay).despawn(); } }Also applies to: 516-523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 450 - 460, Extract the shared drag cleanup from on_drag_end and cancel_drag_on_escape into a teardown_drag helper that accepts Commands, OverrideCursor, source_tab, ghost_entity, and optional overlay_entity. Move the four cleanup operations—clear the cursor, despawn the ghost, restore source_tab visibility, and despawn the overlay—into the helper, then call it from both handlers while leaving their distinct drop or cancellation logic unchanged.
700-733: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the nested
is_far_sideso it does not shadow the outer function.The outer
is_far_sidedefines a nestedfn is_far_sidewith the same name and calls it four times. Rust resolves those calls to the nested item because the item shadows the outer name inside the body. The code is correct, but a reader must resolve the shadowing before the four call sites make sense, and thereturn match { .. };followed by an item definition compounds it.♻️ Proposed rename
fn is_far_side( mouse_pos: Vec2, child_pos: Vec2, parent: &Node, ) -> (bool, bool) { return match parent.flex_direction { FlexDirection::Row => { - (is_far_side(mouse_pos, child_pos, false), false) + (past_center(mouse_pos, child_pos, false), false) } FlexDirection::RowReverse => { - (!is_far_side(mouse_pos, child_pos, false), false) + (!past_center(mouse_pos, child_pos, false), false) } FlexDirection::Column => { - (is_far_side(mouse_pos, child_pos, true), true) + (past_center(mouse_pos, child_pos, true), true) } FlexDirection::ColumnReverse => { - (!is_far_side(mouse_pos, child_pos, true), true) + (!past_center(mouse_pos, child_pos, true), true) } }; - fn is_far_side( + fn past_center( mouse_pos: Vec2, child_pos: Vec2, is_vertical: bool, ) -> bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 700 - 733, Rename the nested helper function inside the outer is_far_side to a distinct descriptive name, and update all four FlexDirection match-arm call sites to use it. Leave the outer is_far_side signature and its direction-handling behavior unchanged.
210-217: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffReuse the drop overlay entity instead of respawning it per drag event.
on_drag_movedespawnsoverlay_entityand spawns a replacement on everyPointer<Drag>event. During a drag this creates and destroys an entity each frame, which churns archetypes and command buffers for a node whose only changing fields areleft,top,width, andheight.Keep one overlay entity for the lifetime of the drag and update its
NodeandVisibility, the same wayghost_nodealready updates the ghost in place at line 210.Also applies to: 316-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 210 - 217, Update on_drag_move to reuse the existing overlay entity throughout the drag instead of despawning old_overlay and creating a replacement on each event. When the drop target changes, update the overlay’s Node fields (left, top, width, and height) and Visibility in place, while preserving the existing overlay entity and ghost_node update flow.editor/moxie_ui/src/widgets/dock/add_popup.rs (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the single-overlay assumption fail loudly.
AddWindowPopupStateis now a component, and three handlers reach it withq_state.single_mut().single_mutreturnsErrfor zero matches and for two or more matches. If a second dock root is ever mounted, every "+" button and every outside-click stops closing the popup, with no log line and no panic. The failure is silent.Add a debug assertion or a warning at the query sites, or scope the lookup to the owning overlay entity the same way
build_popupalready does withui.parent().♻️ Suggested guard in `on_add_click`
let Ok(mut state) = q_state.single_mut() else { + debug_assert!( + false, + "expected exactly one AddWindowPopupState overlay" + ); return; };Also applies to: 58-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/add_popup.rs` around lines 26 - 32, Make the single-overlay invariant explicit at every handler querying AddWindowPopupState, including on_add_click and the handlers around the referenced lines: handle q_state.single_mut() errors with a debug assertion or warning so zero or multiple matches are observable, or scope each query to the owning overlay entity consistently with build_popup and ui.parent().editor/moxie_ui/src/widgets/dock/tree.rs (1)
770-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining public tree operations.
The suite covers split, move, reorder, and simplify well. Three public methods stay untested:
remove_window_kind,insert_tabwith an explicit index into a different leaf, anditer_dfs.remove_window_kindcallssimplifyonce per removed tab, so its interaction with collapsing leaves is the highest-value gap.🧪 Suggested test for `remove_window_kind`
#[test] fn remove_window_kind_drops_every_instance() { let mut t = DockTree::new(); let root = t.set_root_leaf(leaf("root", &["a", "outliner"])); let (right, _) = t.split(root, Edge::Right, "outliner".into()).unwrap(); let _ = right; t.remove_window_kind("outliner"); // Only "a" survives, and the drained split collapsed. assert!(matches!(t.nodes[&t.root.unwrap()], DockNode::Leaf(_))); assert_eq!(window_ids(&t, t.root.unwrap()), vec!["a"]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/tree.rs` around lines 770 - 1012, Add tests covering the three untested public operations: verify remove_window_kind removes every matching tab and simplifies drained leaves/splits, verify insert_tab with an explicit index inserts into a different destination leaf at the requested position and activates the tab, and verify iter_dfs visits the tree nodes in depth-first order. Add these tests alongside the existing DockTree tests, reusing helpers such as window_ids and tab_id_for.editor/moxie/src/ui.rs (1)
164-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the window id literals into constants.
The ids
"viewport","timeline","hierarchy"and"settings"appear both here and insetup_editor_uiat Lines 116-118, 121, 124 and 127.WindowRegistryresolves the dock leaf's window ids against the registered descriptors at runtime, so a typo in either place produces an empty tab with no compile-time error.Define the ids once and use them in both functions.
♻️ Proposed refactor
pub(crate) const PANEL_PADDING: f32 = 12.0; + +const WIN_VIEWPORT: &str = "viewport"; +const WIN_TIMELINE: &str = "timeline"; +const WIN_HIERARCHY: &str = "hierarchy"; +const WIN_SETTINGS: &str = "settings";Then use them at both sites, for example:
let viewport = tree.set_root_leaf( - DockLeaf::new("viewport", DockAreaStyle::TabBar) + DockLeaf::new(WIN_VIEWPORT, DockAreaStyle::TabBar) .with_windows(vec![ - "viewport".into(), - "hierarchy".into(), - "settings".into(), + WIN_VIEWPORT.into(), + WIN_HIERARCHY.into(), + WIN_SETTINGS.into(), ]), ); - tree.split(viewport, Edge::Bottom, "timeline".into()); + tree.split(viewport, Edge::Bottom, WIN_TIMELINE.into());registry.register(DockWindowDescriptor { - id: "timeline".into(), + id: WIN_TIMELINE.into(), name: "Timeline".into(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie/src/ui.rs` around lines 164 - 224, Define shared constants for the window IDs "viewport", "timeline", "hierarchy", and "settings" in the appropriate UI module scope, then replace the literals in both the dock descriptor registrations and setup_editor_ui with those constants. Ensure WindowRegistry registration and dock leaf references use the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/moxie_ui/src/glass/glass.wgsl`:
- Line 46: Update the shape calculation around the WGSL `shape` expression to
use a hard mask when `extra.x` is zero, while retaining the existing smooth
feathering behavior for nonzero values. Ensure the zero-feather path does not
force a one-pixel feather through `max(extra.x, 1.0)`.
In `@editor/moxie_ui/src/lib.rs`:
- Around line 33-50: Update MoxieUiPlugin’s documentation to remove the claim
that DockPlugin pulls in GlassPlugin, since GlassPlugin is registered explicitly
in the plugin tuple. Keep the existing GlassPlugin registration unchanged.
In `@editor/moxie_ui/src/reactive.rs`:
- Around line 250-261: Update component_changed so an absent C returns false on
the initial poll and remains treated as unchanged until a component value is
present, rather than delegating directly to value_changed with Option<C>.
Preserve change detection for inserted or modified C values and use the existing
world.get::<C> lookup.
In `@editor/moxie_ui/src/widgets/dock/drag.rs`:
- Around line 88-113: Update on_tab_drag_start to return immediately when
drag_state is already in an active drag state, especially
DockDragState::Dragging, before resolving the tab or assigning PendingDrag.
Preserve the existing initialization for idle or pending states as appropriate,
ensuring an in-flight drag’s ghost_entity, overlay_entity, and hidden source tab
remain intact.
In `@editor/moxie_ui/src/widgets/dock/split.rs`:
- Around line 207-230: Update on_handle_drag_end to clear override_cursor before
looking up the parent Node, so cleanup still occurs when the parent is missing
or reconfigured. Remove the dependency on the current flex_direction and clear
the override whenever it contains either supported resize cursor icon, while
preserving the existing drag-marker cleanup.
In `@editor/moxie_ui/src/widgets/inspector.rs`:
- Around line 361-379: Update the label construction in the inspector row around
Field::new and collect_leaves so it uses the final reflect-path segment, rather
than the full raw path. When the path is empty, fall back to the target’s
reflected type name; preserve readable nested labels instead of rendering bare
or empty text.
In `@editor/moxie_ui/src/widgets/inspector/widget.rs`:
- Around line 197-200: Update the u64 conversion entry so its outbound to_input
closure saturates values above i64::MAX instead of casting and wrapping;
preserve the existing inbound nonnegative clamp, ensuring both directions
saturate and u64::MAX displays as i64::MAX without losing the target value on
round trip.
- Around line 87-118: Remove the immediate Checked insert/remove operations from
the ValueChange observer and let bind_raw synchronize the checkbox after a
successful field.set, keeping the observer focused on queuing the write. Ensure
the queued write targets the Checkbox root node by using node rather than
change.source where the entity is required, and update the misleading
controlled-state comment accordingly.
In `@editor/moxie/src/ui.rs`:
- Around line 79-106: Document in the reflected EditorSettings definitions that
hdr and physical_size are restart-only settings because setup_editor_ui reads
them only during startup. Ensure the Settings UI communicates that runtime
changes are not applied, without altering setup_editor_ui or adding live-update
handling.
In `@editor/moxie/src/ui/timeline.rs`:
- Around line 262-281: Update on_divider_drag to base the panel width on the
pointer’s absolute drag distance rather than accumulating drag.delta.x. Add the
corresponding DragStart observer/state alongside the existing observer to record
the initial width, then apply drag.distance.x to that starting width before
clamping to NAME_PANEL_MIN and NAME_PANEL_MAX, matching handle_panel_drag’s
approach.
---
Nitpick comments:
In `@editor/moxie_ui/src/elements/timeline_track.rs`:
- Around line 3-11: Update the documentation for the timeline track to state
that the consuming application owns the PIXELS_PER_SECOND scale, while this
element receives only the resolved pixel width through
TimelineTrackProps::width. Keep the existing description of sizing and
pointer-based scrubbing intact.
In `@editor/moxie_ui/src/lib.rs`:
- Around line 1-4: Update the crate-level documentation module list to include
the public icons module declared by pub mod icons, alongside elements, widgets,
glass, reactive, and theme.
In `@editor/moxie_ui/src/widgets/dock/add_popup.rs`:
- Around line 26-32: Make the single-overlay invariant explicit at every handler
querying AddWindowPopupState, including on_add_click and the handlers around the
referenced lines: handle q_state.single_mut() errors with a debug assertion or
warning so zero or multiple matches are observable, or scope each query to the
owning overlay entity consistently with build_popup and ui.parent().
In `@editor/moxie_ui/src/widgets/dock/drag.rs`:
- Around line 450-460: Extract the shared drag cleanup from on_drag_end and
cancel_drag_on_escape into a teardown_drag helper that accepts Commands,
OverrideCursor, source_tab, ghost_entity, and optional overlay_entity. Move the
four cleanup operations—clear the cursor, despawn the ghost, restore source_tab
visibility, and despawn the overlay—into the helper, then call it from both
handlers while leaving their distinct drop or cancellation logic unchanged.
- Around line 700-733: Rename the nested helper function inside the outer
is_far_side to a distinct descriptive name, and update all four FlexDirection
match-arm call sites to use it. Leave the outer is_far_side signature and its
direction-handling behavior unchanged.
- Around line 210-217: Update on_drag_move to reuse the existing overlay entity
throughout the drag instead of despawning old_overlay and creating a replacement
on each event. When the drop target changes, update the overlay’s Node fields
(left, top, width, and height) and Visibility in place, while preserving the
existing overlay entity and ghost_node update flow.
In `@editor/moxie_ui/src/widgets/dock/tree.rs`:
- Around line 770-1012: Add tests covering the three untested public operations:
verify remove_window_kind removes every matching tab and simplifies drained
leaves/splits, verify insert_tab with an explicit index inserts into a different
destination leaf at the requested position and activates the tab, and verify
iter_dfs visits the tree nodes in depth-first order. Add these tests alongside
the existing DockTree tests, reusing helpers such as window_ids and tab_id_for.
In `@editor/moxie/src/ui.rs`:
- Around line 164-224: Define shared constants for the window IDs "viewport",
"timeline", "hierarchy", and "settings" in the appropriate UI module scope, then
replace the literals in both the dock descriptor registrations and
setup_editor_ui with those constants. Ensure WindowRegistry registration and
dock leaf references use the same symbols.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b3883e4-6963-4dd2-a940-ad417392fa64
📒 Files selected for processing (34)
editor/moxie/src/lib.rseditor/moxie/src/playback.rseditor/moxie/src/scene.rseditor/moxie/src/ui.rseditor/moxie/src/ui/hierarchy.rseditor/moxie/src/ui/timeline.rseditor/moxie/src/view.rseditor/moxie_ui/examples/dock_demo.rseditor/moxie_ui/src/elements.rseditor/moxie_ui/src/elements/divider.rseditor/moxie_ui/src/elements/frame.rseditor/moxie_ui/src/elements/ghost_button.rseditor/moxie_ui/src/elements/label.rseditor/moxie_ui/src/elements/playhead.rseditor/moxie_ui/src/elements/timeline_track.rseditor/moxie_ui/src/glass/glass.wgsleditor/moxie_ui/src/glass/material.rseditor/moxie_ui/src/glass/mod.rseditor/moxie_ui/src/icons.rseditor/moxie_ui/src/lib.rseditor/moxie_ui/src/reactive.rseditor/moxie_ui/src/widgets.rseditor/moxie_ui/src/widgets/dock.rseditor/moxie_ui/src/widgets/dock/add_popup.rseditor/moxie_ui/src/widgets/dock/area.rseditor/moxie_ui/src/widgets/dock/drag.rseditor/moxie_ui/src/widgets/dock/reconcile.rseditor/moxie_ui/src/widgets/dock/registry.rseditor/moxie_ui/src/widgets/dock/split.rseditor/moxie_ui/src/widgets/dock/tabs.rseditor/moxie_ui/src/widgets/dock/tree.rseditor/moxie_ui/src/widgets/glass_backdrop.rseditor/moxie_ui/src/widgets/inspector.rseditor/moxie_ui/src/widgets/inspector/widget.rs
💤 Files with no reviewable changes (2)
- editor/moxie_ui/src/glass/mod.rs
- editor/moxie/src/scene.rs
| /// Fires when the watched node's `C` differs from the last poll. | ||
| /// | ||
| /// The entity-local counterpart to [`resource_changed`]: state for a | ||
| /// single widget instance (a popup's open/closed, a field's edit | ||
| /// buffer) belongs on that widget's own node, not in a global | ||
| /// `Resource` that every instance of the widget would have to share. | ||
| /// `C` absent reads as unchanged, not a rebuild — a node that hasn't | ||
| /// had its state inserted yet is not yet ready to build. | ||
| pub fn component_changed<C: Component + Clone + PartialEq>() | ||
| -> impl FnMut(&World, Entity) -> bool { | ||
| value_changed(|world, node| world.get::<C>(node).cloned()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
component_changed fires on the first poll when C is absent.
The doc at lines 256-257 states that an absent C reads as unchanged. The implementation does not hold that promise.
value_changed is instantiated with T = Option<C> and starts with seen: Option<Option<C>> = None. On the first poll with C absent, current is None, so the comparison is None != Some(&None), which is true. The watcher fires and the widget builds once before its state component exists. The intended behavior is to wait until the state lands.
Track the absent case explicitly instead of delegating to value_changed.
🐛 Proposed fix
pub fn component_changed<C: Component + Clone + PartialEq>()
-> impl FnMut(&World, Entity) -> bool {
- value_changed(|world, node| world.get::<C>(node).cloned())
+ let mut seen: Option<C> = None;
+ move |world, node| {
+ // `C` absent is not a rebuild: the node is not ready yet.
+ let Some(current) = world.get::<C>(node) else {
+ return false;
+ };
+ let fired = seen.as_ref() != Some(current);
+ seen = Some(current.clone());
+ fired
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Fires when the watched node's `C` differs from the last poll. | |
| /// | |
| /// The entity-local counterpart to [`resource_changed`]: state for a | |
| /// single widget instance (a popup's open/closed, a field's edit | |
| /// buffer) belongs on that widget's own node, not in a global | |
| /// `Resource` that every instance of the widget would have to share. | |
| /// `C` absent reads as unchanged, not a rebuild — a node that hasn't | |
| /// had its state inserted yet is not yet ready to build. | |
| pub fn component_changed<C: Component + Clone + PartialEq>() | |
| -> impl FnMut(&World, Entity) -> bool { | |
| value_changed(|world, node| world.get::<C>(node).cloned()) | |
| } | |
| /// Fires when the watched node's `C` differs from the last poll. | |
| /// | |
| /// The entity-local counterpart to [`resource_changed`]: state for a | |
| /// single widget instance (a popup's open/closed, a field's edit | |
| /// buffer) belongs on that widget's own node, not in a global | |
| /// `Resource` that every instance of the widget would have to share. | |
| /// `C` absent reads as unchanged, not a rebuild — a node that hasn't | |
| /// had its state inserted yet is not yet ready to build. | |
| pub fn component_changed<C: Component + Clone + PartialEq>() | |
| -> impl FnMut(&World, Entity) -> bool { | |
| let mut seen: Option<C> = None; | |
| move |world, node| { | |
| // `C` absent is not a rebuild: the node is not ready yet. | |
| let Some(current) = world.get::<C>(node) else { | |
| return false; | |
| }; | |
| let fired = seen.as_ref() != Some(current); | |
| seen = Some(current.clone()); | |
| fired | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/moxie_ui/src/reactive.rs` around lines 250 - 261, Update
component_changed so an absent C returns false on the initial poll and remains
treated as unchanged until a component value is present, rather than delegating
directly to value_changed with Option<C>. Preserve change detection for inserted
or modified C values and use the existing world.get::<C> lookup.
| let size = settings.physical_size.max(UVec2::ONE); | ||
| let preview = images.add(Image::new_target_texture( | ||
| size.x, | ||
| size.y, | ||
| TextureFormat::Rgba8Unorm, | ||
| Some(TextureFormat::Rgba8UnormSrgb), | ||
| )); | ||
| commands.insert_resource(PreviewImage(preview.clone())); | ||
|
|
||
| // Own render layer so this camera doesn't also pick up scene | ||
| // meshes (e.g. bevy_vello's composite quad, layer 0) | ||
| // full-window. `IsDefaultUiCamera` catches dock UI spawned | ||
| // without a target (drag ghosts, drop overlays). | ||
| let ui_camera = commands | ||
| .spawn_scene(bsn! [ | ||
| Camera2d | ||
| Camera { | ||
| order: 10, | ||
| clear_color: Color::BLACK, | ||
| } | ||
| TrackViewportCamera | ||
| ]) | ||
| .insert((RenderLayers::layer(1), IsDefaultUiCamera)) | ||
| .id(); | ||
|
|
||
| if settings.hdr { | ||
| commands.entity(ui_camera).insert(Hdr); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether any system reacts to EditorSettings changes to re-apply hdr / physical_size.
set -euo pipefail
rg -n -C 5 'struct EditorSettings' editor --type=rust
rg -n -C 4 'EditorSettings' editor --type=rust -g '!**/ui.rs'
rg -n -C 4 '\bHdr\b|physical_size' editor --type=rustRepository: voxell-tech/motiongfx
Length of output: 7002
Document hdr and physical_size as restart-only settings.
setup_editor_ui runs only once in Startup and reads these EditorSettings fields. The Settings window exposes all reflected EditorSettings fields, so runtime changes to hdr or physical_size are not re-applied. If live updates are not intended, document that; otherwise, add an update handler to re-apply them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/moxie/src/ui.rs` around lines 79 - 106, Document in the reflected
EditorSettings definitions that hdr and physical_size are restart-only settings
because setup_editor_ui reads them only during startup. Ensure the Settings UI
communicates that runtime changes are not applied, without altering
setup_editor_ui or adding live-update handling.
There was a problem hiding this comment.
Known issue, please create one to track this, actually all the other settings aren't updated too.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="voxell-tech/motiongfx"
body=$(cat <<'EOF'
## Summary
Runtime changes to `EditorSettings` are not applied to the running editor.
`setup_editor_ui` reads settings during `Startup`. The Settings window exposes reflected `EditorSettings` fields, but the editor does not react when those fields change. This includes `hdr`, `physical_size`, and the other exposed settings.
## Required changes
Define the runtime-update behavior for each exposed `EditorSettings` field.
For settings that support live updates, add change handling that updates the affected editor UI, camera, render target, or related state.
For settings that require restart, communicate that requirement in the Settings UI and documentation.
## Affected areas
- `editor/moxie/src/ui.rs`
- `EditorSettings` definition and reflection registration
- Settings window and inspector integration
- Systems that consume editor settings
## Rationale
The Settings window currently permits edits that do not take effect. This creates an incorrect runtime expectation.
## Acceptance criteria
- Each exposed `EditorSettings` field has defined behavior after a runtime edit.
- Live-update settings are applied without restarting the editor.
- Restart-only settings are clearly identified in the Settings UI.
- Tests or targeted validation cover the selected behavior.
## Backlinks
- Pull request: https://github.com/voxell-tech/motiongfx/pull/130
- Review comment: https://github.com/voxell-tech/motiongfx/pull/130#discussion_r3718573287
- Requested by: `@nixonyh`
EOF
)
gh issue create \
--repo "$repo" \
--title "Apply EditorSettings changes at runtime or mark them restart-only" \
--body "$body" \
--assignee "nixonyh"Length of output: 1918
@nixonyh, created and assigned follow-up issue #133.
It tracks runtime application or restart-only labeling for all exposed EditorSettings fields, not only hdr and physical_size.
✏️ Learnings added
Learnt from: nixonyh
Repo: voxell-tech/motiongfx PR: 130
File: editor/moxie/src/ui.rs:79-106
Timestamp: 2026-08-05T09:45:41.099Z
Learning: In the `editor/moxie` editor, the Settings window exposes `EditorSettings` fields, but runtime edits to all exposed fields are currently not applied. Follow-up issue `#133` tracks defining live-update behavior or clear restart-only labeling for each setting.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🧹 Nitpick comments (8)
editor/moxie_ui/src/lib.rs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueList
iconsin the crate doc.Line 18 declares
pub mod icons. The crate doc enumerates the public modules but omitsicons. Add it so the enumeration matches the public surface.📝 Proposed doc update
//! Reusable `bevy_ui` widgets for the MotionGfx editor: [`elements`] //! (pure `bsn!` building blocks), [`widgets`] (kernel-composed trees: //! docking engine, reflect inspector), [`glass`] (frosted-glass -//! material), [`reactive`] (kernel adapter), and [`theme`]. +//! material), [`reactive`] (kernel adapter), [`icons`] (shared icon +//! asset paths), and [`theme`].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/lib.rs` around lines 1 - 4, Update the crate-level documentation module list to include the public icons module declared by pub mod icons, alongside elements, widgets, glass, reactive, and theme.editor/moxie_ui/src/elements/timeline_track.rs (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that
PIXELS_PER_SECONDbelongs to the consuming app.
PIXELS_PER_SECONDis not defined inmoxie_ui. The editor crate owns the pixels-per-second scale. This element only receives a resolved pixel width throughTimelineTrackProps::width. State that ownership so the doc stays correct for other consumers of this crate.📝 Proposed doc wording
/// The scrubbable timeline track: a plain node sized to the track's -/// duration (`PIXELS_PER_SECOND` per second), so a clip at time `t` -/// sits at `t * PIXELS_PER_SECOND` from its left edge. +/// duration. The consuming app resolves its own pixels-per-second +/// scale and passes the result as `width`, so a clip at time `t` +/// sits at `t * pixels_per_second` from the track's left edge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/elements/timeline_track.rs` around lines 3 - 11, Update the documentation for the timeline track to state that the consuming application owns the PIXELS_PER_SECOND scale, while this element receives only the resolved pixel width through TimelineTrackProps::width. Keep the existing description of sizing and pointer-based scrubbing intact.editor/moxie_ui/src/widgets/dock/drag.rs (3)
450-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated drag-teardown into one helper.
on_drag_end(lines 450-460) andcancel_drag_on_escape(lines 516-523) run the identical four-step teardown: clear the grab cursor, despawn the ghost, restoresource_tabvisibility, and despawn the overlay. Only the drop handling differs. Two copies of teardown will drift as the drag state grows fields.♻️ Suggested helper
/// Tear down the transient drag entities and the grab cursor. fn teardown_drag( commands: &mut Commands, override_cursor: &mut OverrideCursor, source_tab: Entity, ghost_entity: Entity, overlay_entity: Option<Entity>, ) { clear_grab_cursor(override_cursor); commands.entity(ghost_entity).despawn(); // A consumed drop rebuilds the leaf and despawns the tab anyway, // so restoring visibility is best-effort. commands.entity(source_tab).try_insert(Visibility::Inherited); if let Some(overlay) = overlay_entity { commands.entity(overlay).despawn(); } }Also applies to: 516-523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 450 - 460, Extract the shared drag cleanup from on_drag_end and cancel_drag_on_escape into a teardown_drag helper that accepts Commands, OverrideCursor, source_tab, ghost_entity, and optional overlay_entity. Move the four cleanup operations—clear the cursor, despawn the ghost, restore source_tab visibility, and despawn the overlay—into the helper, then call it from both handlers while leaving their distinct drop or cancellation logic unchanged.
700-733: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the nested
is_far_sideso it does not shadow the outer function.The outer
is_far_sidedefines a nestedfn is_far_sidewith the same name and calls it four times. Rust resolves those calls to the nested item because the item shadows the outer name inside the body. The code is correct, but a reader must resolve the shadowing before the four call sites make sense, and thereturn match { .. };followed by an item definition compounds it.♻️ Proposed rename
fn is_far_side( mouse_pos: Vec2, child_pos: Vec2, parent: &Node, ) -> (bool, bool) { return match parent.flex_direction { FlexDirection::Row => { - (is_far_side(mouse_pos, child_pos, false), false) + (past_center(mouse_pos, child_pos, false), false) } FlexDirection::RowReverse => { - (!is_far_side(mouse_pos, child_pos, false), false) + (!past_center(mouse_pos, child_pos, false), false) } FlexDirection::Column => { - (is_far_side(mouse_pos, child_pos, true), true) + (past_center(mouse_pos, child_pos, true), true) } FlexDirection::ColumnReverse => { - (!is_far_side(mouse_pos, child_pos, true), true) + (!past_center(mouse_pos, child_pos, true), true) } }; - fn is_far_side( + fn past_center( mouse_pos: Vec2, child_pos: Vec2, is_vertical: bool, ) -> bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 700 - 733, Rename the nested helper function inside the outer is_far_side to a distinct descriptive name, and update all four FlexDirection match-arm call sites to use it. Leave the outer is_far_side signature and its direction-handling behavior unchanged.
210-217: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffReuse the drop overlay entity instead of respawning it per drag event.
on_drag_movedespawnsoverlay_entityand spawns a replacement on everyPointer<Drag>event. During a drag this creates and destroys an entity each frame, which churns archetypes and command buffers for a node whose only changing fields areleft,top,width, andheight.Keep one overlay entity for the lifetime of the drag and update its
NodeandVisibility, the same wayghost_nodealready updates the ghost in place at line 210.Also applies to: 316-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 210 - 217, Update on_drag_move to reuse the existing overlay entity throughout the drag instead of despawning old_overlay and creating a replacement on each event. When the drop target changes, update the overlay’s Node fields (left, top, width, and height) and Visibility in place, while preserving the existing overlay entity and ghost_node update flow.editor/moxie_ui/src/widgets/dock/add_popup.rs (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the single-overlay assumption fail loudly.
AddWindowPopupStateis now a component, and three handlers reach it withq_state.single_mut().single_mutreturnsErrfor zero matches and for two or more matches. If a second dock root is ever mounted, every "+" button and every outside-click stops closing the popup, with no log line and no panic. The failure is silent.Add a debug assertion or a warning at the query sites, or scope the lookup to the owning overlay entity the same way
build_popupalready does withui.parent().♻️ Suggested guard in `on_add_click`
let Ok(mut state) = q_state.single_mut() else { + debug_assert!( + false, + "expected exactly one AddWindowPopupState overlay" + ); return; };Also applies to: 58-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/add_popup.rs` around lines 26 - 32, Make the single-overlay invariant explicit at every handler querying AddWindowPopupState, including on_add_click and the handlers around the referenced lines: handle q_state.single_mut() errors with a debug assertion or warning so zero or multiple matches are observable, or scope each query to the owning overlay entity consistently with build_popup and ui.parent().editor/moxie_ui/src/widgets/dock/tree.rs (1)
770-1012: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining public tree operations.
The suite covers split, move, reorder, and simplify well. Three public methods stay untested:
remove_window_kind,insert_tabwith an explicit index into a different leaf, anditer_dfs.remove_window_kindcallssimplifyonce per removed tab, so its interaction with collapsing leaves is the highest-value gap.🧪 Suggested test for `remove_window_kind`
#[test] fn remove_window_kind_drops_every_instance() { let mut t = DockTree::new(); let root = t.set_root_leaf(leaf("root", &["a", "outliner"])); let (right, _) = t.split(root, Edge::Right, "outliner".into()).unwrap(); let _ = right; t.remove_window_kind("outliner"); // Only "a" survives, and the drained split collapsed. assert!(matches!(t.nodes[&t.root.unwrap()], DockNode::Leaf(_))); assert_eq!(window_ids(&t, t.root.unwrap()), vec!["a"]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/tree.rs` around lines 770 - 1012, Add tests covering the three untested public operations: verify remove_window_kind removes every matching tab and simplifies drained leaves/splits, verify insert_tab with an explicit index inserts into a different destination leaf at the requested position and activates the tab, and verify iter_dfs visits the tree nodes in depth-first order. Add these tests alongside the existing DockTree tests, reusing helpers such as window_ids and tab_id_for.editor/moxie/src/ui.rs (1)
164-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the window id literals into constants.
The ids
"viewport","timeline","hierarchy"and"settings"appear both here and insetup_editor_uiat Lines 116-118, 121, 124 and 127.WindowRegistryresolves the dock leaf's window ids against the registered descriptors at runtime, so a typo in either place produces an empty tab with no compile-time error.Define the ids once and use them in both functions.
♻️ Proposed refactor
pub(crate) const PANEL_PADDING: f32 = 12.0; + +const WIN_VIEWPORT: &str = "viewport"; +const WIN_TIMELINE: &str = "timeline"; +const WIN_HIERARCHY: &str = "hierarchy"; +const WIN_SETTINGS: &str = "settings";Then use them at both sites, for example:
let viewport = tree.set_root_leaf( - DockLeaf::new("viewport", DockAreaStyle::TabBar) + DockLeaf::new(WIN_VIEWPORT, DockAreaStyle::TabBar) .with_windows(vec![ - "viewport".into(), - "hierarchy".into(), - "settings".into(), + WIN_VIEWPORT.into(), + WIN_HIERARCHY.into(), + WIN_SETTINGS.into(), ]), ); - tree.split(viewport, Edge::Bottom, "timeline".into()); + tree.split(viewport, Edge::Bottom, WIN_TIMELINE.into());registry.register(DockWindowDescriptor { - id: "timeline".into(), + id: WIN_TIMELINE.into(), name: "Timeline".into(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie/src/ui.rs` around lines 164 - 224, Define shared constants for the window IDs "viewport", "timeline", "hierarchy", and "settings" in the appropriate UI module scope, then replace the literals in both the dock descriptor registrations and setup_editor_ui with those constants. Ensure WindowRegistry registration and dock leaf references use the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/moxie_ui/src/glass/glass.wgsl`:
- Line 46: Update the shape calculation around the WGSL `shape` expression to
use a hard mask when `extra.x` is zero, while retaining the existing smooth
feathering behavior for nonzero values. Ensure the zero-feather path does not
force a one-pixel feather through `max(extra.x, 1.0)`.
In `@editor/moxie_ui/src/lib.rs`:
- Around line 33-50: Update MoxieUiPlugin’s documentation to remove the claim
that DockPlugin pulls in GlassPlugin, since GlassPlugin is registered explicitly
in the plugin tuple. Keep the existing GlassPlugin registration unchanged.
In `@editor/moxie_ui/src/reactive.rs`:
- Around line 250-261: Update component_changed so an absent C returns false on
the initial poll and remains treated as unchanged until a component value is
present, rather than delegating directly to value_changed with Option<C>.
Preserve change detection for inserted or modified C values and use the existing
world.get::<C> lookup.
In `@editor/moxie_ui/src/widgets/dock/drag.rs`:
- Around line 88-113: Update on_tab_drag_start to return immediately when
drag_state is already in an active drag state, especially
DockDragState::Dragging, before resolving the tab or assigning PendingDrag.
Preserve the existing initialization for idle or pending states as appropriate,
ensuring an in-flight drag’s ghost_entity, overlay_entity, and hidden source tab
remain intact.
In `@editor/moxie_ui/src/widgets/dock/split.rs`:
- Around line 207-230: Update on_handle_drag_end to clear override_cursor before
looking up the parent Node, so cleanup still occurs when the parent is missing
or reconfigured. Remove the dependency on the current flex_direction and clear
the override whenever it contains either supported resize cursor icon, while
preserving the existing drag-marker cleanup.
In `@editor/moxie_ui/src/widgets/inspector.rs`:
- Around line 361-379: Update the label construction in the inspector row around
Field::new and collect_leaves so it uses the final reflect-path segment, rather
than the full raw path. When the path is empty, fall back to the target’s
reflected type name; preserve readable nested labels instead of rendering bare
or empty text.
In `@editor/moxie_ui/src/widgets/inspector/widget.rs`:
- Around line 197-200: Update the u64 conversion entry so its outbound to_input
closure saturates values above i64::MAX instead of casting and wrapping;
preserve the existing inbound nonnegative clamp, ensuring both directions
saturate and u64::MAX displays as i64::MAX without losing the target value on
round trip.
- Around line 87-118: Remove the immediate Checked insert/remove operations from
the ValueChange observer and let bind_raw synchronize the checkbox after a
successful field.set, keeping the observer focused on queuing the write. Ensure
the queued write targets the Checkbox root node by using node rather than
change.source where the entity is required, and update the misleading
controlled-state comment accordingly.
In `@editor/moxie/src/ui.rs`:
- Around line 79-106: Document in the reflected EditorSettings definitions that
hdr and physical_size are restart-only settings because setup_editor_ui reads
them only during startup. Ensure the Settings UI communicates that runtime
changes are not applied, without altering setup_editor_ui or adding live-update
handling.
In `@editor/moxie/src/ui/timeline.rs`:
- Around line 262-281: Update on_divider_drag to base the panel width on the
pointer’s absolute drag distance rather than accumulating drag.delta.x. Add the
corresponding DragStart observer/state alongside the existing observer to record
the initial width, then apply drag.distance.x to that starting width before
clamping to NAME_PANEL_MIN and NAME_PANEL_MAX, matching handle_panel_drag’s
approach.
---
Nitpick comments:
In `@editor/moxie_ui/src/elements/timeline_track.rs`:
- Around line 3-11: Update the documentation for the timeline track to state
that the consuming application owns the PIXELS_PER_SECOND scale, while this
element receives only the resolved pixel width through
TimelineTrackProps::width. Keep the existing description of sizing and
pointer-based scrubbing intact.
In `@editor/moxie_ui/src/lib.rs`:
- Around line 1-4: Update the crate-level documentation module list to include
the public icons module declared by pub mod icons, alongside elements, widgets,
glass, reactive, and theme.
In `@editor/moxie_ui/src/widgets/dock/add_popup.rs`:
- Around line 26-32: Make the single-overlay invariant explicit at every handler
querying AddWindowPopupState, including on_add_click and the handlers around the
referenced lines: handle q_state.single_mut() errors with a debug assertion or
warning so zero or multiple matches are observable, or scope each query to the
owning overlay entity consistently with build_popup and ui.parent().
In `@editor/moxie_ui/src/widgets/dock/drag.rs`:
- Around line 450-460: Extract the shared drag cleanup from on_drag_end and
cancel_drag_on_escape into a teardown_drag helper that accepts Commands,
OverrideCursor, source_tab, ghost_entity, and optional overlay_entity. Move the
four cleanup operations—clear the cursor, despawn the ghost, restore source_tab
visibility, and despawn the overlay—into the helper, then call it from both
handlers while leaving their distinct drop or cancellation logic unchanged.
- Around line 700-733: Rename the nested helper function inside the outer
is_far_side to a distinct descriptive name, and update all four FlexDirection
match-arm call sites to use it. Leave the outer is_far_side signature and its
direction-handling behavior unchanged.
- Around line 210-217: Update on_drag_move to reuse the existing overlay entity
throughout the drag instead of despawning old_overlay and creating a replacement
on each event. When the drop target changes, update the overlay’s Node fields
(left, top, width, and height) and Visibility in place, while preserving the
existing overlay entity and ghost_node update flow.
In `@editor/moxie_ui/src/widgets/dock/tree.rs`:
- Around line 770-1012: Add tests covering the three untested public operations:
verify remove_window_kind removes every matching tab and simplifies drained
leaves/splits, verify insert_tab with an explicit index inserts into a different
destination leaf at the requested position and activates the tab, and verify
iter_dfs visits the tree nodes in depth-first order. Add these tests alongside
the existing DockTree tests, reusing helpers such as window_ids and tab_id_for.
In `@editor/moxie/src/ui.rs`:
- Around line 164-224: Define shared constants for the window IDs "viewport",
"timeline", "hierarchy", and "settings" in the appropriate UI module scope, then
replace the literals in both the dock descriptor registrations and
setup_editor_ui with those constants. Ensure WindowRegistry registration and
dock leaf references use the same symbols.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b3883e4-6963-4dd2-a940-ad417392fa64
📒 Files selected for processing (34)
editor/moxie/src/lib.rseditor/moxie/src/playback.rseditor/moxie/src/scene.rseditor/moxie/src/ui.rseditor/moxie/src/ui/hierarchy.rseditor/moxie/src/ui/timeline.rseditor/moxie/src/view.rseditor/moxie_ui/examples/dock_demo.rseditor/moxie_ui/src/elements.rseditor/moxie_ui/src/elements/divider.rseditor/moxie_ui/src/elements/frame.rseditor/moxie_ui/src/elements/ghost_button.rseditor/moxie_ui/src/elements/label.rseditor/moxie_ui/src/elements/playhead.rseditor/moxie_ui/src/elements/timeline_track.rseditor/moxie_ui/src/glass/glass.wgsleditor/moxie_ui/src/glass/material.rseditor/moxie_ui/src/glass/mod.rseditor/moxie_ui/src/icons.rseditor/moxie_ui/src/lib.rseditor/moxie_ui/src/reactive.rseditor/moxie_ui/src/widgets.rseditor/moxie_ui/src/widgets/dock.rseditor/moxie_ui/src/widgets/dock/add_popup.rseditor/moxie_ui/src/widgets/dock/area.rseditor/moxie_ui/src/widgets/dock/drag.rseditor/moxie_ui/src/widgets/dock/reconcile.rseditor/moxie_ui/src/widgets/dock/registry.rseditor/moxie_ui/src/widgets/dock/split.rseditor/moxie_ui/src/widgets/dock/tabs.rseditor/moxie_ui/src/widgets/dock/tree.rseditor/moxie_ui/src/widgets/glass_backdrop.rseditor/moxie_ui/src/widgets/inspector.rseditor/moxie_ui/src/widgets/inspector/widget.rs
💤 Files with no reviewable changes (2)
- editor/moxie_ui/src/glass/mod.rs
- editor/moxie/src/scene.rs
🛑 Comments failed to post (6)
editor/moxie_ui/src/glass/glass.wgsl (1)
46-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor the zero-feather setting.
extra.x == 0.0still creates a soft edge becausemax(extra.x, 1.0)forces a one-pixel feather. Use a hard mask whenextra.xis zero. This preserves the documented crisp-edge mode.Proposed fix
- let shape = 1.0 - smoothstep(-max(extra.x, 1.0), 0.5, d); + let shape = if extra.x > 0.0 { + 1.0 - smoothstep(-extra.x, 0.0, d) + } else { + 1.0 - step(0.0, d) + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/glass/glass.wgsl` at line 46, Update the shape calculation around the WGSL `shape` expression to use a hard mask when `extra.x` is zero, while retaining the existing smooth feathering behavior for nonzero values. Ensure the zero-feather path does not force a one-pixel feather through `max(extra.x, 1.0)`.editor/moxie_ui/src/widgets/dock/drag.rs (1)
88-113: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard
on_tab_drag_startagainst overwriting an active drag.
on_tab_drag_startassigns*drag_state = DockDragState::PendingDrag { .. }unconditionally. If aPointer<DragStart>arrives while the state is alreadyDragging(a second pointer, a multi-touch drag, or aDragStartwithout a matchingDragEnd), the previousghost_entityandoverlay_entityare dropped without a despawn, and the previoussource_tabkeepsVisibility::Hidden.The result is a leaked ghost entity, a leaked overlay entity, and a tab that stays invisible until the leaf is rebuilt. The dock supports exactly one drag at a time, so ignore the new
DragStartwhile a drag is in flight.🐛 Proposed fix
let entity = trigger.event_target(); let Ok(tab) = tabs.get(entity) else { return }; + // One drag at a time: a second pointer must not orphan the + // in-flight ghost/overlay or leave `source_tab` hidden. + if !matches!(*drag_state, DockDragState::Idle) { + return; + } + let display_name = registry📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn on_tab_drag_start( trigger: On<Pointer<DragStart>>, tabs: Query<&super::area::DockTab>, mut drag_state: ResMut<DockDragState>, registry: Res<WindowRegistry>, ui_scale: Res<UiScale>, ) { let entity = trigger.event_target(); let Ok(tab) = tabs.get(entity) else { return }; // One drag at a time: a second pointer must not orphan the // in-flight ghost/overlay or leave `source_tab` hidden. if !matches!(*drag_state, DockDragState::Idle) { return; } let display_name = registry .get(&tab.window_id) .map(|d| d.name.clone()) .unwrap_or_else(|| tab.window_id.clone()); *drag_state = DockDragState::PendingDrag { source_tab: entity, tab_id: tab.tab_id, window_id: tab.window_id.clone(), window_name: display_name, start_pos: Vec2::new( trigger.event().pointer_location.position.x, trigger.event().pointer_location.position.y, ) / ui_scale.0, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/drag.rs` around lines 88 - 113, Update on_tab_drag_start to return immediately when drag_state is already in an active drag state, especially DockDragState::Dragging, before resolving the tab or assigning PendingDrag. Preserve the existing initialization for idle or pending states as appropriate, ensuring an in-flight drag’s ghost_entity, overlay_entity, and hidden source tab remain intact.editor/moxie_ui/src/widgets/dock/split.rs (1)
207-230: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear the cursor override before the
Nodelookup, and do not depend on the flex direction being unchanged.
on_handle_drag_endreturns at Line 223 if the parent has noNode. It also recomputescursor_iconfrom the parent's currentflex_directionat Line 226. If the parent is despawned, loses itsNode, or is re-oriented by the dock reconciler during the drag, the equality check at Line 227 fails andoverride_cursorkeeps the resize icon for the rest of the session. No later code clears it, becauseon_handle_drag_startonly sets the override when it isNone.Clear the override when it holds either resize icon.
🐛 Proposed fix
fn on_handle_drag_end( trigger: On<Pointer<DragEnd>>, handles: Query<&ChildOf, With<PanelHandle>>, - nodes: Query<&Node>, mut override_cursor: ResMut<OverrideCursor>, mut commands: Commands, ) { let handle = trigger.event_target(); - let Ok(&ChildOf(parent)) = handles.get(handle) else { + if handles.get(handle).is_err() { return; - }; + } // End of drag: drop the marker and clear the highlight. commands .entity(handle) .remove::<HandleDragging>() .insert(BackgroundColor(Color::NONE)); - let Ok(node) = nodes.get(parent) else { - return; - }; - let cursor_icon = get_drag_icon(node.flex_direction); - if override_cursor.0 == Some(EntityCursor::System(cursor_icon)) { + // Either resize icon: the group may have been re-oriented mid-drag. + let owned = matches!( + override_cursor.0, + Some(EntityCursor::System( + SystemCursorIcon::ColResize | SystemCursorIcon::RowResize + )) + ); + if owned { override_cursor.0 = None; } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn on_handle_drag_end( trigger: On<Pointer<DragEnd>>, handles: Query<&ChildOf, With<PanelHandle>>, mut override_cursor: ResMut<OverrideCursor>, mut commands: Commands, ) { let handle = trigger.event_target(); if handles.get(handle).is_err() { return; } // End of drag: drop the marker and clear the highlight. commands .entity(handle) .remove::<HandleDragging>() .insert(BackgroundColor(Color::NONE)); // Either resize icon: the group may have been re-oriented mid-drag. let owned = matches!( override_cursor.0, Some(EntityCursor::System( SystemCursorIcon::ColResize | SystemCursorIcon::RowResize )) ); if owned { override_cursor.0 = None; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/dock/split.rs` around lines 207 - 230, Update on_handle_drag_end to clear override_cursor before looking up the parent Node, so cleanup still occurs when the parent is missing or reconfigured. Remove the dependency on the current flex_direction and clear the override whenever it contains either supported resize cursor icon, while preserving the existing drag-marker cleanup.editor/moxie_ui/src/widgets/inspector.rs (1)
361-379: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fall back to a readable label when the reflect path is empty.
labelis the raw reflect path.collect_leavespushespath: prefix.to_string()at Line 243, andprefixis""when the target's own type has a registeredReflectInspect. The row then renders an emptyText, so the user sees a widget with no name. A nested tuple field also renders as a bare index, for example0.Use the last path segment, and fall back to the type name when the path is empty.
🐛 Proposed fix for the empty label
let field = Field::new(target, leaf.path.clone()); - let label = leaf.path; + let label = leaf + .path + .rsplit('.') + .next() + .filter(|segment| !segment.is_empty()) + .map(str::to_string) + .unwrap_or_else(|| "value".to_string());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let field = Field::new(target, leaf.path.clone()); let label = leaf .path .rsplit('.') .next() .filter(|segment| !segment.is_empty()) .map(str::to_string) .unwrap_or_else(|| "value".to_string()); ui.bsn(bsn! { Node { width: Val::Percent(100.0), flex_direction: FlexDirection::Row, justify_content: JustifyContent::SpaceBetween, align_items: AlignItems::Center, column_gap: Val::Px(8.0), padding: UiRect::vertical(Val::Px(2.0)), } }) .with(move |ui| { ui.bsn(bsn! { Text({label}) ThemedText TextFont { font_size: FontSize::Px(12.0) } }); drawer.build(&field, ui);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/inspector.rs` around lines 361 - 379, Update the label construction in the inspector row around Field::new and collect_leaves so it uses the final reflect-path segment, rather than the full raw path. When the path is empty, fall back to the target’s reflected type name; preserve readable nested labels instead of rendering bare or empty text.editor/moxie_ui/src/widgets/inspector/widget.rs (2)
87-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect glass_checkbox to see which entity emits ValueChange. fd -e rs | xargs rg -n -A25 'fn glass_checkbox' rg -n -C3 'ValueChange|Checked' --type=rs -g '!**/inspector/**'Repository: voxell-tech/motiongfx
Length of output: 2016
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files '*.rs' | sed -n '1,120p' echo "== locate glass checkbox and checked/value change ==" rg -n -C4 'glass_checkbox|GlassCheckMark|Checked|ValueChange|impl Scene for Checkbox|entity_mut\(.*\.observe|observe\(' editor || true echo "== relevant widget files ==" for f in editor/moxie_ui/src/widgets/inspector/widget.rs editor/moxie_ui/src/glass/widget.rs editor/moxie_ui/src/inspector.rs; do if [ -f "$f" ]; then echo "--- $f ---" wc -l "$f" sed -n '1,240p' "$f" fi doneRepository: voxell-tech/motiongfx
Length of output: 26777
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== all files ==" git ls-files | sed -n '1,120p' echo "== locate glass checkbox and checked/value change ==" rg -n -C4 'glass_checkbox|GlassCheckMark|Checked|ValueChange|impl Scene for Checkbox|entity_mut\(.*\.observe|observe\(' . || true echo "== relevant widget files ==" for f in editor/moxie_ui/src/widgets/inspector/widget.rs editor/moxie_ui/src/glass/widget.rs editor/moxie_ui/src/inspector.rs; do if [ -f "$f" ]; then echo "--- $f ---" wc -l "$f" sed -n '1,260p' "$f" fi doneRepository: voxell-tech/motiongfx
Length of output: 28329
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Cargo lock bevy version ==" python3 - <<'PY' from pathlib import Path text = Path("Cargo.lock").read_text() for block in text.split("\n\n"): if "name = \"bevy\"" in block: print(block) break PY echo "== Field implementation search ==" rg -n -C5 'struct Field|impl Field|fn set\(.*World|fn set\(&self|fn target\(|target_changed|class Field|deref.*Field|marked_changed|Changed' editor/moxie_ui/src editor/moxie/src || true echo "== inspector.rs lines ==" if [ -f editor/moxie_ui/src/widgets/inspector.rs ]; then wc -l editor/moxie_ui/src/widgets/inspector.rs sed -n '1,260p' editor/moxie_ui/src/widgets/inspector.rs fiRepository: voxell-tech/motiongfx
Length of output: 18899
Remove the optimistic
Checkedwrite, or correct the comment.The comment says the checkbox state only moves after the write lands, but the observer inserts/removes
Checkedimmediately after queuingfield.set. IfField::setcannot resolve the path or the value type mismatches, it only logs a warning, so the checkbox can show a value the inspector target does not hold. Thebind_rawbinding already keepsCheckedin sync after a successful write, which is consistent withnumber_field.Use
nodefor the write too ifCheckedis owned by theCheckboxroot;change.sourcemay be the toggle descendant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/inspector/widget.rs` around lines 87 - 118, Remove the immediate Checked insert/remove operations from the ValueChange observer and let bind_raw synchronize the checkbox after a successful field.set, keeping the observer focused on queuing the write. Ensure the queued write targets the Checkbox root node by using node rather than change.source where the entity is required, and update the misleading controlled-state comment accordingly.
197-200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp the
u64value on the way into the input.The comment states that these types clamp so that
asdoes not wrap or truncate. Only the inbound directionto_fieldclamps. The outbound directionto_inputforu64is|value| value as i64, which wraps for any value abovei64::MAX.u64::MAXthen displays as-1. If the user commits that displayed value,to_fieldmaps-1to0and the target loses its value.Saturate in both directions.
🐛 Proposed fix for the `u64` round trip
- u64 => I64, I64, i64, |value: i64| value.max(0) as u64, |value| value as i64; + u64 => I64, I64, i64, |value: i64| value.max(0) as u64, |value: u64| value.min(i64::MAX as u64) as i64;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// There is no unsigned input format, so these ride an `i64` and // clamp on the way back - `as` alone would wrap or truncate. u32 => I64, I64, i64, |value: i64| value.clamp(0, u32::MAX as i64) as u32, |value| value as i64; u64 => I64, I64, i64, |value: i64| value.max(0) as u64, |value: u64| value.min(i64::MAX as u64) as i64;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/inspector/widget.rs` around lines 197 - 200, Update the u64 conversion entry so its outbound to_input closure saturates values above i64::MAX instead of casting and wrapping; preserve the existing inbound nonnegative clamp, ensuring both directions saturate and u64::MAX displays as i64::MAX without losing the target value on round trip.
049fd98 to
8740393
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/moxie_ui/src/widgets/inspector/widget.rs`:
- Line 200: Update the u64 registration in the widget control mapping to avoid
converting through i64, which corrupts values above i64::MAX. Use an unsigned
decimal control that parses and preserves the full u64 range; otherwise remove
the u64 registration until such support exists, while retaining the existing
nonnegative edit behavior only where the type is safely supported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b7e3afe-8dce-4092-b516-df8535d5354b
📒 Files selected for processing (34)
editor/moxie/src/lib.rseditor/moxie/src/playback.rseditor/moxie/src/scene.rseditor/moxie/src/ui.rseditor/moxie/src/ui/hierarchy.rseditor/moxie/src/ui/timeline.rseditor/moxie/src/view.rseditor/moxie_ui/examples/dock_demo.rseditor/moxie_ui/src/elements.rseditor/moxie_ui/src/elements/divider.rseditor/moxie_ui/src/elements/frame.rseditor/moxie_ui/src/elements/ghost_button.rseditor/moxie_ui/src/elements/label.rseditor/moxie_ui/src/elements/playhead.rseditor/moxie_ui/src/elements/timeline_track.rseditor/moxie_ui/src/glass/glass.wgsleditor/moxie_ui/src/glass/material.rseditor/moxie_ui/src/glass/mod.rseditor/moxie_ui/src/icons.rseditor/moxie_ui/src/lib.rseditor/moxie_ui/src/reactive.rseditor/moxie_ui/src/widgets.rseditor/moxie_ui/src/widgets/dock.rseditor/moxie_ui/src/widgets/dock/add_popup.rseditor/moxie_ui/src/widgets/dock/area.rseditor/moxie_ui/src/widgets/dock/drag.rseditor/moxie_ui/src/widgets/dock/reconcile.rseditor/moxie_ui/src/widgets/dock/registry.rseditor/moxie_ui/src/widgets/dock/split.rseditor/moxie_ui/src/widgets/dock/tabs.rseditor/moxie_ui/src/widgets/dock/tree.rseditor/moxie_ui/src/widgets/glass_backdrop.rseditor/moxie_ui/src/widgets/inspector.rseditor/moxie_ui/src/widgets/inspector/widget.rs
💤 Files with no reviewable changes (2)
- editor/moxie_ui/src/glass/mod.rs
- editor/moxie/src/scene.rs
🚧 Files skipped from review as they are similar to previous changes (28)
- editor/moxie/src/playback.rs
- editor/moxie_ui/src/widgets/glass_backdrop.rs
- editor/moxie_ui/src/glass/material.rs
- editor/moxie_ui/src/widgets/dock/reconcile.rs
- editor/moxie_ui/src/elements/timeline_track.rs
- editor/moxie_ui/src/widgets/dock.rs
- editor/moxie_ui/src/elements/playhead.rs
- editor/moxie/src/view.rs
- editor/moxie_ui/src/icons.rs
- editor/moxie_ui/src/widgets.rs
- editor/moxie_ui/src/elements/divider.rs
- editor/moxie_ui/src/glass/glass.wgsl
- editor/moxie_ui/src/elements/frame.rs
- editor/moxie_ui/src/widgets/dock/add_popup.rs
- editor/moxie_ui/src/elements/ghost_button.rs
- editor/moxie_ui/src/widgets/dock/tabs.rs
- editor/moxie_ui/src/reactive.rs
- editor/moxie_ui/examples/dock_demo.rs
- editor/moxie/src/ui/hierarchy.rs
- editor/moxie_ui/src/widgets/dock/split.rs
- editor/moxie_ui/src/widgets/dock/drag.rs
- editor/moxie_ui/src/elements/label.rs
- editor/moxie/src/ui.rs
- editor/moxie/src/lib.rs
- editor/moxie_ui/src/widgets/dock/area.rs
- editor/moxie_ui/src/widgets/inspector.rs
- editor/moxie_ui/src/widgets/dock/tree.rs
- editor/moxie/src/ui/timeline.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/moxie_ui/src/widgets/inspector/widget.rs`:
- Line 200: Update the u64 registration in the widget control mapping to avoid
converting through i64, which corrupts values above i64::MAX. Use an unsigned
decimal control that parses and preserves the full u64 range; otherwise remove
the u64 registration until such support exists, while retaining the existing
nonnegative edit behavior only where the type is safely supported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b7e3afe-8dce-4092-b516-df8535d5354b
📒 Files selected for processing (34)
editor/moxie/src/lib.rseditor/moxie/src/playback.rseditor/moxie/src/scene.rseditor/moxie/src/ui.rseditor/moxie/src/ui/hierarchy.rseditor/moxie/src/ui/timeline.rseditor/moxie/src/view.rseditor/moxie_ui/examples/dock_demo.rseditor/moxie_ui/src/elements.rseditor/moxie_ui/src/elements/divider.rseditor/moxie_ui/src/elements/frame.rseditor/moxie_ui/src/elements/ghost_button.rseditor/moxie_ui/src/elements/label.rseditor/moxie_ui/src/elements/playhead.rseditor/moxie_ui/src/elements/timeline_track.rseditor/moxie_ui/src/glass/glass.wgsleditor/moxie_ui/src/glass/material.rseditor/moxie_ui/src/glass/mod.rseditor/moxie_ui/src/icons.rseditor/moxie_ui/src/lib.rseditor/moxie_ui/src/reactive.rseditor/moxie_ui/src/widgets.rseditor/moxie_ui/src/widgets/dock.rseditor/moxie_ui/src/widgets/dock/add_popup.rseditor/moxie_ui/src/widgets/dock/area.rseditor/moxie_ui/src/widgets/dock/drag.rseditor/moxie_ui/src/widgets/dock/reconcile.rseditor/moxie_ui/src/widgets/dock/registry.rseditor/moxie_ui/src/widgets/dock/split.rseditor/moxie_ui/src/widgets/dock/tabs.rseditor/moxie_ui/src/widgets/dock/tree.rseditor/moxie_ui/src/widgets/glass_backdrop.rseditor/moxie_ui/src/widgets/inspector.rseditor/moxie_ui/src/widgets/inspector/widget.rs
💤 Files with no reviewable changes (2)
- editor/moxie_ui/src/glass/mod.rs
- editor/moxie/src/scene.rs
🚧 Files skipped from review as they are similar to previous changes (28)
- editor/moxie/src/playback.rs
- editor/moxie_ui/src/widgets/glass_backdrop.rs
- editor/moxie_ui/src/glass/material.rs
- editor/moxie_ui/src/widgets/dock/reconcile.rs
- editor/moxie_ui/src/elements/timeline_track.rs
- editor/moxie_ui/src/widgets/dock.rs
- editor/moxie_ui/src/elements/playhead.rs
- editor/moxie/src/view.rs
- editor/moxie_ui/src/icons.rs
- editor/moxie_ui/src/widgets.rs
- editor/moxie_ui/src/elements/divider.rs
- editor/moxie_ui/src/glass/glass.wgsl
- editor/moxie_ui/src/elements/frame.rs
- editor/moxie_ui/src/widgets/dock/add_popup.rs
- editor/moxie_ui/src/elements/ghost_button.rs
- editor/moxie_ui/src/widgets/dock/tabs.rs
- editor/moxie_ui/src/reactive.rs
- editor/moxie_ui/examples/dock_demo.rs
- editor/moxie/src/ui/hierarchy.rs
- editor/moxie_ui/src/widgets/dock/split.rs
- editor/moxie_ui/src/widgets/dock/drag.rs
- editor/moxie_ui/src/elements/label.rs
- editor/moxie/src/ui.rs
- editor/moxie/src/lib.rs
- editor/moxie_ui/src/widgets/dock/area.rs
- editor/moxie_ui/src/widgets/inspector.rs
- editor/moxie_ui/src/widgets/dock/tree.rs
- editor/moxie/src/ui/timeline.rs
🛑 Comments failed to post (1)
editor/moxie_ui/src/widgets/inspector/widget.rs (1)
200-200: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not convert the full
u64domain throughi64.Line 200 converts values above
i64::MAXto negativei64values. For example,u64::MAXdisplays as-1. A subsequent edit can then write a different value throughvalue.max(0) as u64.Use an unsigned decimal control that parses
u64, or do not registeru64until the control supports its full range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/moxie_ui/src/widgets/inspector/widget.rs` at line 200, Update the u64 registration in the widget control mapping to avoid converting through i64, which corrupts values above i64::MAX. Use an unsigned decimal control that parses and preserves the full u64 range; otherwise remove the u64 registration until such support exists, while retaining the existing nonnegative edit behavior only where the type is safely supported.
2 participants
The goal here is to encapsulate ui elements and widgets into their own modules so private
consts,fns,structs stays private without leaking into unnecessary places.This PR also deletes most of the marker components where it's become useless because of the use of
moxie_ui_kernelbindings & watchers.