-
Notifications
You must be signed in to change notification settings - Fork 0
Interaction menu #77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Interaction menu #77
Conversation
# Conflicts: # common/test/CMakeLists.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces an interaction menu system to the application, featuring a comprehensive refactoring from an old callback-based renderer to a new script-managed architecture. The changes establish a modern, event-driven UI framework with dynamic menu components, key input handling, and improved resource management.
Key changes include:
- Complete refactoring from render thread callbacks to a script-based system with proper lifecycle management
- Introduction of an interaction menu framework with components, themes, and navigation
- Implementation of key input handling system for menu navigation and interactions
Reviewed Changes
Copilot reviewed 63 out of 68 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| menu/src/main.cpp | Complete rewrite implementing startup/shutdown sequence with script manager and sample menu |
| menu/src/ui/menu_renderer.cpp | New menu rendering system with animation and component-based UI |
| menu/src/scripts/script_manager.cpp | New script lifecycle management system |
| menu/src/util/key_input/key_event_listener.cpp | Key input handling system for menu navigation |
| menu/src/render/render_thread.cpp | Refactored render thread using script system instead of callbacks |
| menu/src/ui/components/* | Component-based UI system (labels, buttons, sub-navigation) |
| menu/src/util/startup_shutdown_handler.hpp | Centralized startup/shutdown lifecycle management |
| // Used when the current sub is empty | ||
| std::shared_ptr<components::BaseComponent> fallback_option_; | ||
| // Used when no sub can be found | ||
| std::shared_ptr<Submenu> fallback_submenu_ = std::make_shared<Submenu>("Fallback SubMenu", [this](Submenu* sub) {; |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a stray semicolon after the opening brace of the lambda expression. This will cause a compilation error.
| std::shared_ptr<Submenu> fallback_submenu_ = std::make_shared<Submenu>("Fallback SubMenu", [this](Submenu* sub) {; | |
| std::shared_ptr<Submenu> fallback_submenu_ = std::make_shared<Submenu>("Fallback SubMenu", [this](Submenu* sub) { |
| const auto char_x_size = CalcTextSizeRaw(ImGui::GetFont(), font_size, " ").x; | ||
| const auto chars_per_line = static_cast<std::uint32_t>(real_max_x / char_x_size); | ||
|
|
||
| auto* lines = new std::string[max_lines]; |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using raw new[] and delete[] is not recommended in modern C++. Consider using std::vector<std::string> or std::unique_ptr<std::string[]> for automatic memory management.
| auto* lines = new std::string[max_lines]; | |
| std::vector<std::string> lines(max_lines); |
| str = WordWrapGetString(lines, line_count); | ||
| delete[] lines; |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This corresponds to the raw new[] on line 115. Using RAII containers would eliminate the need for manual memory management.
| str = WordWrapGetString(lines, line_count); | |
| delete[] lines; | |
| str = WordWrapGetString(lines.data(), line_count); | |
| // No need to delete[] lines; vector handles memory automatically. |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | ||
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | ||
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer dereference. GetCurrentComponent() can return nullptr as documented in the header file, but this is not checked before calling HandleButtonPress().
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | |
| } | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | |
| } | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); | |
| } |
| if (menu_ui_navigation.WasKeyPressed(VK_RETURN)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | ||
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | ||
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer dereference. GetCurrentComponent() can return nullptr but this is not checked before calling HandleButtonPress().
| if (menu_ui_navigation.WasKeyPressed(VK_RETURN)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (menu_ui_navigation.WasKeyPressed(VK_RETURN)) { | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | |
| } | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | |
| } | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); | |
| } |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | ||
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | ||
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | ||
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer dereference. GetCurrentComponent() can return nullptr but this is not checked before calling HandleButtonPress().
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | |
| submenu->GetCurrentComponent()->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kSUBMIT); | |
| } | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_LEFT)) { | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kLEFT); | |
| } | |
| } else if (menu_ui_navigation.WasKeyPressed(VK_RIGHT)) { | |
| auto current_component = submenu->GetCurrentComponent(); | |
| if (current_component) { | |
| current_component->HandleButtonPress(components::BaseComponent::PressedButton::kRIGHT); | |
| } |
| # Create directory if needed | ||
| if (dir_path) | ||
| file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/include/${PROJECT_NAME}/${dir_path}) | ||
| file(MAKE_DIRECTORY $INTERFACE_DIR/${dir_path}) |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing braces around the variable expansion. This should be ${INTERFACE_DIR}/${dir_path} instead of $INTERFACE_DIR/${dir_path}.
| file(MAKE_DIRECTORY $INTERFACE_DIR/${dir_path}) | |
| file(MAKE_DIRECTORY ${INTERFACE_DIR}/${dir_path}) |
|
|
||
| rotation_start_idx_ = -1; // Reset the rotation start index after rotating | ||
| } |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rotation start index is reset inside the loop, which means it only affects the first iteration. This should be moved outside the loop.
| rotation_start_idx_ = -1; // Reset the rotation start index after rotating | |
| } | |
| } | |
| rotation_start_idx_ = -1; // Reset the rotation start index after rotating |
|




This pull request introduces several changes across multiple files, focusing on utility enhancements, logging improvements, and build system adjustments. The most significant updates include the addition of new utility classes, refactoring of logging functionality, and improvements to CMake configurations to streamline header management and asset embedding.
Utility Enhancements:
RangedValueutility class incommon/src/util/ranged_value.hppfor type-safe bounded values, along with comprehensive tests incommon/test/src/util/ranged_value.cpp. This class ensures values are clamped within a specified range and integrates with the Glaze library for serialization. [1] [2]enum_bitops.hppincommon/src/util/enum_bitops.hppto enable bitwise operations on enums using themagic_enumlibrary.Logging Improvements:
exception_report.cppby removing redundantcommon::prefixes forconversionandfs::vfs. [1] [2] [3]vectored_handler.cppto use modern C++ syntax, including an if-with-initializer and conditional compilation for debug builds. [1] [2]vectored_handler.cpp.Build System Adjustments:
common/CMakeLists.txtandcommon/test/CMakeLists.txtto include the newranged_valueandenum_bitopsutilities. [1] [2]imfont/CMakeLists.txtto use a build-type-specificINTERFACE_DIRfor header management and updated asset embedding to include new font files. [1] [2] [3]Asset Updates:
imfont/src/imfont.cppwith a sharper variant (fa-sharp-solid-900.ttf) and added a bold version of the Roboto Mono font. [1] [2]These changes collectively improve the codebase's utility, maintainability, and build process while introducing new features for bounded value handling and enhanced logging.