Reduce QtWidgets dependency#147
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe change introduces 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@framework/ui/uimodule.cpp`:
- Around line 81-82: Construct m_widgetStyle only on the GUI initialization
path, rather than unconditionally in registerExports(). Update the relevant
GuiApp branch in onAllInited() or init() so WidgetStyle is created before
QApplication::setStyle(this) and remains available for GUI mode, while console
and test modes never allocate it.
In `@framework/ui/view/widgetstyle.cpp`:
- Around line 38-40: Update widgetstyle.cpp to include iconcodes.h directly
alongside its existing includes, ensuring the IconCode and iconCodeToChar
declarations used by the file are available without relying on transitive
headers.
In `@framework/uicomponents/qml/Muse/UiComponents/dialogview.cpp`:
- Around line 160-163: Update the content-size assignments in the geometry
update flow to use the frame-margin-removed rectangle, matching the existing
m_globalPos calculation. Set content width and height from
dlgRect.marginsRemoved(frameMargins) so viewGeometry() does not reintroduce
frame margins on subsequent updateGeometry() calls.
- Around line 134-135: Remove the redundant moveLeft and moveTop calls from the
dialog rectangle positioning logic in dialogview.cpp, leaving the existing
dlgRect calculation and subsequent 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00586041-16c3-4eb7-827e-a4e8fdd5023e
📒 Files selected for processing (20)
framework/global/CMakeLists.txtframework/interactive/iinteractiveuriregister.hframework/tours/internal/toursprovider.cppframework/ui/CMakeLists.txtframework/ui/api/themeapi.cppframework/ui/api/themeapi.hframework/ui/internal/uiengine.cppframework/ui/internal/uiengine.hframework/ui/iuiengine.hframework/ui/qml/Muse/Ui/navigationsection.cppframework/ui/uimodule.cppframework/ui/uimodule.hframework/ui/view/widgetstyle.cppframework/ui/view/widgetstyle.hframework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/dialogview.cppframework/uicomponents/qml/Muse/UiComponents/internal/popupviewclosecontroller.cppframework/uicomponents/qml/Muse/UiComponents/itemmultiselectionmodel.cppframework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/vst/CMakeLists.txt
💤 Files with no reviewable changes (3)
- framework/ui/iuiengine.h
- framework/ui/internal/uiengine.cpp
- framework/ui/internal/uiengine.h
| m_theme = std::make_shared<api::ThemeApi>(nullptr); | ||
| m_widgetStyle = new WidgetStyle(m_theme); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
m_widgetStyle leaks in non-GUI run modes.
m_widgetStyle is allocated unconditionally in registerExports(), but ownership is only transferred to QApplication when init() → QApplication::setStyle(this) runs, and that only happens for GuiApp mode (see onAllInited() early-return at Lines 139-141). In console/test run modes the style is never adopted and is never deleted (raw pointer, no onDeinit cleanup), leaking both the WidgetStyle and its retained ThemeApi reference.
Consider constructing it only on the GUI path:
🔧 Proposed fix
m_theme = std::make_shared<api::ThemeApi>(nullptr);
- m_widgetStyle = new WidgetStyle(m_theme); m_theme->init();
- m_widgetStyle->init();
+
+ m_widgetStyle = new WidgetStyle(m_theme); // becomes owned by QApplication
+ m_widgetStyle->init();📝 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.
| m_theme = std::make_shared<api::ThemeApi>(nullptr); | |
| m_widgetStyle = new WidgetStyle(m_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 `@framework/ui/uimodule.cpp` around lines 81 - 82, Construct m_widgetStyle only
on the GUI initialization path, rather than unconditionally in
registerExports(). Update the relevant GuiApp branch in onAllInited() or init()
so WidgetStyle is created before QApplication::setStyle(this) and remains
available for GUI mode, while console and test modes never allocate it.
| m_globalPos = dlgRect.marginsRemoved(frameMargins).topLeft(); | ||
|
|
||
| setContentWidth(dlgRect.width()); | ||
| setContentHeight(dlgRect.height()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Content size set from frame-inclusive dlgRect — dialog will grow on each geometry update.
dlgRect is built at line 127–128 by adding frameMargins to the content geometry. After positioning and clamping, m_globalPos correctly removes the margins (line 160), but setContentWidth(dlgRect.width()) and setContentHeight(dlgRect.height()) (lines 162–163) still include the frame margins. Since viewGeometry() (line 166–168) reads back contentWidth/contentHeight, the next call to updateGeometry() will add the frame margins again, causing the dialog to grow by frameMargins.left() + frameMargins.right() (width) and frameMargins.top() + frameMargins.bottom() (height) on every invocation.
The old code only adjusted dlgRect height with a fixed title-bar constant, so width was unaffected. The new marginsAdded(frameMargins) adds horizontal margins too, making this a regression.
🐛 Proposed fix: remove margins before setting content size
m_globalPos = dlgRect.marginsRemoved(frameMargins).topLeft();
-setContentWidth(dlgRect.width());
-setContentHeight(dlgRect.height());
+QRect contentRect = dlgRect.marginsRemoved(frameMargins);
+setContentWidth(contentRect.width());
+setContentHeight(contentRect.height());📝 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.
| m_globalPos = dlgRect.marginsRemoved(frameMargins).topLeft(); | |
| setContentWidth(dlgRect.width()); | |
| setContentHeight(dlgRect.height()); | |
| m_globalPos = dlgRect.marginsRemoved(frameMargins).topLeft(); | |
| QRect contentRect = dlgRect.marginsRemoved(frameMargins); | |
| setContentWidth(contentRect.width()); | |
| setContentHeight(contentRect.height()); |
🤖 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 `@framework/uicomponents/qml/Muse/UiComponents/dialogview.cpp` around lines 160
- 163, Update the content-size assignments in the geometry update flow to use
the frame-margin-removed rectangle, matching the existing m_globalPos
calculation. Set content width and height from
dlgRect.marginsRemoved(frameMargins) so viewGeometry() does not reintroduce
frame margins on subsequent updateGeometry() calls.
cbjeukendrup
force-pushed
the
reduce-qtwidgets-dependency
branch
2 times, most recently
from
302d6f5 to
06c063e
Compare
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 `@framework/ui/api/themeapi.cpp`:
- Line 25: Remove the unused widgetstyle.h include from ThemeApi so the theme
implementation no longer depends on the QtWidgets-facing WidgetStyle layer.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83fbebd6-8746-4fb9-9eb5-a33ab0cd97a2
📒 Files selected for processing (20)
framework/global/CMakeLists.txtframework/interactive/iinteractiveuriregister.hframework/tours/internal/toursprovider.cppframework/ui/CMakeLists.txtframework/ui/api/themeapi.cppframework/ui/api/themeapi.hframework/ui/internal/uiengine.cppframework/ui/internal/uiengine.hframework/ui/iuiengine.hframework/ui/qml/Muse/Ui/navigationsection.cppframework/ui/uimodule.cppframework/ui/uimodule.hframework/ui/view/widgetstyle.cppframework/ui/view/widgetstyle.hframework/uicomponents/qml/Muse/UiComponents/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/dialogview.cppframework/uicomponents/qml/Muse/UiComponents/internal/popupviewclosecontroller.cppframework/uicomponents/qml/Muse/UiComponents/itemmultiselectionmodel.cppframework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/vst/CMakeLists.txt
💤 Files with no reviewable changes (3)
- framework/ui/iuiengine.h
- framework/ui/internal/uiengine.cpp
- framework/ui/internal/uiengine.h
Instead of hardcoding some value and taking some other value from QApplication (QtWidgets), we can just use the dedicated QWindow methods for this.
The comment about PCH is not relevant anymore, because PCH is nowadays its own target and does not include QtWidgets headers anymore.
cbjeukendrup
force-pushed
the
reduce-qtwidgets-dependency
branch
from
06c063e to
aae16c1
Compare
2 participants
No description provided.