refactor(ui): improve menu layout and organization#1178
Conversation
WalkthroughThis update introduces a static map to track the number of features per category in the menu system. Category headers in the UI now display feature counts. The feature detail panel is reorganized into "Settings" and "About" tabs, and inline tooltips in the feature list are removed for a cleaner interface. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant Util
User->>Menu: Open Features Menu
Menu->>Menu: BuildCategoryCounts()
loop For each category
Menu->>Util: DrawCategoryHeader(categoryName, isExpanded, categoryCount)
end
User->>Menu: Select Feature
Menu->>Menu: Show Detail Panel (Tabbed: Settings/About)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
Documentation and Community
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![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 (1)
src/Menu.cpp (1)
587-588: Category count integration works correctly.The logic properly retrieves and passes category counts to the header drawing function. Minor optimization opportunity: consider using a string_view-compatible map or caching the string conversion if this becomes a performance concern.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Menu.cpp(5 hunks)src/Menu.h(3 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (9)
src/Menu.h (3)
6-6: LGTM! Include statement is appropriate.The inclusion of "Feature.h" is necessary for the categoryCounts functionality and follows proper dependency management.
293-293: LGTM! Well-encapsulated design.The private
BuildCategoryCounts()method follows good encapsulation practices by keeping the implementation details internal to the Menu class.
220-220: Consider thread safety for the static member.The static
categoryCountsmap looks well-designed for tracking feature counts per category. However, consider potential thread safety implications if this map could be accessed concurrently.#!/bin/bash # Description: Check if the categoryCounts map is accessed from multiple threads # Expected: Find usage patterns to assess thread safety requirements # Search for usage of categoryCounts rg -A 3 -B 3 "categoryCounts" # Look for threading-related code in the codebase rg -A 2 "std::thread|std::mutex|std::lock"src/Utils/UI.h (1)
137-140: LGTM! Function signature update is well-documented.The addition of the
categoryCountparameter is properly documented and the signature change is consistent with the implementation in UI.cpp.src/Utils/UI.cpp (2)
354-390: LGTM! Consistent implementation of display name usage.The implementation correctly uses the formatted
displayNameconsistently across all UI operations (text size calculation, ID generation, and text rendering). This ensures the category count is properly displayed in the interface.
345-348: Verify C++20 support for std::format usage.The implementation correctly integrates the category count into the display name. However,
std::formatrequires C++20 support.#!/bin/bash # Description: Check if the project is configured for C++20 support # Expected: Find C++20 configuration in build files # Search for C++ standard configuration rg -i "c\+\+20|cxx.*20|std.*20" --type cmake --type makefile fd -e cmake -e vcxproj -e props | xargs rg -l "c\+\+20|cxx.*20|std.*20" # Check for other std::format usage in codebase rg "std::format"src/Menu.cpp (3)
222-222: LGTM - Clean addition of category counting infrastructure.The static
categoryCountsmap is appropriately placed and typed for tracking feature counts per category.
295-295: LGTM - Proper initialization placement.Calling
BuildCategoryCounts()duringInit()ensures category counts are populated before UI rendering begins.
659-798: Excellent UI refactor - significant improvement in organization.The tabbed interface is a major improvement over the previous single-panel layout:
✅ Proper separation of concerns: Settings vs. About information
✅ Correct ImGui implementation: Proper tab bar structure and child windows
✅ Enhanced UX: Better organization of feature management controls and information
✅ Consistent styling: Proper use of theme colors and status indicatorsThis refactor aligns perfectly with the PR objectives to improve menu layout and organization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Menu.cpp (2)
587-588: Optimize map lookup and verify count accuracy.Two potential improvements:
Performance: Line 587 creates an unnecessary temporary string. Consider using
find()or storing header.name asstd::stringto avoid conversion.Accuracy:
BuildCategoryCounts()counts all features, but the UI only displays loaded features that are in the menu (see lines 819-823). This could lead to misleading counts.Apply this diff to improve performance:
-int count = categoryCounts[std::string(header.name)]; +auto it = categoryCounts.find(header.name); +int count = (it != categoryCounts.end()) ? it->second : 0;
659-798: Excellent UI reorganization with opportunity for refactoring.The tab-based separation of "Settings" (management) and "About" (information) significantly improves the user experience and logical organization. The state handling for disabled/loaded/failed features is comprehensive and well-implemented.
Consider breaking this large block into separate methods for better maintainability:
void DrawFeatureSettingsTab(Feature* feat, bool isDisabled, bool isLoaded, bool hasFailedMessage); void DrawFeatureAboutTab(Feature* feat, bool isDisabled, bool isLoaded, bool hasFailedMessage);This would make the
DrawMenuVisitorcleaner and each tab's logic more focused and testable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Menu.cpp(5 hunks)src/Menu.h(3 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (9)
src/Utils/UI.h (1)
137-140: Function signature update looks good.The addition of the
categoryCountparameter toDrawCategoryHeaderis well-named and appropriately typed for displaying feature counts in category headers.src/Menu.h (3)
3-3: Include addition supports feature counting functionality.The addition of
"Feature.h"include is consistent with the need to work with Feature objects for counting features per category.
220-220: Static member for category counts is well-designed.Using
std::unordered_map<std::string, int>for tracking feature counts per category is efficient and appropriate. The static nature ensures consistent counts across the application.
293-293: Private method declaration follows good encapsulation.The
BuildCategoryCounts()method declaration properly encapsulates the category counting logic within the Menu class.src/Utils/UI.cpp (2)
345-348: Function implementation correctly matches header and formats display string.The updated function signature matches the header declaration, and the use of
std::formatto create the display string with category count is clean and safe.
354-354: Consistent usage of formatted display name throughout the function.The implementation correctly uses the formatted
displayNameconsistently for text size calculation, ID pushing, and text rendering, ensuring the category count is properly displayed in all contexts.Also applies to: 361-361, 390-390
src/Menu.cpp (3)
222-222: LGTM: Clean static member for category tracking.The static
unordered_mapis an appropriate choice for storing category counts that are computed once and used throughout the UI rendering lifecycle.
295-295: LGTM: Proper initialization placement.Calling
BuildCategoryCounts()during initialization ensures category counts are available when the UI is first rendered.
2589-2600: Clean implementation with potential count mismatch.The method correctly iterates through features and builds category counts. However, this counts all features while the UI display logic (lines 819-823) only shows features that are both
loadedandIsInMenu().Please verify if the count should match visible features or include all features. If it should match visible features, consider filtering:
void Menu::BuildCategoryCounts() { const std::vector<Feature*>& features = Feature::GetFeatureList(); - // Get the category of each feature, and if it is not in the featureGroups - // vector, add it and increment the count for (auto& feature : features) { + if (feature->IsInMenu() && feature->loaded) { std::string_view category = feature->GetCategory(); categoryCounts[std::string(category)]++; + } } }
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
Screenshots? |
…#1178) * refactor(ui): improve menu layout and organization * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * refactor(ui): simplify and improve theme color handling * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix(ui): improve category counting and code cleanup --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
3 participants
This PR includes my updates to the feature menu layout and organization, as well as the label number counts of features under each menu category.
Summary by CodeRabbit
New Features
UI Improvements
Removals