Modification of PR 2547: Recent History #2660
Conversation
- Use symbolic constant for entry count - Remove explicit save - Add early return - Add an assert for invalid index - Remove invalid entries
(I forgot to upload it)
- Renamed History to Recent for consistency - Added a red X so that each entry can be manually deleted - Made it so the "Recent Discs" becomes grey and unclickable if no discs are in the list - Bumped the max disc in the list from 10 to 15 - Added something to clear just the missing discs from the list
Summary of ChangesHello @jyetcv, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a 'Recent Discs' feature, significantly enhancing user convenience by providing quick access to previously loaded disc images directly from the application's menu. This streamlines the process of switching between or revisiting frequently used discs, improving the overall user experience by reducing repetitive navigation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'Recent Discs' history feature. The implementation adds the necessary configuration, logic to manage the recent list when a disc is loaded, and a UI menu to display and interact with the history. My review focuses on improving code style and clarity. I've suggested using a const variable instead of a #define for better type safety in C++, and refactoring the ImGui menu rendering logic to be simpler and more readable. The core logic for managing the recent list appears correct, though complex.
ui/xui/actions.cc
Outdated
| #include "../xemu-notifications.h" | ||
| #include "snapshot-manager.hh" | ||
|
|
||
| #define MAX_RECENT_DISCS 11 |
There was a problem hiding this comment.
In C++, it's generally better to use const or constexpr variables instead of #define for constants. This provides type safety and respects scope. Consider defining MAX_RECENT_DISCS as a static const.
static const unsigned int MAX_RECENT_DISCS = 11;
ui/xui/menubar.cc
Outdated
| bool has_any_entries = (g_config.general.recent.discs_count > 0); | ||
| if (ImGui::BeginMenu("Recent Discs", has_any_entries)) { | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
|
|
||
| if (ImGui::MenuItem(filename)) { | ||
| ActionLoadDiscFile(disc_path); | ||
| } | ||
|
|
||
| g_free((void *)filename); | ||
| } | ||
| if (g_config.general.recent.discs_count > 0) { | ||
| ImGui::Separator(); | ||
| if (ImGui::MenuItem("Clear Recent Discs")) { | ||
| ActionClearDiscRecent(); | ||
| } | ||
| ImGui::EndMenu(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for displaying the 'Recent Discs' menu can be simplified for better readability and to follow ImGui's usage patterns more closely. The if (g_config.general.recent.discs_count > 0) check is redundant since ImGui::BeginMenu would have already returned false if the list were empty. Also, ImGui::EndMenu() should be called unconditionally at the end of the ImGui::BeginMenu block if it returned true. The indentation of some items is also inconsistent. The suggested change simplifies the logic and fixes the styling.
bool has_any_entries = (g_config.general.recent.discs_count > 0);
if (ImGui::BeginMenu("Recent Discs", has_any_entries)) {
for (int i = 0; i < g_config.general.recent.discs_count; i++) {
const char *disc_path = g_config.general.recent.discs[i];
const char *filename = g_path_get_basename(disc_path);
if (ImGui::MenuItem(filename)) {
ActionLoadDiscFile(disc_path);
}
g_free((void *)filename);
}
ImGui::Separator();
if (ImGui::MenuItem("Clear Recent Discs")) {
ActionClearDiscRecent();
}
ImGui::EndMenu();
}2 participants
Based on ehw's PR.
The code was simplified a little bit.
Feel free to modify whatever you want.