fix: added missing includes and add helper method to enable build#1135
Conversation
WalkthroughThe changes refactor how the error color for UI rendering is accessed by introducing a public accessor method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Feature
participant Menu
User->>Feature: Trigger DrawUnloadedUI()
Feature->>Menu: Menu::GetSingleton()
Feature->>Menu: GetTheme()
Menu-->>Feature: Return ThemeSettings (read-only)
Feature->>Feature: Access StatusPalette.Error from ThemeSettings
Feature->>UI: ImGui::TextColored(errorColor, "This feature is not installed!")
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
soda3000
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Menu.h (1)
182-182: Accessor could be safer & clearer with[[nodiscard]]+noexcept
Returning a const reference is fine, but marking the functionnoexceptand[[nodiscard]]prevents accidental misuse and advertises that the call is side-effect free.-const ThemeSettings& GetTheme() const { return settings.Theme; } +[[nodiscard]] const ThemeSettings& GetTheme() const noexcept { return settings.Theme; }Optional, but a cheap win.
src/Feature.h (1)
3-5: Superfluous/expensive includes
DrawUnloadedUIonly needs theMenudeclaration andImVec4; the fullMenu.h(≈1 k-loc) drags extra headers into every TU that includesFeature.h.Consider:
#include "Globals.h" // Still required? class Menu; // forward declarationand move the
#include "Menu.h"to the implementation file that actually callsMenu::GetSingleton().
This shaves compile time and avoids unnecessary rebuilds whenMenu.hchanges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Feature.h(2 hunks)src/Menu.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
src/Feature.h (1)
60-60: Theme access now properly encapsulated – nice
UsingMenu::GetSingleton()->GetTheme()removes the writeable global and respects the new accessor.
|
|
||
| #include "Utils/Serialize.h" | ||
| #include <dxgi1_4.h> | ||
| #include <winrt/Base.h> |
There was a problem hiding this comment.
Incorrect case in winrt include may break builds on case-sensitive filesystems
The Windows C++/WinRT header is winrt/base.h (all lower-case). Using Base.h compiles on the default Windows file-system but fails on case-sensitive environments (WSL, CI containers, cross-platform tool-chains).
-#include <winrt/Base.h>
+#include <winrt/base.h>🤖 Prompt for AI Agents
In src/Menu.h at line 5, the include directive uses incorrect casing for the
Windows C++/WinRT header. Change the include from #include <winrt/Base.h> to
#include <winrt/base.h> using all lowercase letters to ensure compatibility with
case-sensitive filesystems.
There was a problem hiding this comment.
pretty sure that's okay
There was a problem hiding this comment.
@soda3000 It usually builds on a default Windows workstation because NTFS is case-insensitive, but any workflow that compiles the project on a case-sensitive file-system (e.g. WSL, Linux CI runner, macOS, or even Windows with the optional case-sensitive flag enabled) will look for the literal path and fail to find Base.h.
Microsoft ships the header as
…/cppwinrt/winrt/base.h
(all lower-case) in both the VS/BuildTools and vcpkg layouts.
Compilers such as Clang/LLVM on a case-sensitive volume therefore error with:
fatal error: 'winrt/Base.h' file not found
Switching to #include <winrt/base.h> keeps Windows builds unchanged while preventing those portability breaks. Let me know if you’d like me to submit the one-line fix, or if you prefer to leave it as-is and we can revisit later.
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
At least it builds. Thanks.
* Fixed includes and encapsulation to build * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Fixed includes and encapsulation to build * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Fixed includes and encapsulation to build * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2 participants
Wasn't able to build without adjusting these few missing includes and also since settings is private, I added a helper method to get the ThemeSettings publicly.
Summary by CodeRabbit