Skip to content
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

Add thumbnail dimensions to config #3262

Merged
merged 6 commits into from
Jan 4, 2025
Merged

Add thumbnail dimensions to config #3262

merged 6 commits into from
Jan 4, 2025

Conversation

Cohee1207
Copy link
Member

Also groups config values for thumbnails in one area.

Checklist:

@Cohee1207 Cohee1207 added 🟩 PR - Small <100 lines changed 👷 Maintainer [ISSUE][PR] Posts by a maintainer or author of SillyTavern 🏭 Backend Changes [PR] Contains changes to the backend and/or API labels Jan 4, 2025
@Cohee1207 Cohee1207 requested a review from Wolfsblvt January 4, 2025 19:57
Copy link
Collaborator

@Wolfsblvt Wolfsblvt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as described.
Settings migration also works nicely. Tested like two different scenarios, didn't care for more.
image

Tested different dimensions too and it works if you change the values and delete the thumbnails.

Question though. Is it intentional to not have a basic level of config value validation?
Both for the quality value that existed before, and the new dimensions.
If you set a valid number but not a valid dimension (like a negative number) it runs into an error on thumbnail generation and uses the original size image.
If you set one dimension to 0, it generates a png that is broken and can't even be opened by an image viewer.

image
image

I think having basic value checking and using the defaults as fallbacks would be easy enough to implement.

@Cohee1207 Cohee1207 merged commit 8623d11 into staging Jan 4, 2025
2 checks passed
@Cohee1207 Cohee1207 deleted the thumbnail-config branch January 4, 2025 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏭 Backend Changes [PR] Contains changes to the backend and/or API 👷 Maintainer [ISSUE][PR] Posts by a maintainer or author of SillyTavern 🟩 PR - Small <100 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants