ui/xui: Add "Recent" under Machine to keep track of recently loaded disc images #2547
Conversation
- Use symbolic constant for entry count - Remove explicit save - Add early return - Add an assert for invalid index - Remove invalid entries
|
This PR currently adds actions for managing a recent list, but doesn't actually have the mentioned UI changes? Maybe you did not stage the full set of changes? |
(I forgot to upload it)
My bad, I forgot to commit the changes for the menu bar. |
- 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
|
Sorry for the delay in making these changes. I believe I addressed everything in the review and made some more changes:
Here's what the list looks like now: Recent Discs grayed out with no discs in the list: |
mborgerson
left a comment
mborgerson
left a comment
There was a problem hiding this comment.
Appreciate the contribution, but in its current state I think it's too complex for a recent files list. Please see remarks inline.
| if (!disc_path) continue; | ||
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
| bool file_exists = qemu_access(disc_path, F_OK) != -1; |
There was a problem hiding this comment.
This gets called for every file in the list, for every frame that is rendered. It's also called above for every file. So each file is checked for existence ~120 times per second. I'm not sure the check is really even necessary, we can just error when someone selects a file that isn't present.
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | ||
|
|
||
| if (file_exists) { | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); |
There was a problem hiding this comment.
Lots of duplication here and below. I appreciate the thoughtfulness but I'm not sure why someone needs to curate their recent list so meticulously. Let's simplify this and just drop the ActionRemoveDiscFromRecent feature.
| ActionClearDiscRecent(); | ||
| } | ||
|
|
||
| if (ImGui::MenuItem("Clear Missing Recent Discs")) { |
There was a problem hiding this comment.
Same argument above, let's just drop the missing disc handling
| g_free((void *)filename); | ||
| } | ||
|
|
||
| if (g_config.general.recent.discs_count > 0) { |
There was a problem hiding this comment.
This will always be true, because you've disabled the menu when there are none to show
|
|
||
| void ActionLoadDiscFromRecent(unsigned int index) | ||
| { | ||
| g_assert(index < g_config.general.recent.discs_count); |
There was a problem hiding this comment.
Not sure why we need this? We can just call ActionLoadDiscFile in the menu handler
| error_free(err); | ||
| } else { | ||
| if (file_path && file_path[0]) { | ||
| if (!g_config.general.recent.discs) { |
|
Due to some unfortunate circumstances I can no longer work on this for the time being. Feel free to do whatever. |
|
No need to close. Let's just move it to draft state for now and I (or someone) will address the requested changes and you will get the deserved credit for the contribution! Thanks for the work! |
|
/gemini review |
![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' feature, which is a great addition for user convenience. The implementation is mostly solid, but there are a few areas that need attention, particularly concerning memory management in ActionLoadDiscFile which can lead to memory leaks. I've also identified opportunities to improve code maintainability by reducing duplication and enhancing efficiency in the UI rendering logic. My review includes specific suggestions to address these points.
| } else { | ||
| if (file_path && file_path[0]) { | ||
| if (!g_config.general.recent.discs) { | ||
| g_config.general.recent.discs = g_new0(const char *, 1); | ||
| g_config.general.recent.discs_count = 0; | ||
| } | ||
|
|
||
| int i; | ||
| for (i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| if (g_strcmp0(g_config.general.recent.discs[i], file_path) == 0) { | ||
| for (int j = i; j > 0; j--) { | ||
| g_config.general.recent.discs[j] = g_config.general.recent.discs[j-1]; | ||
| } | ||
| g_config.general.recent.discs[0] = g_strdup(file_path); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (i == g_config.general.recent.discs_count) { | ||
| if (g_config.general.recent.discs_count >= MAX_RECENT_DISCS) { | ||
| for (int j = MAX_RECENT_DISCS; j < g_config.general.recent.discs_count; j++) { | ||
| g_free((void*)g_config.general.recent.discs[j]); | ||
| } | ||
| g_config.general.recent.discs_count = MAX_RECENT_DISCS; | ||
| } else { | ||
| const char **new_discs = g_renew(const char *, g_config.general.recent.discs, | ||
| g_config.general.recent.discs_count + 1); | ||
| g_config.general.recent.discs = new_discs; | ||
| g_config.general.recent.discs_count++; | ||
| } | ||
|
|
||
| for (int i = g_config.general.recent.discs_count - 1; i > 0; i--) { | ||
| g_config.general.recent.discs[i] = g_config.general.recent.discs[i-1]; | ||
| } | ||
| g_config.general.recent.discs[0] = g_strdup(file_path); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic for managing the recent discs list has a couple of memory leaks:
- When a disc path that is already in the list is loaded again, the existing string pointer at that index is leaked because it's overwritten during the shift operation.
- When adding a new disc to a full list, the last element's string is not freed before being overwritten by the shift, also causing a leak.
The logic is also quite complex. I've suggested a refactored version that is both correct and easier to understand. It handles finding existing items, moving them to the front, and adding new items while correctly managing memory.
} else {
if (!file_path || !file_path[0]) {
return;
}
if (!g_config.general.recent.discs) {
g_config.general.recent.discs = g_new0(const char *, 0);
g_config.general.recent.discs_count = 0;
}
// Remove existing entry if it exists to re-add it at the front.
int found_idx = -1;
for (int i = 0; i < g_config.general.recent.discs_count; i++) {
if (g_strcmp0(g_config.general.recent.discs[i], file_path) == 0) {
found_idx = i;
break;
}
}
if (found_idx != -1) {
if (found_idx == 0) {
return; // Already at front.
}
g_free((void*)g_config.general.recent.discs[found_idx]);
memmove(&g_config.general.recent.discs[found_idx], &g_config.general.recent.discs[found_idx + 1], (g_config.general.recent.discs_count - found_idx - 1) * sizeof(const char *));
g_config.general.recent.discs_count--;
}
// Add new entry at the front.
if (g_config.general.recent.discs_count >= MAX_RECENT_DISCS) {
g_free((void*)g_config.general.recent.discs[MAX_RECENT_DISCS - 1]);
g_config.general.recent.discs_count--;
}
const char **new_discs = g_renew(const char *, g_config.general.recent.discs, g_config.general.recent.discs_count + 1);
g_config.general.recent.discs = new_discs;
if (g_config.general.recent.discs_count > 0) {
memmove(&g_config.general.recent.discs[1], &g_config.general.recent.discs[0], g_config.general.recent.discs_count * sizeof(const char *));
}
g_config.general.recent.discs[0] = g_strdup(file_path);
g_config.general.recent.discs_count++;
}| #include "../xemu-notifications.h" | ||
| #include "snapshot-manager.hh" | ||
|
|
||
| #define MAX_RECENT_DISCS 15 |
There was a problem hiding this comment.
For better type safety and to follow modern C++ practices, it's recommended to use static constexpr for compile-time constants instead of #define macros. This avoids potential issues with macro expansion and respects scope.
static constexpr int MAX_RECENT_DISCS = 15;| if (qemu_access(file_path, F_OK) == -1) { | ||
| g_free((void*)g_config.general.recent.discs[index]); | ||
| for (int j = index; j < g_config.general.recent.discs_count - 1; j++) { | ||
| g_config.general.recent.discs[j] = g_config.general.recent.discs[j + 1]; | ||
| } | ||
| g_config.general.recent.discs_count--; | ||
| if (g_config.general.recent.discs_count == 0) { | ||
| g_free(g_config.general.recent.discs); | ||
| g_config.general.recent.discs = NULL; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic to remove a disc from the recent list when it's found to be missing is duplicated from ActionRemoveDiscFromRecent. To improve maintainability and avoid code duplication, you can call ActionRemoveDiscFromRecent(index) here instead.
if (qemu_access(file_path, F_OK) == -1) {
ActionRemoveDiscFromRecent(index);
return;
}| int valid_count = 0; | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | ||
| g_free((void *)filename); | ||
|
|
||
| if (file_exists) { | ||
| valid_count++; | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates through the recent discs list just to count how many valid entries exist. The main loop for drawing the menu items then repeats the same file system checks (qemu_access) and string operations (g_path_get_basename). This is inefficient, especially with a longer list of recent files.
Consider iterating only once, caching the results (file existence and basename) in a temporary structure or arrays, and then using that cached information to both decide if the menu should be enabled and to draw the items. This will reduce redundant file system I/O and improve UI responsiveness.
| if (file_exists) { | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | ||
| ImGui::PushID(i + 1000); | ||
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | ||
| ActionRemoveDiscFromRecent(i); | ||
| } | ||
| ImGui::PopID(); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| ImGui::SameLine(); | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | ||
| if (ImGui::MenuItem(filename)) { | ||
| ActionLoadDiscFromRecent(i); | ||
| } | ||
| ImGui::PopStyleColor(); | ||
| } else { | ||
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | ||
|
|
||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | ||
| ImGui::PushID(i + 1000); | ||
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | ||
| ActionRemoveDiscFromRecent(i); | ||
| } | ||
| ImGui::PopID(); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| ImGui::SameLine(); | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | ||
| ImGui::Text("%s", missing_filename); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| g_free(missing_filename); | ||
| } | ||
|
|
There was a problem hiding this comment.
The code to display the 'X' button for removing a recent disc entry is duplicated inside both the if (file_exists) and else blocks. To reduce code duplication and improve readability, this common UI element can be factored out and placed before the conditional block.
ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255));
ImGui::PushID(i + 1000);
if (ImGui::Button("X", ImVec2(20.0f, 0))) {
ActionRemoveDiscFromRecent(i);
}
ImGui::PopID();
ImGui::PopStyleColor();
ImGui::SameLine();
if (file_exists) {
ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255));
if (ImGui::MenuItem(filename)) {
ActionLoadDiscFromRecent(i);
}
ImGui::PopStyleColor();
} else {
char *missing_filename = g_strconcat(filename, " (missing)", nullptr);
ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255));
ImGui::Text("%s", missing_filename);
ImGui::PopStyleColor();
g_free(missing_filename);
}There was a problem hiding this comment.
Pull request overview
This pull request adds a "Recent Discs" feature to the xemu UI that tracks and displays the last 15 recently loaded disc images under the Machine menu. The feature persists data in the xemu.toml configuration file and provides options to clear the history or remove missing entries.
Changes:
- Added recent discs tracking with a maximum of 15 entries
- Implemented UI menu showing recent discs with removal buttons and missing file indicators
- Added functions to load, remove, and clear recent disc entries
- Extended configuration schema to store recent discs array
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| ui/xui/menubar.cc | Added "Recent Discs" submenu with display logic for recent entries, removal buttons, and clear options |
| ui/xui/actions.hh | Added function declarations for recent discs management |
| ui/xui/actions.cc | Implemented recent discs tracking logic in ActionLoadDiscFile and added helper functions for managing the recent list |
| config_spec.yml | Added configuration schema for storing recent discs as an array of strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (g_strcmp0(g_config.general.recent.discs[i], file_path) == 0) { | ||
| for (int j = i; j > 0; j--) { | ||
| g_config.general.recent.discs[j] = g_config.general.recent.discs[j-1]; | ||
| } |
There was a problem hiding this comment.
Memory leak detected when moving an existing disc to the front of the list. The old pointer at position 0 is overwritten at line 75 without being freed first. When the disc already exists in the list, the pointer at position 0 is shifted but not freed, then a new duplicate is created with g_strdup. The old pointer should be freed before being replaced.
| if (g_strcmp0(g_config.general.recent.discs[i], file_path) == 0) { | |
| for (int j = i; j > 0; j--) { | |
| g_config.general.recent.discs[j] = g_config.general.recent.discs[j-1]; | |
| } | |
| if (g_strcmp0(g_config.general.recent.discs[i], file_path) == 0) { | |
| const char *old_disc = g_config.general.recent.discs[i]; | |
| for (int j = i; j > 0; j--) { | |
| g_config.general.recent.discs[j] = g_config.general.recent.discs[j-1]; | |
| } | |
| g_free((void *)old_disc); |
| int valid_count = 0; | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | ||
| g_free((void *)filename); | ||
|
|
||
| if (file_exists) { | ||
| valid_count++; | ||
| } | ||
| } | ||
|
|
||
| bool has_valid_entries = (valid_count > 0); | ||
| if (ImGui::BeginMenu("Recent Discs", has_valid_entries)) { | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | ||
|
|
||
| if (file_exists) { | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | ||
| ImGui::PushID(i + 1000); | ||
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | ||
| ActionRemoveDiscFromRecent(i); | ||
| } | ||
| ImGui::PopID(); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| ImGui::SameLine(); | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | ||
| if (ImGui::MenuItem(filename)) { | ||
| ActionLoadDiscFromRecent(i); | ||
| } | ||
| ImGui::PopStyleColor(); | ||
| } else { | ||
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | ||
|
|
||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | ||
| ImGui::PushID(i + 1000); | ||
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | ||
| ActionRemoveDiscFromRecent(i); | ||
| } | ||
| ImGui::PopID(); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| ImGui::SameLine(); | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | ||
| ImGui::Text("%s", missing_filename); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| g_free(missing_filename); | ||
| } | ||
|
|
||
| g_free((void *)filename); | ||
| } |
There was a problem hiding this comment.
Missing check for NULL or empty discs array before accessing it. If g_config.general.recent.discs is NULL or discs_count is 0, this code will still attempt to iterate, which could cause issues. Consider adding a guard condition before the loop.
| int valid_count = 0; | |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| const char *disc_path = g_config.general.recent.discs[i]; | |
| if (!disc_path) continue; | |
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| g_free((void *)filename); | |
| if (file_exists) { | |
| valid_count++; | |
| } | |
| } | |
| bool has_valid_entries = (valid_count > 0); | |
| if (ImGui::BeginMenu("Recent Discs", has_valid_entries)) { | |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| const char *disc_path = g_config.general.recent.discs[i]; | |
| if (!disc_path) continue; | |
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| if (file_exists) { | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | |
| if (ImGui::MenuItem(filename)) { | |
| ActionLoadDiscFromRecent(i); | |
| } | |
| ImGui::PopStyleColor(); | |
| } else { | |
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | |
| ImGui::Text("%s", missing_filename); | |
| ImGui::PopStyleColor(); | |
| g_free(missing_filename); | |
| } | |
| g_free((void *)filename); | |
| } | |
| bool has_valid_entries = false; | |
| if (g_config.general.recent.discs && g_config.general.recent.discs_count > 0) { | |
| int valid_count = 0; | |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| const char *disc_path = g_config.general.recent.discs[i]; | |
| if (!disc_path) continue; | |
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| g_free((void *)filename); | |
| if (file_exists) { | |
| valid_count++; | |
| } | |
| } | |
| has_valid_entries = (valid_count > 0); | |
| if (ImGui::BeginMenu("Recent Discs", has_valid_entries)) { | |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| const char *disc_path = g_config.general.recent.discs[i]; | |
| if (!disc_path) continue; | |
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| if (file_exists) { | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | |
| if (ImGui::MenuItem(filename)) { | |
| ActionLoadDiscFromRecent(i); | |
| } | |
| ImGui::PopStyleColor(); | |
| } else { | |
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | |
| ImGui::Text("%s", missing_filename); | |
| ImGui::PopStyleColor(); | |
| g_free(missing_filename); | |
| } | |
| g_free((void *)filename); | |
| } | |
| ImGui::EndMenu(); | |
| } | |
| } |
| int valid_count = 0; | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
| if (qemu_access(disc_path, F_OK) != -1) { | ||
| valid_count++; | ||
| } | ||
| } | ||
|
|
||
| const char **valid_discs = NULL; | ||
| if (valid_count > 0) { | ||
| valid_discs = (const char **)g_new0(const char *, valid_count); |
There was a problem hiding this comment.
Missing check for NULL or empty discs array. If g_config.general.recent.discs is NULL, this code will still attempt to iterate, which could cause a crash. Consider adding a guard condition before the loop, similar to line 64 where initialization is checked.
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| g_free((void*)g_config.general.recent.discs[i]); | ||
| } | ||
| g_free(g_config.general.recent.discs); | ||
| g_config.general.recent.discs = NULL; | ||
| g_config.general.recent.discs_count = 0; | ||
| } | ||
|
|
||
| void ActionClearMissingRecentDiscs(void) | ||
| { |
There was a problem hiding this comment.
Missing NULL check for discs array. If g_config.general.recent.discs is NULL, the loop will still attempt to iterate with discs_count, which could be a non-zero stale value. Add a guard condition to check if the array is NULL before iteration.
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| g_free((void*)g_config.general.recent.discs[i]); | |
| } | |
| g_free(g_config.general.recent.discs); | |
| g_config.general.recent.discs = NULL; | |
| g_config.general.recent.discs_count = 0; | |
| } | |
| void ActionClearMissingRecentDiscs(void) | |
| { | |
| if (g_config.general.recent.discs) { | |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| g_free((void*)g_config.general.recent.discs[i]); | |
| } | |
| g_free(g_config.general.recent.discs); | |
| } | |
| g_config.general.recent.discs = NULL; | |
| g_config.general.recent.discs_count = 0; | |
| } | |
| void ActionClearMissingRecentDiscs(void) | |
| { | |
| if (!g_config.general.recent.discs || g_config.general.recent.discs_count == 0) { | |
| g_free(g_config.general.recent.discs); | |
| g_config.general.recent.discs = NULL; | |
| g_config.general.recent.discs_count = 0; | |
| return; | |
| } |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | ||
|
|
||
| if (file_exists) { | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | ||
| ImGui::PushID(i + 1000); | ||
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | ||
| ActionRemoveDiscFromRecent(i); | ||
| } | ||
| ImGui::PopID(); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| ImGui::SameLine(); | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | ||
| if (ImGui::MenuItem(filename)) { | ||
| ActionLoadDiscFromRecent(i); | ||
| } | ||
| ImGui::PopStyleColor(); | ||
| } else { | ||
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | ||
|
|
||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | ||
| ImGui::PushID(i + 1000); | ||
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | ||
| ActionRemoveDiscFromRecent(i); | ||
| } | ||
| ImGui::PopID(); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| ImGui::SameLine(); | ||
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | ||
| ImGui::Text("%s", missing_filename); | ||
| ImGui::PopStyleColor(); | ||
|
|
||
| g_free(missing_filename); | ||
| } | ||
|
|
||
| g_free((void *)filename); | ||
| } | ||
|
|
||
| if (g_config.general.recent.discs_count > 0) { | ||
| ImGui::Separator(); | ||
|
|
||
| if (ImGui::MenuItem("Clear Recent Discs")) { | ||
| ActionClearDiscRecent(); | ||
| } | ||
|
|
||
| if (ImGui::MenuItem("Clear Missing Recent Discs")) { | ||
| ActionClearMissingRecentDiscs(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing check for NULL or empty discs array before accessing it. If g_config.general.recent.discs is NULL, this code will still attempt to iterate, which could cause a crash. The second loop should have the same guard condition as needed for the first loop.
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| const char *disc_path = g_config.general.recent.discs[i]; | |
| if (!disc_path) continue; | |
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| if (file_exists) { | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | |
| if (ImGui::MenuItem(filename)) { | |
| ActionLoadDiscFromRecent(i); | |
| } | |
| ImGui::PopStyleColor(); | |
| } else { | |
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | |
| ImGui::Text("%s", missing_filename); | |
| ImGui::PopStyleColor(); | |
| g_free(missing_filename); | |
| } | |
| g_free((void *)filename); | |
| } | |
| if (g_config.general.recent.discs_count > 0) { | |
| ImGui::Separator(); | |
| if (ImGui::MenuItem("Clear Recent Discs")) { | |
| ActionClearDiscRecent(); | |
| } | |
| if (ImGui::MenuItem("Clear Missing Recent Discs")) { | |
| ActionClearMissingRecentDiscs(); | |
| } | |
| } | |
| if (g_config.general.recent.discs && g_config.general.recent.discs_count > 0) { | |
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | |
| const char *disc_path = g_config.general.recent.discs[i]; | |
| if (!disc_path) continue; | |
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| if (file_exists) { | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 255, 255, 255)); | |
| if (ImGui::MenuItem(filename)) { | |
| ActionLoadDiscFromRecent(i); | |
| } | |
| ImGui::PopStyleColor(); | |
| } else { | |
| char *missing_filename = g_strconcat(filename, " (missing)", nullptr); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(255, 0, 0, 255)); | |
| ImGui::PushID(i + 1000); | |
| if (ImGui::Button("X", ImVec2(20.0f, 0))) { | |
| ActionRemoveDiscFromRecent(i); | |
| } | |
| ImGui::PopID(); | |
| ImGui::PopStyleColor(); | |
| ImGui::SameLine(); | |
| ImGui::PushStyleColor(ImGuiCol_Text, IM_COL32(128, 128, 128, 255)); | |
| ImGui::Text("%s", missing_filename); | |
| ImGui::PopStyleColor(); | |
| g_free(missing_filename); | |
| } | |
| g_free((void *)filename); | |
| } | |
| } | |
| if (g_config.general.recent.discs_count > 0) { | |
| ImGui::Separator(); | |
| if (ImGui::MenuItem("Clear Recent Discs")) { | |
| ActionClearDiscRecent(); | |
| } | |
| if (ImGui::MenuItem("Clear Missing Recent Discs")) { | |
| ActionClearMissingRecentDiscs(); | |
| } | |
| } |
|
|
||
| const char *filename = g_path_get_basename(disc_path); | ||
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | ||
| g_free((void *)filename); | ||
|
|
||
| if (file_exists) { | ||
| valid_count++; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicated file existence check. The filename is extracted and freed immediately without being used, and the file_exists check result is discarded. This duplicates the work done in the second loop (lines 161-203) and wastes resources by getting the basename and freeing it without using it.
| const char *filename = g_path_get_basename(disc_path); | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| g_free((void *)filename); | |
| if (file_exists) { | |
| valid_count++; | |
| } | |
| } | |
| bool file_exists = qemu_access(disc_path, F_OK) != -1; | |
| if (file_exists) { | |
| valid_count++; | |
| } | |
| } |
| void ActionClearMissingRecentDiscs(void) | ||
| { | ||
| int valid_count = 0; | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
| if (qemu_access(disc_path, F_OK) != -1) { | ||
| valid_count++; | ||
| } | ||
| } | ||
|
|
||
| const char **valid_discs = NULL; | ||
| if (valid_count > 0) { | ||
| valid_discs = (const char **)g_new0(const char *, valid_count); | ||
|
|
||
| int valid_index = 0; | ||
| for (int i = 0; i < g_config.general.recent.discs_count; i++) { | ||
| const char *disc_path = g_config.general.recent.discs[i]; | ||
| if (!disc_path) continue; | ||
| if (qemu_access(disc_path, F_OK) != -1) { | ||
| valid_discs[valid_index++] = disc_path; | ||
| } else { | ||
| g_free((void*)disc_path); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| g_free(g_config.general.recent.discs); | ||
| g_config.general.recent.discs = valid_discs; | ||
| g_config.general.recent.discs_count = valid_count; | ||
| } |
There was a problem hiding this comment.
Missing free for all invalid disc paths when valid_count is 0. When no valid discs remain, the function should still free all the invalid disc paths before setting discs to NULL. Currently, if valid_count is 0, the code skips the loop that frees invalid paths (lines 221-229), resulting in memory leaks for all the disc path strings.
| if (i == g_config.general.recent.discs_count) { | ||
| if (g_config.general.recent.discs_count >= MAX_RECENT_DISCS) { | ||
| for (int j = MAX_RECENT_DISCS; j < g_config.general.recent.discs_count; j++) { | ||
| g_free((void*)g_config.general.recent.discs[j]); | ||
| } | ||
| g_config.general.recent.discs_count = MAX_RECENT_DISCS; | ||
| } else { | ||
| const char **new_discs = g_renew(const char *, g_config.general.recent.discs, | ||
| g_config.general.recent.discs_count + 1); | ||
| g_config.general.recent.discs = new_discs; | ||
| g_config.general.recent.discs_count++; | ||
| } | ||
|
|
||
| for (int i = g_config.general.recent.discs_count - 1; i > 0; i--) { | ||
| g_config.general.recent.discs[i] = g_config.general.recent.discs[i-1]; | ||
| } | ||
| g_config.general.recent.discs[0] = g_strdup(file_path); | ||
| } |
There was a problem hiding this comment.
The condition on line 80 will always be true when reached. The variable 'i' is only incremented in the for loop and exits with i == g_config.general.recent.discs_count. This check is redundant and can be removed.
| if (i == g_config.general.recent.discs_count) { | |
| if (g_config.general.recent.discs_count >= MAX_RECENT_DISCS) { | |
| for (int j = MAX_RECENT_DISCS; j < g_config.general.recent.discs_count; j++) { | |
| g_free((void*)g_config.general.recent.discs[j]); | |
| } | |
| g_config.general.recent.discs_count = MAX_RECENT_DISCS; | |
| } else { | |
| const char **new_discs = g_renew(const char *, g_config.general.recent.discs, | |
| g_config.general.recent.discs_count + 1); | |
| g_config.general.recent.discs = new_discs; | |
| g_config.general.recent.discs_count++; | |
| } | |
| for (int i = g_config.general.recent.discs_count - 1; i > 0; i--) { | |
| g_config.general.recent.discs[i] = g_config.general.recent.discs[i-1]; | |
| } | |
| g_config.general.recent.discs[0] = g_strdup(file_path); | |
| } | |
| if (g_config.general.recent.discs_count >= MAX_RECENT_DISCS) { | |
| for (int j = MAX_RECENT_DISCS; j < g_config.general.recent.discs_count; j++) { | |
| g_free((void*)g_config.general.recent.discs[j]); | |
| } | |
| g_config.general.recent.discs_count = MAX_RECENT_DISCS; | |
| } else { | |
| const char **new_discs = g_renew(const char *, g_config.general.recent.discs, | |
| g_config.general.recent.discs_count + 1); | |
| g_config.general.recent.discs = new_discs; | |
| g_config.general.recent.discs_count++; | |
| } | |
| for (int i = g_config.general.recent.discs_count - 1; i > 0; i--) { | |
| g_config.general.recent.discs[i] = g_config.general.recent.discs[i-1]; | |
| } | |
| g_config.general.recent.discs[0] = g_strdup(file_path); |
| for (int i = g_config.general.recent.discs_count - 1; i > 0; i--) { | ||
| g_config.general.recent.discs[i] = g_config.general.recent.discs[i-1]; |
There was a problem hiding this comment.
Variable shadowing: The loop variable 'i' shadows the outer variable 'i' declared at line 69. This makes the code harder to read and maintain. Consider using a different variable name for the inner loop, such as 'j' or 'k'.
| for (int i = g_config.general.recent.discs_count - 1; i > 0; i--) { | |
| g_config.general.recent.discs[i] = g_config.general.recent.discs[i-1]; | |
| for (int j = g_config.general.recent.discs_count - 1; j > 0; j--) { | |
| g_config.general.recent.discs[j] = g_config.general.recent.discs[j-1]; |
3 participants
Disclaimer: I had help in implementing this with minimax ai.
Adds an item under Machine in the menu bar at the top that lists the last 10 recently opened disc images (let me know if you want more, could go a little higher). If a disc image is deleted, it's greyed out so the end user can't click on it and open it by mistake. An option to clear history is added to the bottom if the user wants to remove all the entries without editing their xemu.toml file. Entries are saved in the xemu.toml file.
Tested under Windows, needs testing under macOS/Linux just to make sure it all works properly.
Fixes #2544 .