Add Take Screenshot option - #1908
Conversation
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces screenshot functionality within the GUI. The changes add a new "Take Screenshot" menu item that triggers screenshot capture via the emulator's GPU. New methods are implemented to convert the captured screenshot into an image, generate a timestamp for unique file naming, and write the image to a PNG file. Both source and header files have been updated to include these new functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GUI
participant GPU
participant FS
User->>GUI: Select "Take Screenshot"
GUI->>GPU: Capture current screen data
GPU-->>GUI: Return screenshot data
GUI->>GUI: Convert screenshot to image format
GUI->>GUI: Generate timestamped filename
GUI->>FS: Write PNG image file
FS-->>GUI: Confirm file saved
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/gui/gui.h (1)
432-434: Consider moving helper functions to a shared utility.These helper functions are currently duplicated from the webserver module. To avoid code duplication and potential maintenance issues, consider moving these to a shared utility class or namespace that both the GUI and webserver can use.
- std::string getDateString(); - clip::image convertScreenshotToImage(PCSX::GPU::ScreenShot&& screenshot); - bool writeImagePNG(std::string filename, clip::image&& img);These functions could be moved to a new utility namespace, for example:
// In a new header file like src/utils/screenshot_utils.h namespace PCSX::Utils { std::string getDateString(); clip::image convertScreenshotToImage(GPU::ScreenShot&& screenshot); bool writeImagePNG(std::string filename, clip::image&& img); }src/gui/gui.cc (3)
2931-2943: Consider adding more flexibility to date format.The date format is currently hardcoded. Consider making it configurable or moving it to a constant to make it easier to change in the future.
- ss << std::put_time(local_time, "%Y-%m-%d-%H-%M-%S"); + constexpr const char* SCREENSHOT_DATE_FORMAT = "%Y-%m-%d-%H-%M-%S"; + ss << std::put_time(local_time, SCREENSHOT_DATE_FORMAT);
2945-2953: Add user feedback for screenshot operations.The method returns a success status, but there's no visible feedback to the user when a screenshot is taken. Consider adding a notification when a screenshot is successfully saved or if it fails.
bool PCSX::GUI::saveScreenShot() { std::filesystem::path path = g_system->getPersistentDir() / (getSaveStatePrefix(true) + getDateString() + ".png"); auto screenshot = g_emulator->m_gpu->takeScreenShot(); clip::image img = convertScreenshotToImage(std::move(screenshot)); bool success = writeImagePNG(path.string(), std::move(img)); + + if (success) { + addNotification(fmt::format(_("Screenshot saved to {}"), path.string())); + } else { + addNotification(_("Failed to save screenshot")); + } return success; }
2945-2953: Consider adding a hotkey for the screenshot feature.As mentioned in your PR description, adding a hotkey (like F12) would improve usability. This would require adding a key handler in the
startFramemethod similar to other hotkeys.Would you like me to suggest an implementation for the screenshot hotkey?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gui/gui.cc(2 hunks)src/gui/gui.h(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/gui/gui.cc (3)
src/gui/gui.h (6) (6)
screenshot(433:433)filename(411:411)filename(412:412)filename(413:413)filename(423:423)filename(434:434)src/core/web-server.cc (4) (4)
screenshot(713:742)screenshot(713:713)filename(743:743)filename(743:743)src/core/gpu.h (2) (2)
g_emulator(62:62)g_emulator(63:63)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: build-openbios
- GitHub Check: asan
🔇 Additional comments (4)
src/gui/gui.h (2)
30-31: Necessary includes for the screenshot functionality.These includes support date formatting and string operations needed for the new screenshot feature.
424-424: Appropriate public interface for screenshot functionality.The public method provides a clean interface for the screenshot feature, returning a boolean to indicate success or failure.
src/gui/gui.cc (2)
1321-1323: Good placement for the screenshot menu item.Adding the screenshot option to the Emulation menu is intuitive and follows standard application conventions.
2898-2927: Check color channel mask values.There appears to be a difference in the red and blue mask values compared to the webserver implementation. The comments suggest that the values might have been intentionally swapped. Verify that this is correct for the GUI's needs.
- spec.red_mask = 0x1f; // 0x7c00; - spec.blue_mask = 0x7c00; // 0x1f; + spec.red_mask = 0x7c00; + spec.blue_mask = 0x1f;Original webserver implementation for reference:
spec.red_mask = 0x7c00; spec.blue_mask = 0x1f;
|
Bit busy this week, I'll look at the various PRs over the week-end. |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/gui/gui.h:432
- [nitpick] Since these helper functions are now used in multiple modules, consider renaming or relocating them into a common utilities module to prevent code duplication and ease future maintenance.
std::string getDateString();
| auto local_time = std::localtime(&in_time_t); | ||
|
|
||
| std::stringstream ss; | ||
| ss << std::put_time(local_time, "%Y-%m-%d-%H-%M-%S"); |
There was a problem hiding this comment.
It appears that is needed for std::put_time to function correctly. Please add the necessary include to avoid potential compile errors.
|
Sorry, took a while to get back to you, lots of real life stuff. So, yeah, it might be relevant to create a new file, like screenshot.h/screenshot.cc to factorize this a bit. We can create a few static functions in a namespace like For the date helper, it probably would belong into the screenshot namespace for now? We can move it if needs be if we start re-using it elsewhere maybe? For the hotkey, sure, I don't mind. Note there's already a hotkey to take a screenshot into the clipboard, and the code probably needs to be refactored with the new screenshot code too: Lines 1095 to 1125 in 34ea4f1 |
This adds a Take Screenshot option to the Emulation menu.
I want to stress that this is a tentative implementation and I would like to request comments on the following: