Improve rom info #146
Improve rom info #146
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (45)
src/menu/actions.h (1)
14-14: LGTM! Consider adding documentation for the new function.The addition of
actions_initis a good improvement, likely for initializing the actions module. Its declaration follows proper C conventions and is consistently placed within the file.To enhance code maintainability, consider adding a brief documentation comment for this new function, similar to how the file itself is documented. For example:
/** * @brief Initialize the actions module */ void actions_init(void);src/menu/sound.h (3)
15-21: LGTM: Well-structured sound effect enumThe
sound_effect_tenum is a good addition, providing a clear and structured way to represent different sound effects. The naming is consistent and descriptive, covering common UI interactions.Consider adding a brief comment above the enum to describe its purpose and usage. For example:
/** * @brief Enumeration of available sound effects for menu interactions. */ typedef enum { // ... (existing enum members) } sound_effect_t;
26-27: LGTM: New sound effect initialization and control functionsThe addition of
sound_init_sfx()andsound_use_sfx(bool)functions is consistent with the existing sound initialization functions and provides necessary control over sound effects.Consider adding brief documentation for these new functions to improve clarity. For example:
/** * @brief Initialize sound effects system. */ void sound_init_sfx(void); /** * @brief Enable or disable sound effects. * @param enable True to enable sound effects, false to disable. */ void sound_use_sfx(bool enable);
28-28: LGTM: New function to play sound effectsThe addition of
sound_play_effect(sound_effect_t sfx)provides a clear way to play specific sound effects using thesound_effect_tenum.Consider adding brief documentation for this new function to improve clarity. For example:
/** * @brief Play a specified sound effect. * @param sfx The sound effect to play, as defined in sound_effect_t. */ void sound_play_effect(sound_effect_t sfx);.devcontainer/Dockerfile (1)
Line range hint
1-17: Consider pinning the MIPS64 GCC toolchain version.While updating the SC64 deployer version is good for keeping the project up-to-date, it's worth noting that the MIPS64 GCC toolchain version is not specified. For better reproducibility and to avoid potential issues with future toolchain updates, consider pinning this version as well.
You could modify the relevant line to include a specific version, like this:
- wget https://github.com/DragonMinded/libdragon/releases/download/toolchain-continuous-prerelease/gcc-toolchain-mips64-x86_64.deb && \ + wget https://github.com/DragonMinded/libdragon/releases/download/toolchain-9.1.0/gcc-toolchain-mips64-x86_64.deb && \Replace
9.1.0with the desired version. Make sure to check the libdragon releases page for the latest stable version.src/menu/views/flashcart_info.c (3)
8-8: Approved: Enhanced user feedback with sound effectThe addition of
sound_play_effect(SFX_EXIT)when the back action is invoked improves user interaction by providing audio feedback. This is a good enhancement to the user experience.Consider defining a constant for the sound effect (e.g.,
SOUND_EFFECT_BACK) to improve code readability and maintainability.
22-24: Approved: Improved user communicationThe addition of the message "This feature is not yet supported." provides clear information to the user about the current state of the flashcart information feature. This improves user communication and aligns with the PR objectives.
Consider using a constant for the message text to facilitate easier updates and potential localization in the future. For example:
#define FLASHCART_INFO_NOT_SUPPORTED_MSG "This feature is not yet supported." // In the draw function component_main_text_draw( ALIGN_CENTER, VALIGN_TOP, "FLASHCART INFORMATION\n\n\n" FLASHCART_INFO_NOT_SUPPORTED_MSG "\n\n" );
27-31: Consider using a task tracking system for future enhancementsThe added comments provide valuable information about planned improvements to the flashcart information display. However, it's generally better to use a task tracking system (like GitHub Issues) for managing future enhancements rather than inline comments in the code.
Would you like me to create GitHub issues for these planned enhancements? This would help in tracking the progress of these features and keep the codebase clean.
If you prefer to keep these as comments, consider using a standardized format like:
// TODO: Display cart type, firmware version, and supported features (flashcart_features_t)src/menu/views/error.c (2)
8-8: LGTM: Sound effect for back action.The addition of
sound_play_effect(SFX_EXIT);provides appropriate auditory feedback when the user navigates back from the error screen.Consider moving the sound effect call before setting
menu->next_modefor consistency with other similar patterns in the codebase:if (menu->actions.back) { + sound_play_effect(SFX_EXIT); menu->next_mode = MENU_MODE_BROWSER; - sound_play_effect(SFX_EXIT); }
53-55: LGTM: Sound effect for error display.The addition of
sound_play_effect(SFX_ERROR);provides appropriate auditory feedback when an error message is displayed.Consider adding error handling for the
sound_play_effectcall to ensure robustness:+ if (sound_play_effect(SFX_ERROR) != SOUND_OK) { + debugf("Failed to play error sound effect\n"); + } menu->next_mode = MENU_MODE_ERROR; menu->error_message = error_message;This change assumes the existence of a
SOUND_OKstatus and adebugffunction for logging. Adjust as necessary based on your project's conventions.src/menu/views/rtc.c (2)
22-22: LGTM: Sound effect addition enhances user feedbackThe addition of the
sound_play_effect(SFX_EXIT)call provides auditory feedback when the user exits the menu, which is a good UX improvement.Consider defining an enum or constants for the different sound effects (if not already done) to improve code readability and maintainability. For example:
typedef enum { SFX_EXIT, SFX_ENTER, // ... other sound effects } SoundEffect;This would make it clearer what sound effects are available throughout the application.
39-39: LGTM: Updated user instructions provide more optionsThe modification to include information about setting the date and time via a game is a good addition, providing users with more options.
Consider clarifying the phrase "a game that uses it" to be more specific. For example:
- "application and set via USB or a game that uses it.\n\n" + "application and set via USB or a compatible N64 game with RTC support.\n\n"This change would make it clearer to users what kind of games can be used for this purpose.
src/menu/views/credits.c (1)
16-16: LGTM: Added sound effect for menu navigation.The addition of
sound_play_effect(SFX_EXIT);enhances user experience by providing audio feedback when exiting the credits view. This is a good improvement to the menu navigation.Consider adding error handling for the
sound_play_effectcall, in case the sound system fails to initialize or the effect doesn't exist:if (sound_play_effect(SFX_EXIT) != SOUND_OK) { // Log error or handle gracefully }This assumes that
sound_play_effectreturns a status code. If it doesn't, you can disregard this suggestion.src/menu/views/system_info.c (2)
31-33: LGTM: Sound effect for menu exitThe addition of
sound_play_effect(SFX_EXIT);when exiting the system info view is a good improvement for user experience. It provides auditory feedback to the user's action.For improved clarity, consider adding a brief comment explaining the purpose of this sound effect:
if (menu->actions.back) { menu->next_mode = MENU_MODE_BROWSER; + // Play exit sound effect when leaving the system info view sound_play_effect(SFX_EXIT); }
Line range hint
1-91: Consider adding sound effects for consistencyThe addition of a sound effect for exiting the system info view enhances user feedback. For consistency, consider adding appropriate sound effects to other user interactions within this view, if applicable. This could include sounds for navigation or selection of options, providing a more cohesive auditory experience throughout the menu system.
Would you like assistance in identifying potential places to add sound effects for other user interactions?
src/menu/settings.c (2)
42-42: LGTM: Consistent loading of sound_enabled settingThe change to load
sound_enabledfrom the "menu" section is consistent with its new status as a regular setting rather than a beta feature. This aligns well with the initialization in theinitstructure.For consistency with other settings, consider adding a TODO comment similar to the one for
pal60_enabled:- settings->sound_enabled = mini_get_bool(ini, "menu", "sound_enabled", init.sound_enabled); + settings->sound_enabled = mini_get_bool(ini, "menu", "sound_enabled", init.sound_enabled); // TODO: consider changing file setting nameThis maintains consistency in code style and reminds developers to review the setting name in the future if needed.
58-58: LGTM: Consistent saving of sound_enabled settingThe change to save
sound_enabledto the "menu" section is consistent with the loading function and the overall structure of the settings. This ensures that the setting is properly persisted and retrieved.For consistency with other settings, consider grouping similar settings together:
mini_set_bool(ini, "menu", "pal60", settings->pal60_enabled); mini_set_bool(ini, "menu", "show_protected_entries", settings->show_protected_entries); +mini_set_bool(ini, "menu", "sound_enabled", settings->sound_enabled); mini_set_string(ini, "menu", "default_directory", settings->default_directory); mini_set_bool(ini, "menu", "use_saves_folder", settings->use_saves_folder); -mini_set_bool(ini, "menu", "sound_enabled", settings->sound_enabled);This groups all boolean settings together, improving readability and maintainability.
docs/99_developer_guide.md (1)
Line range hint
67-77: Great addition! Consider clarifying the purpose.The new instructions for testing generated documentation in the dev-container are a valuable addition. They provide clear steps for setting up the environment and serving the documentation locally, which is crucial for ensuring the quality of documentation changes.
Consider adding a brief explanation of why testing the generated docs locally is important. For example:
### Test generated docs in the dev-container +Testing the documentation locally allows you to preview changes and ensure everything renders correctly before submitting your changes. + Install Prerequisites: ```bash apt-get install ruby-full build-essential zlib1g-dev gem install jekyll bundler<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 9-9: Punctuation: ':' Trailing punctuation in heading (MD026, no-trailing-punctuation) </blockquote></details> </details> </blockquote></details> <details> <summary>docs/00_getting_started_sd.md (1)</summary><blockquote> `3-16`: **Excellent additions for flashcart-specific instructions!** The new sections for SC64 and other supported flashcarts provide clear and specific instructions, which aligns well with the PR objective of enhancing ROM file versatility. This improvement will help users set up their SD cards correctly based on their flashcart type. Consider adjusting the heading levels for consistency: ```diff -#### SC64 +### SC64 -#### Other supported flashcarts +### Other supported flashcartsThis change will maintain a consistent heading hierarchy throughout the document.
🧰 Tools
🪛 Markdownlint
4-4: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
src/menu/views/image_viewer.c (3)
2-2: LGTM! Consider grouping related headers.The inclusion of "../sound.h" is appropriate for adding sound functionality to the image viewer.
Consider grouping related headers together. You might want to move this include statement next to other menu-related headers for better organization.
44-44: LGTM! Sound effects enhance user experience.The addition of sound effects for navigation actions is a good improvement to the user interface. The placement and choice of sounds (SFX_EXIT for back action, SFX_ENTER for enter action) are appropriate and consistent with user expectations.
For consistency, consider adding a sound effect for the case when
show_messagebecomes false without changing the mode (line 46). This would provide audio feedback for all user interactions.Also applies to: 53-53
Sound Effects Implementation Issues Identified
The verification process revealed the following issues with the sound effects implementation:
- Missing Sound File:
exit.wavis not present in theassetsdirectory. This will prevent theSFX_EXITsound effect from playing.- Improper Linking: The
sfx_exitandsfx_entersound effects are not properly initialized insound.c, resulting in sound effects not being loaded correctly.Recommended Actions:
Add
exit.wavto Assets:
- Ensure that the
exit.wavfile is added to theassetsdirectory.- Verify the file's integrity and compatibility with the application.
Initialize Sound Effects in
sound.c:
- Update
sound.cto loadsfx_exitandsfx_entercorrectly. For example:void sound_init_mp3_playback(void) { sfx_exit = wav64_load("assets/exit.wav"); sfx_enter = wav64_load("assets/enter.wav"); // ... other initialization code }- Confirm that the initialization function is called appropriately during the application's startup.
Test Sound Playback:
- After adding the sound file and updating the code, perform tests to ensure that both
SFX_EXITandSFX_ENTERplay as expected during user interactions.- Adjust volume levels if necessary to maintain consistency with other UI sounds.
🔗 Analysis chain
Line range hint
1-138: Overall, good improvements to user feedback.The changes in this file successfully implement sound effects for user actions in the image viewer, enhancing the user experience. The modifications are minimal, well-placed, and align with the PR objectives.
To ensure the sound effects are working as intended, please verify:
- The correct sound files are available and properly linked.
- The sound effects play correctly during navigation in the image viewer.
- The volume levels are appropriate and consistent with other UI sounds in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify sound-related files and usage echo "Checking for sound.h file:" fd --type f "sound.h" echo "\nChecking for sound effect definitions:" rg --type c "SFX_EXIT|SFX_ENTER" -C 3 echo "\nChecking for other uses of sound_play_effect:" rg --type c "sound_play_effect" -C 3Length of output: 20926
Script:
#!/bin/bash # Verify the availability of sound files associated with SFX_EXIT and SFX_ENTER echo "Checking for sound files related to SFX_EXIT and SFX_ENTER:" # Assuming sound files have common audio extensions fd --type f -e wav -e mp3 -e ogg -e flac | rg -i "exit|enter" echo "\nVerifying that sound files are properly linked in sound.c:" # Check if sound effects are linked to specific files in sound.c ast-grep --lang c --pattern $' void sound_init_mp3_playback { $$$ sfx_exit = wav64_load("$_"); sfx_enter = wav64_load("$_"); $$$ } 'Length of output: 482
Script:
#!/bin/bash # Check for the existence of exit.wav echo "Checking for exit.wav in assets:" fd --type f -e wav -e mp3 -e ogg -e flac | rg -i "exit" echo "\nVerifying the linking of sfx_exit and sfx_enter in sound.c:" # Use a broader pattern to find how sfx_exit and sfx_enter are loaded ast-grep --lang c --pattern $' void sound_init_$$$ { $$$ sfx_exit = wav64_load("$_"); sfx_enter = wav64_load("$_"); $$$ } 'Length of output: 396
src/menu/views/load_emulator.c (2)
39-41: LGTM: Exit sound effect addedThe addition of
sound_play_effect(SFX_EXIT);enhances the user experience by providing auditory feedback when exiting the emulator loading view. This is a good improvement.For consistency, consider adding a sound effect for the enter action as well. This would provide a balanced audio experience for user interactions.
Line range hint
1-150: Overall review: Sound integration enhances user experienceThe changes in this file successfully integrate sound functionality into the emulator loading process. The addition of an exit sound effect improves user feedback without disrupting the existing logic. The modifications are minimal and well-placed.
Consider the following suggestions for further improvement:
- Add sound effects for other user actions (e.g., when entering the emulator loading view) to provide a consistent audio experience.
- Ensure that the sound effects are tested across different emulator types to confirm they work as expected in all scenarios.
src/menu/views/text_viewer.c (1)
59-59: LGTM: Sound effect for exit actionThe addition of
sound_play_effect(SFX_EXIT);enhances user feedback when navigating back from the text viewer. This is a good improvement to the user experience.Consider adding error handling for the
sound_play_effectcall if the sound module's implementation requires it. For example:if (sound_play_effect(SFX_EXIT) != SOUND_OK) { // Handle error or log it }Makefile (2)
80-85: LGTM! Consider adding documentation for the new sound assets.The addition of the SOUNDS variable with various .wav files is a good enhancement, aligning with the PR's objective to improve ROM capabilities.
Consider adding a comment above the SOUNDS variable to briefly explain the purpose of these sound assets and how they will be used in the project.
107-109: LGTM! Consider adding a comment explaining the audio conversion process.The new rule for generating .wav64 files is correctly implemented and necessary for processing the newly added sound assets.
Consider adding a brief comment above this rule to explain the purpose of the .wav64 format and why this conversion is necessary for the N64 platform. This would improve the maintainability of the Makefile.
src/menu/components/background.c (1)
Line range hint
1-224: Overall assessment of changes in background.cThe changes in this file appear to be improvements in terms of code clarity and potentially efficiency. The simplification of the rectangle filling for image darkening and the relocation of image center calculations seem logical. However, it's crucial to ensure these changes don't introduce any regressions or unintended side effects.
To maintain the integrity of the codebase:
- Verify that the darkening process still works as expected with the simplified rectangle filling.
- Confirm that relocating the image center calculations doesn't affect any dependent operations.
- Run thorough tests on the background component to ensure all functionalities remain intact.
Consider adding unit tests for the
prepare_backgroundfunction if they don't already exist. This would help catch any potential issues introduced by these or future changes.README.md (3)
52-90: Excellent addition of GamePak sprites documentationThe new "GamePak sprites" section under "Experimental features" is a comprehensive and well-structured addition to the README. It provides clear guidelines on using PNG files for N64 GamePak sprites, including supported dimensions, file naming conventions, and directory structure. The inclusion of a compatibility mode for deprecated naming conventions is thoughtful.
Suggestion for improvement:
Consider adding a brief note about the potential impact on menu performance or loading times when using GamePak sprites, especially for users with large ROM collections.
124-128: Great addition of sound attribution and licensingThe new "Sounds" subsection under "Open source software and licenses used" is a valuable addition. It properly attributes the sources of the menu sound effects and provides clear licensing information. This demonstrates good open-source practices and legal compliance.
Suggestion for improvement:
Consider adding a brief note about how users can customize or replace these sounds, if that's a possibility. This could enhance the user experience for those who might want to personalize their menu sounds.
Line range hint
1-128: Overall excellent improvements to README documentationThe changes to the README.md file significantly enhance the documentation quality and completeness. Key improvements include:
- Addition of menu sound effects to the feature list.
- Comprehensive documentation on GamePak sprites usage.
- Proper attribution and licensing information for sound effects.
These changes align well with the PR objectives and provide valuable information for users and contributors. The document structure remains clear and well-organized.
Suggestion for future improvement:
Consider adding a table of contents at the beginning of the README to help users navigate the growing amount of information more easily.src/menu/components/constants.h (2)
78-80: LGTM! Consider adding a brief comment explaining 64DD.The addition of constants for 64DD box art dimensions is a good improvement for supporting different types of box art.
Consider adding a brief comment explaining what 64DD stands for (Nintendo 64 Disk Drive) for better clarity, especially for developers who might not be familiar with the term.
91-99: LGTM! Consider minor improvements for consistency and clarity.The addition of constants for different box art positions is a good improvement, allowing for precise layout control.
- Consider standardizing the abbreviation format in constant names:
- Either use
BOXART_X_JPandBOXART_X_DD, orBOXART_X_JPNandBOXART_X_64DDfor consistency.- In the comments, "caratules" might be a typo. Did you mean "cartridges" or is this a specific term?
- Consider adding a brief comment explaining the calculation for each position, which could help future maintenance.
src/menu/rom_info.h (3)
226-230: Add documentation for the new settings structure.The addition of the
settingsstructure withcheats_enabledandpatches_enabledflags is a good improvement for fine-grained control. However, to enhance clarity and maintainability, consider adding brief documentation for these new fields.Consider adding comments like this:
struct { /** @brief Flag to enable or disable cheats for this ROM. */ bool cheats_enabled; /** @brief Flag to enable or disable patches for this ROM. */ bool patches_enabled; } settings;
232-235: Add documentation and consider null-termination for the description field.The addition of the
metadatastructure with adescriptionfield is a good improvement for storing additional ROM information. However, consider the following suggestions:
- Add documentation for the new field to enhance clarity.
- Ensure null-termination for the description to prevent potential issues.
Consider updating the structure like this:
struct { /** @brief Additional description or notes about the ROM (null-terminated). */ char description[256]; } metadata;Also, ensure that any function filling this field guarantees null-termination, perhaps by using
strncpywith an explicit null terminator:strncpy(rom_info->metadata.description, source, sizeof(rom_info->metadata.description) - 1); rom_info->metadata.description[sizeof(rom_info->metadata.description) - 1] = '\0';
238-239: Add documentation for the new function declarations.The addition of
rom_info_get_cic_seedandrom_info_loadfunction declarations is consistent with the changes to therom_info_tstructure. However, to improve usability and maintainability, consider adding brief documentation for these functions.Consider adding comments like this:
/** * @brief Retrieves the CIC seed for the given ROM information. * @param rom_info Pointer to the ROM information structure. * @param seed Pointer to store the retrieved CIC seed. * @return true if the CIC seed was successfully retrieved, false otherwise. */ bool rom_info_get_cic_seed(rom_info_t *rom_info, uint8_t *seed); /** * @brief Loads ROM information from the specified path. * @param path Pointer to the path structure containing the ROM file location. * @param rom_info Pointer to the ROM information structure to be filled. * @return ROM_OK if successful, or an appropriate error code otherwise. */ rom_err_t rom_info_load(path_t *path, rom_info_t *rom_info);src/flashcart/sc64/sc64.c (1)
257-258: LGTM! Consider updating documentation.The addition of
FLASHCART_FEATURE_AUTO_CICandFLASHCART_FEATURE_AUTO_REGIONfeatures is a good improvement. These features can enhance compatibility and user experience.Consider updating the relevant documentation to reflect these new features, ensuring users and developers are aware of the SC64's expanded capabilities.
src/menu/sound.c (1)
62-83: Consider logging unrecognized sound effects insound_play_effectIn the
defaultcase of the switch statement withinsound_play_effect, no action is taken if an unrecognizedsfxvalue is passed. Adding a log message can help with debugging unexpected values.For example:
default: + // Log a warning about the unrecognized sound effect + debugf("Warning: Unrecognized sound effect %d\n", sfx); break;src/menu/components/boxart.c (2)
87-88: Redundantifstatement can be removedThe nested
if (file_exists(path_get(path)))at line 88 is redundant since the same condition is already checked at line 87. This redundant check can be removed to simplify the code.Apply this diff to simplify the code:
if (file_exists(path_get(path))) { - if (file_exists(path_get(path))) { if (png_decoder_start(path_get(path), BOXART_WIDTH_MAX, BOXART_HEIGHT_MAX, png_decoder_callback, b) == PNG_OK) { path_free(path); return b; } - } }
133-143: Consider handling additional image dimensionsIn the
component_boxart_drawfunction, the conditions adjustbox_xandbox_ybased on specific image dimensions. Consider handling cases where the image dimensions are outside the expected ranges to ensure proper rendering or provide fallback values.You might want to add an
elseclause to handle unexpected image sizes or log a warning for easier debugging.src/menu/menu.c (1)
30-31: Consider makingINTERLACEDandFPS_LIMITuser-configurable settingsDefining
INTERLACEDandFPS_LIMITas constants limits flexibility. Consider making these values configurable through the settings to allow users to adjust display preferences according to their needs.src/menu/views/settings_editor.c (1)
48-51: Implement theset_use_default_settingsfunction to restore default settingsThe function
set_use_default_settingsis currently commented out with aFIXMEnote, indicating that its implementation is pending. Implementing this function will allow users to restore the menu settings to their default values, enhancing usability.Would you like assistance in implementing this function? I can help provide a solution or open a new GitHub issue to track this task.
src/menu/views/load_rom.c (2)
103-103: Avoid hardcoding multiple newline characters in 'format_rom_description'Using multiple
\ncharacters to create vertical space can lead to inconsistent layouts and is not optimal. Consider handling spacing within the layout or formatting functions to maintain consistent styling.You might adjust the return statement as follows:
- return description[0] == '\0' ? "None.\n\n\n\n\n\n\n" : description; + return description[0] == '\0' ? "None." : description;And manage the spacing in the calling function or through styling.
125-127: Consider making 'format_boolean_type' function inlineFor a small utility function like
format_boolean_type, making itinlinecan improve performance by avoiding function call overhead.Apply this change:
-static const char *format_boolean_type (bool bool_value) { +static inline const char *format_boolean_type (bool bool_value) { return bool_value ? "On" : "Off"; }src/menu/rom_info.c (1)
921-921: Clarify the return value inrom_info_get_cic_seedThe return statement in
rom_info_get_cic_seedmight be confusing:return (!rom_info->boot_override.cic);Consider adding a comment to explain why the function returns the negation of
rom_info->boot_override.cicto improve readability and maintainability.
🛑 Comments failed to post (20)
src/menu/sound.h (1)
13-13: 💡 Codebase verification
Conflict Detected: Channel number 2 is already in use
The
SOUND_SFX_CHANNELis assigned the value2, which is already used insrc/menu/mp3_player.cfor.channels = 2. Please choose a different, unique channel number to avoid potential conflicts in the sound system.
- Location of conflict:
src/menu/mp3_player.c🔗 Analysis chain
LGTM: New sound effects channel defined, but clarification needed
The addition of
SOUND_SFX_CHANNELis good for separating sound effects from MP3 playback. The use of parentheses in the macro definition is a good practice.Could you please clarify why the specific value of 2 was chosen for this channel? It would be helpful to understand if there's a particular reason for this choice or if it's related to the underlying audio system.
To verify the uniqueness of this channel number, let's run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other occurrences of channel number 2 in the codebase # Test: Search for other occurrences of channel number 2 rg --type c --type cpp --type h "channel.*2" -g '!src/menu/sound.h'Length of output: 114
src/flashcart/flashcart.h (1)
32-33: 💡 Codebase verification
Action Required: Update
flashcart.cto handle newflashcart_features_tenum values.The switch statements in
flashcart.ccurrently do not handleFLASHCART_FEATURE_AUTO_CICandFLASHCART_FEATURE_AUTO_REGION. To ensure consistent behavior across the codebase, please add cases for these new enum values. For example:switch (feature) { case FLASHCART_FEATURE_64DD: return true; case FLASHCART_FEATURE_RTC: return true; case FLASHCART_FEATURE_USB: return true; case FLASHCART_FEATURE_AUTO_CIC: return true; case FLASHCART_FEATURE_AUTO_REGION: return true; default: return false; }This update will ensure that the new features are recognized and handled appropriately within
flashcart.c.🔗 Analysis chain
LGTM! Consider adding explanatory comments.
The addition of
FLASHCART_FEATURE_AUTO_CICandFLASHCART_FEATURE_AUTO_REGIONto theflashcart_features_tenum is appropriate and aligns with the PR objectives of enhancing ROM-related capabilities. These new features will likely improve the versatility of the flashcart system.Consider adding brief comments to explain each enum value, including the new ones. This would improve the overall documentation and make it easier for other developers to understand the purpose of each feature. For example:
typedef enum { FLASHCART_FEATURE_64DD, /**< Support for 64DD functionality */ FLASHCART_FEATURE_RTC, /**< Real-Time Clock support */ FLASHCART_FEATURE_USB, /**< USB connectivity support */ FLASHCART_FEATURE_AUTO_CIC, /**< Automatic CIC (Copy Protection) detection */ FLASHCART_FEATURE_AUTO_REGION /**< Automatic region detection */ } flashcart_features_t;Let's verify the impact of these new features on the rest of the codebase:
This script will help us identify any areas of the codebase that might need to be updated to handle these new enum values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new enum values and potential impacts # Search for direct usage of the new enum values echo "Searching for direct usage of new enum values:" rg --type c --type cpp "FLASHCART_FEATURE_AUTO_CIC|FLASHCART_FEATURE_AUTO_REGION" # Search for switch statements or if-else chains that might need updating echo "Searching for switch statements or if-else chains using flashcart_features_t:" rg --type c --type cpp -A 10 "switch.*flashcart_features_t|if.*flashcart_features_t" # Search for array initializations that might need updating echo "Searching for array initializations using flashcart_features_t:" rg --type c --type cpp "flashcart_features_t.*\{" -A 10Length of output: 3704
.github/workflows/build.yml (1)
80-81: 💡 Codebase verification
Verified tag name change and identified workflow issue
The
tag_namehas been successfully updated to'rolling_release', and no references to'rolling-release'remain in the repository. This ensures that the tag name change should not disrupt any dependent scripts or processes.The
make_latest: trueparameter has been correctly added, which will mark this rolling release as the latest release on GitHub, enhancing its visibility.Issue Identified: The workflow includes a step for uploading a dev rolling release conditioned on the
developbranch (refs/heads/develop). However, thedevelopbranch does not exist in the repository. This means the dev release step will never execute, potentially missing out on intended pre-releases.
- Recommendation: Create the
developbranch if it's intended to be used, or update the workflow to reference the correct existing branch.🔗 Analysis chain
Review tag name change and latest release setting
Two important changes have been made to the "Upload rolling release" step:
- The
tag_namehas been changed fromrolling-releasetorolling_release(underscore instead of hyphen).- A new parameter
make_latest: truehas been added.The tag name change is minor but could potentially break any scripts or processes that depend on the exact tag name. Ensure that all relevant documentation and dependent processes are updated accordingly.
The addition of
make_latest: trueis significant as it will mark this rolling release as the latest release on GitHub. This change improves visibility of the rolling release but may have implications for how users and automated tools interpret the "latest" release of your project.To ensure these changes don't cause unintended consequences, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any references to the old tag name in the repository rg "rolling-release" --type yaml --type md # List all workflow files to verify if there are any other workflows that might be affected by this change fd -e yml -e yaml .github/workflowsLength of output: 335
Script:
#!/bin/bash # List all branches to verify if 'develop' branch exists git branch -a # Search for any references to 'rolling-release' in the entire repository rg "rolling-release" # Search for any references to 'rolling_release' to ensure consistency rg "rolling_release" # Check the status of the repository to ensure it's on the correct branch git statusLength of output: 216
Script:
#!/bin/bash # Display the contents of the build.yml file to verify the tag_name cat .github/workflows/build.yml # Search for tag_name assignments in all workflow files rg "tag_name" .github/workflows/*.ymlLength of output: 3802
src/menu/sound.c (2)
86-86:
⚠️ Potential issueEnsure proper cleanup of sound effect resources
The
sound_deinitfunction currently does not close the WAV files opened insound_init_sfx, which can lead to resource leaks.Introduce a
sound_deinit_sfxfunction to close all opened sound effects and call it withinsound_deinit. For example:void sound_deinit_sfx(void) { if (sfx_enabled) { wav64_close(&sfx_cursor); wav64_close(&sfx_exit); wav64_close(&sfx_setting); wav64_close(&sfx_enter); wav64_close(&sfx_error); sfx_enabled = false; } } void sound_deinit(void) { if (sound_initialized) { sound_deinit_sfx(); mixer_close(); audio_close(); sound_initialized = false; } }
43-51:
⚠️ Potential issueAdd error handling for
wav64_opencalls insound_init_sfxIn the
sound_init_sfxfunction, thewav64_openfunction is called for each sound effect without checking the return value. If any of these calls fail, it could lead to null pointer dereferences or crashes when attempting to play the sound effects.Consider adding error checking for each
wav64_opencall and handling failures appropriately. For example:void sound_init_sfx (void) { mixer_ch_set_vol(SOUND_SFX_CHANNEL, 0.5f, 0.5f); - wav64_open(&sfx_cursor, "rom:/cursorsound.wav64"); + if (!wav64_open(&sfx_cursor, "rom:/cursorsound.wav64")) { + // Handle error, e.g., log a warning or disable this sound effect + } // Repeat for other wav64_open calls sfx_enabled = true; }Committable suggestion was skipped due to low confidence.
src/menu/actions.c (2)
93-94:
⚠️ Potential issueInitialize
pressedvariableThe
pressedvariable is declared but not initialized. If no buttons are pressed on any joypad port,pressedmay contain indeterminate values when accessed later, leading to unexpected behavior.Apply this diff to initialize
pressed:-joypad_buttons_t pressed; +joypad_buttons_t pressed = {0};📝 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.joypad_buttons_t pressed = {0};
28-29:
⚠️ Potential issueInitialize
held_dirandfast_dirvariablesThe variables
held_dirandfast_dirare declared but not initialized. This could lead to undefined behavior if no direction input is detected on any joypad port, as their values would remain uninitialized when accessed later in the function.Apply this diff to initialize the variables:
joypad_8way_t held_dir; joypad_8way_t fast_dir; +held_dir = JOYPAD_8WAY_NONE; +fast_dir = JOYPAD_8WAY_NONE;📝 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.joypad_8way_t held_dir; joypad_8way_t fast_dir; held_dir = JOYPAD_8WAY_NONE; fast_dir = JOYPAD_8WAY_NONE;src/menu/components/boxart.c (3)
71-71:
⚠️ Potential issuePotential memory leak:
pathnot freed before reinitializationBefore reinitializing
pathwithpath_initat line 71, the existingpathshould be freed usingpath_free(path);to prevent a memory leak.Apply this diff to fix the issue:
// Reset the directory path used for boxart. + path_free(path); path = path_init(storage_prefix, BOXART_DIRECTORY);📝 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.// Reset the directory path used for boxart. path_free(path); path = path_init(storage_prefix, BOXART_DIRECTORY);
68-74:
⚠️ Potential issueBuffer overflow due to insufficient size of
file_namearrayThe
file_namearray is declared with a size of 8 bytes, but when usingsprintfwith format"%c%c%c%c.png", it writes 9 bytes including the null terminator. This results in a buffer overflow.Apply this diff to fix the issue:
- char file_name[8]; + char file_name[9]; // Use `snprintf` to prevent buffer overflow - sprintf(file_name, "%c%c%c%c.png", game_code[0], game_code[1], game_code[2], game_code[3]); + snprintf(file_name, sizeof(file_name), "%c%c%c%c.png", game_code[0], game_code[1], game_code[2], game_code[3]);Repeat similar changes for the other
sprintfcalls:At line 84-85:
- sprintf(file_name, "%c%c%c.png", game_code[0], game_code[1], game_code[2]); + snprintf(file_name, sizeof(file_name), "%c%c%c.png", game_code[0], game_code[1], game_code[2]);At line 98-99:
- sprintf(file_name, "%c%c.png", game_code[1], game_code[2]); + snprintf(file_name, sizeof(file_name), "%c%c.png", game_code[1], game_code[2]);📝 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.char file_name[9]; // reset the directory path used for boxart. path = path_init(storage_prefix, BOXART_DIRECTORY); // Use `snprintf` to prevent buffer overflow snprintf(file_name, sizeof(file_name), "%c%c%c%c.png", game_code[0], game_code[1], game_code[2], game_code[3]); path_push(path, file_name);🧰 Tools
🪛 cppcheck
[error] 73-73: Buffer is accessed out of bounds
(bufferAccessOutOfBounds)
40-57:
⚠️ Potential issueMissing
breakstatements inswitchcase leading to unintended fall-throughThe
switchstatement lacksbreak;statements after eachcase, causing unintended fall-through to subsequent cases. This will result in multiplepath_pushcalls being executed, which is likely not the intended behavior.Apply this diff to fix the issue:
switch (current_image_view) { case IMAGE_GAMEPAK_FRONT: path_push(path, "gamepak_front.png"); + break; case IMAGE_GAMEPAK_BACK: path_push(path, "gamepak_back.png"); + break; case IMAGE_BOXART_BACK: path_push(path, "boxart_back.png"); + break; case IMAGE_BOXART_LEFT: path_push(path, "boxart_left.png"); + break; case IMAGE_BOXART_RIGHT: path_push(path, "boxart_right.png"); + break; case IMAGE_BOXART_BOTTOM: path_push(path, "boxart_bottom.png"); + break; case IMAGE_BOXART_TOP: path_push(path, "boxart_top.png"); + break; default: path_push(path, "boxart_front.png"); + break; }📝 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.switch (current_image_view) { case IMAGE_GAMEPAK_FRONT: path_push(path, "gamepak_front.png"); break; case IMAGE_GAMEPAK_BACK: path_push(path, "gamepak_back.png"); break; case IMAGE_BOXART_BACK: path_push(path, "boxart_back.png"); break; case IMAGE_BOXART_LEFT: path_push(path, "boxart_left.png"); break; case IMAGE_BOXART_RIGHT: path_push(path, "boxart_right.png"); break; case IMAGE_BOXART_BOTTOM: path_push(path, "boxart_bottom.png"); break; case IMAGE_BOXART_TOP: path_push(path, "boxart_top.png"); break; default: path_push(path, "boxart_front.png"); break; }src/menu/views/load_disk.c (4)
10-10: 🛠️ Refactor suggestion
Consider encapsulating
boxartto improve modularityUsing a static variable
boxartmay limit the flexibility and maintainability of the code. Encapsulatingboxartwithin themenu_tstructure or another context structure can enhance modularity and make the code more reusable, especially if multiple instances of the menu are needed in the future.
96-97:
⚠️ Potential issueAdd NULL check before drawing
boxartTo prevent potential null pointer dereferences, check if
boxartis not NULL before callingcomponent_boxart_draw(boxart).Apply this diff to add a NULL check:
if (boxart != NULL) { component_boxart_draw(boxart); }📝 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.if (boxart != NULL) { component_boxart_draw(boxart); }
154-156:
⚠️ Potential issueSet
boxartto NULL after freeingAfter freeing
boxartin thedeinitfunction, set it to NULL to avoid dangling pointers and potential double-free errors in the future.Apply this diff:
static void deinit (void) { component_boxart_free(boxart); + boxart = NULL; }📝 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.static void deinit (void) { component_boxart_free(boxart); boxart = NULL; }
173-173:
⚠️ Potential issueHandle potential failure of
component_boxart_init
component_boxart_initmay return NULL if the initialization fails. It's important to check the return value and handle the error appropriately to prevent unexpected behavior later.Apply this diff to add error handling:
boxart = component_boxart_init(menu->storage_prefix, menu->load.disk_info.id, IMAGE_BOXART_FRONT); + if (boxart == NULL) { + // Handle initialization failure, e.g., log an error or set a fallback image + }Committable suggestion was skipped due to low confidence.
src/menu/menu.c (1)
38-39:
⚠️ Potential issueReplace
assertwith proper error handling for memory allocation failureUsing
assert(menu != NULL);will abort the program if memory allocation fails. In production code, it's better to handle such failures gracefully. Consider checking the result ofcallocand providing an error message or entering a safe fail state.src/menu/views/settings_editor.c (2)
14-17:
⚠️ Potential issueAvoid casting
void*directly toboolto prevent undefined behaviorCasting a
void*directly to aboolcan lead to unexpected results or undefined behavior because pointer values may not correctly represent boolean values when converted. Instead, cast thevoid*to an integer type likeuintptr_tbefore casting tobool. This ensures the correct boolean value is obtained.Apply the following changes to all affected functions:
-static void set_pal60_type (menu_t *menu, void *arg) { - menu->settings.pal60_enabled = (bool) (arg); +static void set_pal60_type (menu_t *menu, void *arg) { + menu->settings.pal60_enabled = (bool)(uintptr_t)(arg); settings_save(&menu->settings); } -static void set_protected_entries_type (menu_t *menu, void *arg) { - menu->settings.show_protected_entries = (bool) (arg); +static void set_protected_entries_type (menu_t *menu, void *arg) { + menu->settings.show_protected_entries = (bool)(uintptr_t)(arg); settings_save(&menu->settings); menu->browser.reload = true; } -static void set_use_saves_folder_type (menu_t *menu, void *arg) { - menu->settings.use_saves_folder = (bool) (arg); +static void set_use_saves_folder_type (menu_t *menu, void *arg) { + menu->settings.use_saves_folder = (bool)(uintptr_t)(arg); settings_save(&menu->settings); } -static void set_sound_enabled_type (menu_t *menu, void *arg) { - menu->settings.sound_enabled = (bool) (arg); +static void set_sound_enabled_type (menu_t *menu, void *arg) { + menu->settings.sound_enabled = (bool)(uintptr_t)(arg); sound_use_sfx(menu->settings.sound_enabled); settings_save(&menu->settings); } #ifdef BETA_SETTINGS -static void set_bgm_enabled_type (menu_t *menu, void *arg) { - menu->settings.bgm_enabled = (bool) (arg); +static void set_bgm_enabled_type (menu_t *menu, void *arg) { + menu->settings.bgm_enabled = (bool)(uintptr_t)(arg); settings_save(&menu->settings); } -static void set_rumble_enabled_type (menu_t *menu, void *arg) { - menu->settings.rumble_enabled = (bool) (arg); +static void set_rumble_enabled_type (menu_t *menu, void *arg) { + menu->settings.rumble_enabled = (bool)(uintptr_t)(arg); settings_save(&menu->settings); } #endifAlso, update the context menu definitions to cast the boolean values to
void*viauintptr_t:static component_context_menu_t set_pal60_type_context_menu = { .list = { - {.text = "On", .action = set_pal60_type, .arg = (void *) (true) }, - {.text = "Off", .action = set_pal60_type, .arg = (void *) (false) }, + {.text = "On", .action = set_pal60_type, .arg = (void *)(uintptr_t)(true) }, + {.text = "Off", .action = set_pal60_type, .arg = (void *)(uintptr_t)(false) }, COMPONENT_CONTEXT_MENU_LIST_END, }}; // Repeat the above changes for all other context menusAlso applies to: 19-24, 26-29, 31-35, 38-41, 44-47
55-91: 🛠️ Refactor suggestion
Refactor to reduce code duplication in context menu and setting functions
The code for creating context menus and setting functions for each option is repetitive. Consider refactoring by creating a generic function and data structures to handle these settings dynamically. This will improve maintainability and make it easier to add or modify settings in the future.
For example, you can define a structure to represent each setting and loop through an array to create context menus and setting functions.
src/menu/views/load_rom.c (1)
215-219: 🛠️ Refactor suggestion
Simplify toggle logic for 'show_extra_info_message'
The toggle logic for
show_extra_info_messagecan be simplified using logical negation, enhancing code readability.Apply this diff to simplify the code:
if (menu->actions.lz_context) { - if (show_extra_info_message) { - show_extra_info_message = false; - } else { - show_extra_info_message = true; - } + show_extra_info_message = !show_extra_info_message; sound_play_effect(SFX_SETTING); }Committable suggestion was skipped due to low confidence.
src/menu/rom_info.c (2)
813-815:
⚠️ Potential issuePrevent potential buffer overflow when copying ROM description
The use of
memcpywithout ensuring the source string fits within the destination buffer may lead to a buffer overflow ifrom_descriptionis longer thanrom_info->metadata.description.const char *rom_description = mini_get_string(ini, "metadata", "description", ""); memcpy(rom_info->metadata.description, rom_description, sizeof(rom_info->metadata.description));Consider using
strncpyorsnprintfto safely copy the string and ensure it is null-terminated.Apply this diff to fix the issue:
- memcpy(rom_info->metadata.description, rom_description, sizeof(rom_info->metadata.description)); + strncpy(rom_info->metadata.description, rom_description, sizeof(rom_info->metadata.description) - 1); + rom_info->metadata.description[sizeof(rom_info->metadata.description) - 1] = '\0';
842-846:
⚠️ Potential issueFree allocated memory on early return to prevent memory leaks
In the
save_overridefunction, ifmini_try_loadfails and returnsNULL, the allocatedrom_info_pathis not freed before returning, leading to a memory leak.mini_t *ini = mini_try_load(path_get(rom_info_path)); if (!ini) { return ROM_ERR_SAVE_IO; }Ensure that
rom_info_pathis freed before returning to prevent memory leaks.Apply this diff to fix the memory leak:
if (!ini) { + path_free(rom_info_path); return ROM_ERR_SAVE_IO; }
Improve layout handling
networkfusion
force-pushed
the
improve-rom-info
branch
from
a536060 to
3b0c497
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant enhancements to the handling of ROM information within the codebase. Key updates include the restructuring of the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/menu/rom_info.h (2)
226-235: Consider adding validation constraints for the description field.The new structures align well with the PR objectives. However, a few suggestions:
- Consider documenting the format/constraints for the description field (e.g., allowed characters, line breaks).
- Consider adding a constant for the description length (256) to make it configurable.
+/** @brief Maximum length for ROM description. */ +#define ROM_DESCRIPTION_MAX_LENGTH 256 /** @brief The ROM metadata. */ struct { - char description[256]; + char description[ROM_DESCRIPTION_MAX_LENGTH]; } metadata;
238-239: Add documentation for the new function declarations.The functions lack documentation comments. Consider adding Doxygen-style comments to match the documentation style used throughout the file.
+/** + * @brief Get the CIC seed for the ROM. + * @param rom_info Pointer to the ROM information structure. + * @param seed Pointer to store the CIC seed. + * @return true if successful, false otherwise. + */ bool rom_info_get_cic_seed (rom_info_t *rom_info, uint8_t *seed); + +/** + * @brief Load ROM information from a file. + * @param path Pointer to the path structure. + * @param rom_info Pointer to store the ROM information. + * @return ROM_OK on success, error code otherwise. + */ rom_err_t rom_info_load (path_t *path, rom_info_t *rom_info);src/menu/views/load_rom.c (1)
248-249: Simplify newline usage after description.The current implementation uses multiple newlines which might cause inconsistent spacing. Consider using a single newline for better maintainability.
- "Description:\n %s\n" - "\n\n\n\n\n\n\n" + "Description:\n %s\n" + "\n"src/menu/rom_info.c (1)
832-833: Specify a section name for settings keysIn the
mini_get_boolcalls forcheats_enabledandpatches_enabled, the section is set toNULL, which refers to the global section. For consistency with other settings read from specific sections like"metadata"and"custom_boot", consider specifying a section name such as"settings".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/menu/rom_info.c (5 hunks)
- src/menu/rom_info.h (3 hunks)
- src/menu/views/load_rom.c (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/menu/rom_info.h (1)
Line range hint
202-212: Verify the breaking changes in existing ROM ini files.The rename from
overridetoboot_overrideimproves clarity but introduces breaking changes. The PR objectives mention that override parameters have been moved to a new[custom_boot]section in ini files.Let's verify the impact on existing ROM ini files:
✅ Verification successful
Let's gather more specific information about the usage of the old 'override' structure in the codebase.
Based on the results, I can now generate the final response as we have sufficient information about the breaking changes and their implementation.
Breaking changes are properly implemented with backward compatibility
The rename from
overridetoboot_overrideis well-handled in the codebase:
- All references in
rom_info.care updated to useboot_override- The configuration is now read from the new
[custom_boot]section as intended- The code maintains proper initialization of override flags (all set to
falseby default)- The override functionality is properly encapsulated in getter/setter methods that handle the state consistently
The implementation ensures backward compatibility through:
- Default initialization of all override flags to
false- Using
AUTOMATICas the default value for all override types when reading from config- Proper handling of the new
[custom_boot]section in the configuration parser🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all ini files and check for existing 'override' usage # to assess the impact of breaking changes echo "Searching for ROM ini files with 'override' settings..." rg -l '\[.*override.*\]' --type ini echo "Checking for any hardcoded references to the old 'override' structure..." rg -l 'override\.' --type cpp --type cLength of output: 414
Script:
#!/bin/bash # Check the specific usage of 'override' in rom_info.c echo "Checking rom_info.c for 'override' references with context..." rg 'override\.' -B 2 -A 2 src/menu/rom_info.c # Look for any ini file patterns regardless of extension echo -e "\nSearching for potential ROM configuration files..." fd -e cfg -e conf -e ini -e config # Check for any ROM configuration related code echo -e "\nChecking for ROM configuration parsing code..." rg -l "ini|config|cfg" src/menu/rom_info.cLength of output: 3284
src/menu/views/load_rom.c (3)
125-127: LGTM!The inline function is well-implemented and follows the codebase's formatting pattern.
253-254: LGTM!The feature toggle display is well-formatted and consistently uses the boolean formatter.
Also applies to: 260-261
256-262: Verify handling of relocated override parameters.The PR objectives mention that override parameters have been moved to a new [custom_boot] section. Ensure that the ROM info loading function properly handles both old and new parameter locations for backward compatibility.
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient information about how the override parameters are handled.
Override parameters are properly handled in the new [custom_boot] section
The code shows proper implementation of the relocated override parameters:
- All override parameters (cic_type, save_type, tv_type) are now read from the [custom_boot] section in
rom_info.c- The code maintains proper state tracking through the
boot_overridestruct with boolean flags for each override- The getter functions (e.g.,
rom_info_get_cic_type) correctly handle both overridden and default values- The override functions (e.g.,
rom_info_override_cic_type) properly save to the new section usingsave_override🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ROM info loading and parameter handling code rg -A 5 "custom_boot" . rg -A 5 "override" .Length of output: 9831
src/menu/rom_info.c (1)
796-798: Initialization of new fields is appropriateInitializing
rom_info->metadata.description,rom_info->settings.cheats_enabled, androm_info->settings.patches_enabledto default values ensures that these fields have defined values before being used elsewhere.
| static const char *format_rom_description(char *description) { | ||
| return description[0] == '\0' ? "None." : description; | ||
| } |
There was a problem hiding this comment.
Add null check and const qualifier to description parameter.
The function should handle null descriptions and mark the input parameter as const since it's not modified.
-static const char *format_rom_description(char *description) {
+static const char *format_rom_description(const char *description) {
+ if (!description) return "None.";
return description[0] == '\0' ? "None." : description;
}📝 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.
| static const char *format_rom_description(char *description) { | |
| return description[0] == '\0' ? "None." : description; | |
| } | |
| static const char *format_rom_description(const char *description) { | |
| if (!description) return "None."; | |
| return description[0] == '\0' ? "None." : description; | |
| } |
|
|
||
| if (!ini) { | ||
| path_free(overrides_path); | ||
| path_free(rom_info_path); |
There was a problem hiding this comment.
Handle missing INI file by creating a new one
When mini_try_load fails to load an INI file (returns NULL), the code currently returns an error. Instead, you should initialize a new INI object using mini_new() to allow saving overrides even when the INI file does not exist.
Apply this diff to fix the issue:
if (!ini) {
- path_free(rom_info_path);
- return ROM_ERR_SAVE_IO;
+ ini = mini_new();
+ if (!ini) {
+ path_free(rom_info_path);
+ return ROM_ERR_SAVE_IO;
+ }
}Committable suggestion was skipped due to low confidence.
| const char *rom_description = mini_get_string(ini, "metadata", "description", ""); | ||
| memcpy(rom_info->metadata.description , rom_description, sizeof(rom_info->metadata.description)); | ||
|
|
There was a problem hiding this comment.
Prevent potential buffer overflow when copying the description
Using memcpy to copy rom_description into rom_info->metadata.description can lead to a buffer overflow if the source string is longer than the destination buffer. To ensure safety, consider using strncpy and explicitly null-terminating the destination buffer.
Apply this diff to fix the issue:
const char *rom_description = mini_get_string(ini, "metadata", "description", "");
-memcpy(rom_info->metadata.description, rom_description, sizeof(rom_info->metadata.description));
+strncpy(rom_info->metadata.description, rom_description, sizeof(rom_info->metadata.description) - 1);
+rom_info->metadata.description[sizeof(rom_info->metadata.description) - 1] = '\0';📝 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.
| const char *rom_description = mini_get_string(ini, "metadata", "description", ""); | |
| memcpy(rom_info->metadata.description , rom_description, sizeof(rom_info->metadata.description)); | |
| const char *rom_description = mini_get_string(ini, "metadata", "description", ""); | |
| strncpy(rom_info->metadata.description, rom_description, sizeof(rom_info->metadata.description) - 1); | |
| rom_info->metadata.description[sizeof(rom_info->metadata.description) - 1] = '\0'; |
networkfusion
force-pushed
the
develop
branch
from
6990621 to
fb0a04e
Compare
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Renames `components` to `ui_components` ## Motivation and Context <!--- What does this sample do? What problem does it solve? --> <!--- If it fixes/closes/resolves an open issue, please link to the issue here --> Makes the source more clear as to its functionality ## How Has This Been Tested? <!-- (if applicable) --> <!--- Please describe in detail how you tested your sample/changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots <!-- (if appropriate): --> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Improvement (non-breaking change that adds a new feature) - [ ] Bug fix (fixes an issue) - [ ] Breaking change (breaking change) - [ ] Documentation Improvement - [ ] Config and build (change in the configuration and build system, has no impact on code or features) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [ ] All new and existing tests passed. <!--- It would be nice if you could sign off your contribution by replacing the name with your GitHub user name and GitHub email contact. --> Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
networkfusion
force-pushed
the
develop
branch
from
7220d37 to
96506ed
Compare
Description
In this PR I have changed the ROM file that provides the "override" settings to be a more generic file that is able to handle more situations related to the ROM information.
The file can now contain the ROM description, and also contains whether the ROM enables GS and patch support (for future use).
To use it, a "group name" needs adding to the ROM specific
ini:which if availiable, is added to the ROM load screen.
Old ROM ini files will probably be broken as the
overrideparameters have also been added to a "group name"[custom_boot], so you would either need to manually adjust the file or re 'set' the overridden parameters in the menu.Motivation and Context
Works towards a "descriptive" ROM load system.
How Has This Been Tested?
Screenshots
Types of changes
Checklist:
Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation