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

config: Move setUseOptipng call after we determine if optipng is available #93

Merged
merged 1 commit into from Jun 3, 2021

Conversation

iainlane
Copy link
Collaborator

@iainlane iainlane commented Jun 3, 2021

Currently we tell AsCompose to use optipng just based on the value of
the feature flag. AsCompose will respect what we tell it without further
checking. If optipng is in fact not available then we'll crash later on
when we first try to use it.

Fix this by setting the flag after we check if optipng is installed.

…lable

Currently we tell AsCompose to use optipng just based on the value of
the feature flag. AsCompose will respect what we tell it without further
checking. If optipng is in fact not available then we'll crash later on
when we first try to use it.

Fix this by setting the flag *after* we check if optipng is installed.
@iainlane
Copy link
Collaborator Author

iainlane commented Jun 3, 2021

btw I think that crashing is kind of dodgy behaviour. Should asc_globals_set_use_optipng be lookng at priv->optipng_bin and refusing to set to TRUE if that is NULL (logging a critical)?

@ximion
Copy link
Owner

ximion commented Jun 3, 2021

btw I think that crashing is kind of dodgy behaviour. Should asc_globals_set_use_optipng be lookng at priv->optipng_bin and refusing to set to TRUE if that is NULL (logging a critical)?

Oh, it absolutely should do that! Crashing is never acceptable (unless it's an assertion in tests or a g_error in other code).
Thanks for the patch!

@ximion ximion merged commit 8de864d into ximion:master Jun 3, 2021
3 checks passed
@iainlane iainlane deleted the fix-optipng-usage branch June 3, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants