-
Notifications
You must be signed in to change notification settings - Fork 198
Screenshot functionality #340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Creates new functionality in `storage.py` to take screenshots. - Also rearranges some former `Storage()` functions to make working with multiple classes that need to work with files more ergonomic. - Fixed a bug in `storage.py` that would prevent saving manually from a game script. - Initialization of the `screenshot` instance under `storage.py` added to `runner.py`. - Added the function `screen.screenshot()` for manually controlled screenshots. - Added F12 as a default keybinding in `game.py` to take screenshots.
Locally flake8 doesn't have any problems anymore and the documentation is updated. Want to give it another go? |
There's a lot here, but I think it would be better if screenshots were saved to a standard "Pictures" directory for each platform, and I am hesitant about exposing screenshot() as an API and also giving it a default key binding. I think F12 should be sufficient. I don't think this needs to be wired through the Storage classes, but what were you aiming to achieve with that? |
We can also save to pictures directory of course. I used storage since it already had functionality useful for checking validity of directories and setting up proper paths per system. Since screenshots are files to be stored locally, I also thought it idiomatic to use the existing infrastructure and from my experience, games use different approaches. Some save screenshots to system picture folders, other do so in their own user data directories. Since there was no clear "standard" case, I went with this one. EDIT: Also if there's more stuff in the future that has to deal with files, having reworked storage.py means that it's easier to integrate coming stuff too. But of course this would only be true if The systems can be separated of course, but then we'd likely still need to import or rewrite those utility functions as far as I can see. If you would prefer that though, I'm not opposed to it. The user function being exposed was simply done to not restrict options. This way, if a user introduces a system of custom keybindings for example or just wants to give a different key as an easier to reach option where maybe you want to take a lot of screenshots, there's something provided already. I couldn't see any harm in introducing it, but again if you feel differently, I'm not inherently against removing it. |
I think we do need to save to "user library" directories because "~/.config/pgzero" is not a place anyone will find a screenshot. We also need to print the path it was saved to. |
Based on review by lordmauve, screenshots are now saved in the user home directory under the subfolder `Pictures`. Because this negates some of the former justification to make `storage.py` functionality more generalized, these changes were reverted unless still required with this change.
You're correct. I've made the changes. Screenshots now saved in user pictures directory, a message is printed stating the file name and directory and I've updated the documentation to also state the default keybinding F12. |
- Screenshots saved as `.png`. - Timestamp printed in ISO format. - All functionality decoupled from `storage.py` and moved to `screen.py`. - Using `sys.platform` instead of `platform.sys`. - Only recognized OSes use user folder for screenshot, otherwise warns and uses CWD. - Using `exist_ok` on `os.makedirs()` instead of passing errors manually. - Using F12 to take screenshots catches errors and prints a traceback. - User defined screenshots propagate the error instead. - Only prints info about screenshots when using F12, otherwise returns filepath to user.
Thanks for the review, I've implemented all your suggestions. I liked the idea of having a central spot for file system stuff at first, but you were right that this wasn't the time to start that by cobbling together individual parts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is pretty close, just a few style nits.
I'd like to have a few simple tests of the path computation and writing screenshots (maybe not to my real ~/Pictures
, we can unittest.mock.patch()
it to write to a TemporaryDirectory).
All suggestions from review have been incorporated, except for writing the tests.
Okay, I tried adding relevant tests for the screenshot functionality. This was my best effort, as I don't really know what I'm doing as soon as tests go beyond simple asserts. If this is unsatisfactory, I'm not sure I can improve upon it without very explicit instruction. While I would appreciate that, it might be the case that it would be less work overall if you edited the PR yourself then. |
Creates new functionality in
storage.py
to take screenshots.Storage()
functions to make working with multiple classes that need to work with files more ergonomic.storage.py
that would prevent saving manually from a game script.screenshot
instance understorage.py
added torunner.py
.screen.screenshot()
for manually controlled screenshots.game.py
to take screenshots.Fixes #290