Add contextual tool rail actions#437
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces context-sensitive toolbar rail buttons and centralizes mode-driven action visibility in MainWindow. It adds a helper to create reusable rail buttons for Dope Sheet, Curve Editor, Merge Animations, Material Editor, and Mesh Validation. The core ChangesToolbar Rail Buttons and Mode-Driven Visibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
fernandotonon
force-pushed
the
codex/contextual-tool-rail
branch
from
5f295d5 to
7c9b4ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/mainwindow.cpp (2)
2596-2618: ⚡ Quick win
updateToolRailForMode()is called redundantly insideupdateMergeAnimationsButton.
SelectionSet::selectionChangedis connected toupdateMergeAnimationsButtonfirst (line ~1401) and then directly toupdateToolRailForMode(line ~1403). Qt fires connections in registration order, soupdateToolRailForModealready executes afterupdateMergeAnimationsButtonhas updatedactionMerge_Animations->isEnabled()on every selection change. The three explicitupdateToolRailForMode()calls at lines 2600, 2612, and 2618 are therefore redundant and causeupdateToolRailForModeto iterate all toolbar actions twice per selection event.♻️ Proposed fix — remove the three redundant calls
if (skelEntities.size() < 2) { ui->actionMerge_Animations->setEnabled(false); - updateToolRailForMode(); return; } // Check all pairs are compatible Ogre::SkeletonPtr baseSkel = skelEntities.first()->getMesh()->getSkeleton(); for (int i = 1; i < skelEntities.size(); ++i) { Ogre::SkeletonPtr otherSkel = skelEntities[i]->getMesh()->getSkeleton(); if (!AnimationMerger::areSkeletonsCompatible(baseSkel, otherSkel)) { ui->actionMerge_Animations->setEnabled(false); - updateToolRailForMode(); return; } } ui->actionMerge_Animations->setEnabled(true); - updateToolRailForMode();🤖 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 `@src/mainwindow.cpp` around lines 2596 - 2618, The three explicit calls to updateToolRailForMode() inside updateMergeAnimationsButton are redundant because SelectionSet::selectionChanged is already connected to updateToolRailForMode and Qt will call it after updateMergeAnimationsButton; remove the updateToolRailForMode() calls that follow setting ui->actionMerge_Animations->setEnabled(...) (the ones after the early return when skelEntities.size() < 2, after detecting incompatible skeletons, and after enabling the action) so updateMergeAnimationsButton only sets ui->actionMerge_Animations and returns, leaving updateToolRailForMode() to be invoked by the existing signal connection.
1811-1815: 💤 Low value
setSharedToolbarActionVisible:visibleparameter does not control action visibility — name is misleading.
action->setVisible(true)is unconditionally called regardless ofvisible; the parameter only governs the toolbar-widget button. A caller passingfalsewould reasonably expect the action to be hidden, but it won't be. Consider renaming tosetSharedToolbarButtonVisibleor adding an inline comment clarifying the intent.♻️ Proposed clarification
- auto setSharedToolbarActionVisible = [this](QAction* action, bool visible) { - action->setVisible(true); + // Keeps the QAction reachable in menus (action always visible) while + // showing/hiding only its toolbar-button representation. + auto setSharedToolbarButtonVisible = [this](QAction* action, bool showButton) { + action->setVisible(true); // keep reachable in menus if (QWidget* toolbarButton = ui->toolToolbar->widgetForAction(action)) - toolbarButton->setVisible(visible); + toolbarButton->setVisible(showButton); };- setSharedToolbarActionVisible(ui->actionMaterial_Editor, objectMode || materialMode); - setSharedToolbarActionVisible(ui->actionMerge_Animations, objectMode || animationMode); + setSharedToolbarButtonVisible(ui->actionMaterial_Editor, objectMode || materialMode); + setSharedToolbarButtonVisible(ui->actionMerge_Animations, objectMode || animationMode);🤖 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 `@src/mainwindow.cpp` around lines 1811 - 1815, The lambda setSharedToolbarActionVisible currently forces the QAction visible regardless of the visible parameter, so change action->setVisible(true) to action->setVisible(visible) so the QAction visibility follows the boolean; ensure the ui->toolToolbar->widgetForAction(action) branch continues to set the toolbar widget visibility as before (toolbarButton->setVisible(visible)), and update any callers or rename the lambda if you prefer the original behavior to only affect the toolbar button.
🤖 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.
Nitpick comments:
In `@src/mainwindow.cpp`:
- Around line 2596-2618: The three explicit calls to updateToolRailForMode()
inside updateMergeAnimationsButton are redundant because
SelectionSet::selectionChanged is already connected to updateToolRailForMode and
Qt will call it after updateMergeAnimationsButton; remove the
updateToolRailForMode() calls that follow setting
ui->actionMerge_Animations->setEnabled(...) (the ones after the early return
when skelEntities.size() < 2, after detecting incompatible skeletons, and after
enabling the action) so updateMergeAnimationsButton only sets
ui->actionMerge_Animations and returns, leaving updateToolRailForMode() to be
invoked by the existing signal connection.
- Around line 1811-1815: The lambda setSharedToolbarActionVisible currently
forces the QAction visible regardless of the visible parameter, so change
action->setVisible(true) to action->setVisible(visible) so the QAction
visibility follows the boolean; ensure the
ui->toolToolbar->widgetForAction(action) branch continues to set the toolbar
widget visibility as before (toolbarButton->setVisible(visible)), and update any
callers or rename the lambda if you prefer the original behavior to only affect
the toolbar button.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8282ad56-84af-48a0-9397-d32f7f848c45
📒 Files selected for processing (2)
src/mainwindow.cppsrc/mainwindow_test.cpp
… rail refreshes - Rename `setSharedToolbarActionVisible` -> `setSharedToolbarButtonVisible` and add a comment explaining the contract: the QAction stays visible in menus (so users always find Material Editor / Merge Animations there), while only the toolbar-button widget is gated by mode. The previous parameter name suggested it controlled QAction visibility, but the body unconditionally set the action visible regardless of the flag. - Remove three redundant `updateToolRailForMode()` calls inside `updateMergeAnimationsButton()`. `SelectionSet::selectionChanged` is wired to `updateMergeAnimationsButton` first and `updateToolRailForMode` right after, so the rail is already refreshed for free after this slot completes — calling it here just iterated every toolbar action twice per selection event. Added a comment so the next reader sees why. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed both CodeRabbit findings in 119d199:
Validation:
|
|
1 participant
Summary
Validation
cmake --build build_local --target UnitTests -j"$(nproc)"env QT_QPA_PLATFORM=offscreen build_local/bin/UnitTests --gtest_filter=MainWindowTest.ContextualToolRail*:MainWindowTest.ModeBarLoadsAndModeChangeUpdatesStatusIndicatorcmake --build build_local --target QtMeshEditor -j"$(nproc)"Summary by CodeRabbit
Release Notes
New Features
Improvements