-
Notifications
You must be signed in to change notification settings - Fork 222
[New Feature] Multi-resolution screenshot generator #747
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
Open
kdmukai
wants to merge
28
commits into
SeedSigner:dev
Choose a base branch
from
kdmukai:multiresolution_screenshot_generator
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
[New Feature] Multi-resolution screenshot generator #747
kdmukai
wants to merge
28
commits into
SeedSigner:dev
from
kdmukai:multiresolution_screenshot_generator
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Still seems a bit off (overshoots the horizontal center to the right), but close enough.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Adds support for generating screenshots beyond our original 240x240 resolution.
SettingsConstants
.--resolution
command line option to render any new arbitrary resolution. Note that our baseline 240x240 resolution is always rendered, regardless of this option.--no-clean
option to prevent wiping out the screenshots dir before generating.generator.py
to treat it as a class-based test case. This provides a clear universal setup/teardown, regardless of how many screenshot combinations (locale @ resolution) are generated.Discussion items
I needed to be able to track state in the screenshot generator across individual locale / resolution invocations within a given run. Now that it's been refactored into a class based test, I opted to use some slightly hack-y class-level attrs for that state management (
locale_combos
,have_cleaned_screenshots_dir
). I can't decide if it's clever, ugly, or somewhere inbetween.Default behavior: clean the screenshots dir. Many times in the past I've been confused by a screenshot that turned out to just be stale that wasn't part of the most recent run. But on the flip side, there have definitely been times when I found it convenient to keep prior screenshots around (e.g. screenshots for a new locale I'm testing), even if they did risk becoming a bit stale. So the
--no-clean
option gives us flexibility. But either way, the final README will only link up the screenshots from that particular run.Future TODO items
As we get deeper into supporting a wider variety of screens, it'll be easier to figure out how to clean up the slightly half-baked organizational overlap between
SettingsConstants.ALL_DISPLAY_CONFIGURATIONS
and thedisplay_driver.ALL_DISPLAY_TYPES
constants. It's currently a little redundant but it's not horrible enough to need refactoring right now.The final README hard codes four of the English 240x240 screenshots to enhance the README's presentation. But the
--locale
option plus the default dir clean means that those screenshots aren't guaranteed to exist. A future enhancement would be to make those preview embeds dynamic.This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: