Migrate to trait based inspector - #127
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe editor inspector now supports resource and component targets through reflection. Registered ChangesInspector generalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EditorPlugin
participant InspectorFields
participant InspectorTarget
participant TypeRegistry
participant InspectorWidget
EditorPlugin->>TypeRegistry: register_default_inspects()
EditorPlugin->>InspectorFields: pass resource target
InspectorFields->>InspectorTarget: resolve reflected resource
InspectorTarget->>TypeRegistry: access registered inspector
TypeRegistry->>InspectorWidget: build field widget
InspectorWidget->>InspectorTarget: read and write field values
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
editor/motiongfx_editor_ui/src/inspector/widget.rs (1)
95-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment does not match the behavior.
The comment says the checkbox state moves only after the write lands. The code inserts or removes
Checkedunconditionally, in the same observer, without checking thatfield.setsucceeded. If the path no longer resolves,field.setonly logs a warning, and the checkbox still flips until the nexttarget_changedpoll corrects it.Either drop the claim from the comment, or move the
Checkedupdate into the queued command after a successful write.🤖 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/motiongfx_editor_ui/src/inspector/widget.rs` around lines 95 - 105, Update the observer around field.set and the Checked component so the checkbox state changes only after a successful write, moving the Checked insert/remove into the queued command that follows a successful field.set; alternatively, revise the misleading comment to describe the current unconditional behavior. Keep target_changed correction behavior unchanged.editor/motiongfx_editor_ui/src/inspector.rs (2)
350-382: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe row label is the raw reflect path.
labelisleaf.pathverbatim. For a nested leaf the panel showsphysical_size.x, and for a tuple-struct leaf it shows a bare index such as0. Both read poorly in the settings panel.Consider labelling with the last path segment and keeping the full path as a tooltip or debug detail.
🤖 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/motiongfx_editor_ui/src/inspector.rs` around lines 350 - 382, Update the label construction in build_fields so nested leaves display only the final path segment rather than the full leaf.path, while tuple-struct indices receive a readable field label instead of a bare number. Preserve the complete path separately for a tooltip or debug detail if the existing UI supports one.
249-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFields of unsupported reflect kinds disappear without a trace.
collect_leavesdescends intoStructandTupleStructonly. An enum, list, map, tuple, or opaque field with no registeredReflectInspectmatches the_ => {}arm, so the inspector renders no row and logs nothing. A user cannot tell the field from a field that does not exist.Consider a
debug!on the fallthrough arm with the path and type name. It keeps the walk unchanged and makes a missing row explainable.🤖 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/motiongfx_editor_ui/src/inspector.rs` around lines 249 - 279, The fallthrough arm in collect_leaves currently drops unsupported reflected fields silently. Add a debug-level log there that includes the current path and reflected type name, while leaving traversal behavior unchanged for Struct and TupleStruct and retaining the empty fallthrough logic.
🤖 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/motiongfx_editor_ui/src/inspector.rs`:
- Around line 72-96: Update accessor and the related read/write/changed_tick
paths to resolve reflection data according to the InspectorTarget variant: use
ReflectResource for Resource targets and ReflectComponent for Component targets.
Preserve the existing cloned accessor behavior and ensure resources annotated
only with #[reflect(Resource)] remain supported as documented.
In `@editor/motiongfx_editor_ui/src/inspector/widget.rs`:
- Around line 197-201: Update the u32 conversion closure in the numeric format
mappings to clamp the i64 input to the full u32 range before casting, preventing
values above u32::MAX from truncating. Keep the existing lower-bound behavior
and use the same shared clamp approach for the u64 row so both conversions
remain uniform.
---
Nitpick comments:
In `@editor/motiongfx_editor_ui/src/inspector.rs`:
- Around line 350-382: Update the label construction in build_fields so nested
leaves display only the final path segment rather than the full leaf.path, while
tuple-struct indices receive a readable field label instead of a bare number.
Preserve the complete path separately for a tooltip or debug detail if the
existing UI supports one.
- Around line 249-279: The fallthrough arm in collect_leaves currently drops
unsupported reflected fields silently. Add a debug-level log there that includes
the current path and reflected type name, while leaving traversal behavior
unchanged for Struct and TupleStruct and retaining the empty fallthrough logic.
In `@editor/motiongfx_editor_ui/src/inspector/widget.rs`:
- Around line 95-105: Update the observer around field.set and the Checked
component so the checkbox state changes only after a successful write, moving
the Checked insert/remove into the queued command that follows a successful
field.set; alternatively, revise the misleading comment to describe the current
unconditional behavior. Keep target_changed correction behavior unchanged.
🪄 Autofix (Beta)
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: 5f58721f-3709-4706-903c-de81a06e2e7c
📒 Files selected for processing (4)
editor/motiongfx_editor/src/lib.rseditor/motiongfx_editor/src/scene.rseditor/motiongfx_editor_ui/src/inspector.rseditor/motiongfx_editor_ui/src/inspector/widget.rs
| /// The accessor for this target's type, cloned out so the registry | ||
| /// guard is never held while the caller runs. Nesting two read | ||
| /// guards on one thread can deadlock the moment a writer queues | ||
| /// between them, and callers here reach for the registry again. | ||
| fn accessor(&self, world: &World) -> Option<ReflectComponent> { | ||
| let registry = world.resource::<AppTypeRegistry>().read(); | ||
| registry | ||
| .get_type_data::<ReflectComponent>(self.type_id()) | ||
| .cloned() | ||
| } | ||
|
|
||
| /// Runs `read` against the value, or returns `None` when the | ||
| /// target is gone or its type was never registered with | ||
| /// `#[reflect(Component)]` / `#[reflect(Resource)]`. | ||
| pub fn read<R>( | ||
| &self, | ||
| world: &World, | ||
| read: impl FnOnce(&dyn Reflect) -> R, | ||
| ) -> Option<R> { | ||
| let entity = self.entity(world)?; | ||
| let component = self.accessor(world)?; | ||
| let value = | ||
| component.reflect(world.get_entity(entity).ok()?)?; | ||
| Some(read(value)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how EditorSettings and other inspector targets are reflected.
fd -e rs . editor | xargs rg -n -B6 'struct EditorSettings'
# Any type registered with reflect(Resource) but not reflect(Component).
rg -n -C4 'reflect\((Resource|Component)' --type=rs editor
# Confirm ReflectResource is never consulted by the inspector.
rg -n 'ReflectResource|ReflectComponent' --type=rs editorRepository: voxell-tech/motiongfx
Length of output: 683
🏁 Script executed:
#!/bin/bash
set -e
echo "Files matching inspector.rs:"
fd -e rs 'inspector\.rs$' . || true
echo
echo "EditorSettings definition:"
fd -e rs 'lib\.rs$' . | xargs rg -n -B8 'struct EditorSettings' || true
echo
echo "Reflect annotations in editor:"
find . -type f -name '*.rs' | xargs rg -n -C4 'reflect\((Resource|Component|SettingsGroup)\)' || true
echo
echo "ReflectResource/ReflectComponent in editor:"
find . -type f -name '*.rs' | xargs rg -n 'ReflectResource|ReflectComponent' || true
echo
echo "inspector snippets if present:"
for f in $(fd -e rs 'inspector\.rs$' .); do
echo "--- $f ---"
sed -n '1,130p' "$f"
doneRepository: voxell-tech/motiongfx
Length of output: 7588
Use ReflectResource for the resource path.
accessor() always resolves ReflectComponent, so #[reflect(Resource)] targets such as EditorSettings return None through read()/write()/changed_tick(). The doc says either annotation works, but EditorSettings uses only #[reflect(Resource, SettingsGroup, Default)]. Guard by target variant: use ReflectResource for InspectorTarget::Resource (or document that resources must also use #[reflect(Component)]).
🤖 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/motiongfx_editor_ui/src/inspector.rs` around lines 72 - 96, Update
accessor and the related read/write/changed_tick paths to resolve reflection
data according to the InspectorTarget variant: use ReflectResource for Resource
targets and ReflectComponent for Component targets. Preserve the existing cloned
accessor behavior and ensure resources annotated only with #[reflect(Resource)]
remain supported as documented.
1 participant
No description provided.