GNOME Bugzilla – Bug 683371
Add API to query whether a save option key is supported
Last modified: 2016-08-02 19:19:42 UTC
The gdk_pixbuf_save_* family of functions has two keys/values arrays to specify the backend a set of metadata options to be used when creating the pixbuf. Those keys and values are backend-implementation specific, but some times they're also shared by more than one backend (e.g. "icc-profile"). It would be nice for applications being able to discover which options are available before saving (and risk getting a BAD_OPTION error back, or spawning a warning); the proposed API allows that.
Created attachment 223476 [details] [review] gdk-pixbuf: add gdk_pixbuf_format_is_option_supported() API This is useful when you want to set an option (e.g. icc-profile) that might be supported by different formats, but not all of them.
Created attachment 223477 [details] [review] modules: implement is_option_supported() for builtin modules Follow-up to previous commit; implement the new API in the modules we ship that can save pixbufs.
Review of attachment 223476 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +3225,3 @@ + module = _gdk_pixbuf_get_named_module (format->name, NULL); + if (!module) + goto out; return FALSE directly. @@ +3230,3 @@ + goto out; + + if (module->is_option_supported) return FALSE directly if !module->is_option_supported. @@ +3231,3 @@ + + if (module->is_option_supported) + retval = (* module->is_option_supported) (option_key); and return directly from here. ::: gdk-pixbuf/gdk-pixbuf-io.h @@ +247,3 @@ GError **error); + gboolean (* save_to_callback) (GdkPixbufSaveFunc save_func, Whitespace changes?
Review of attachment 223477 [details] [review]: ::: gdk-pixbuf/io-png.c @@ +1088,3 @@ + if (g_strcmp0 (option_key, "compression") == 0 || + g_strcmp0 (option_key, "icc-profile") == 0 || + strncmp (option_key, "tEXt::", 6) == 0) Will crash if option_key is NULL, though that check should probably be in the other patch.
Review of attachment 223476 [details] [review]: ::: gdk-pixbuf/gdk-pixbuf-io.h @@ +51,3 @@ gchar **gdk_pixbuf_format_get_mime_types (GdkPixbufFormat *format); gchar **gdk_pixbuf_format_get_extensions (GdkPixbufFormat *format); +gboolean gdk_pixbuf_format_is_option_supported is_save_option_supported()? @@ +254,3 @@ + GError **error); + + gboolean (* is_option_supported) (const gchar *option_key); is_save_option_supported() instead?
Created attachment 289224 [details] [review] gdk-pixbuf: add gdk_pixbuf_format_is_save_option_supported() API -- Addressed review comments and renamed the API.
Created attachment 289225 [details] [review] modules: implement is_option_supported() for builtin modules -- Update for new API
Review of attachment 289225 [details] [review]: Please review all the loaders you're modifying here for which options are now supported. The patch here isn't up-to-date. ::: gdk-pixbuf/io-tiff.c @@ +913,3 @@ +{ + if (g_strcmp0 (option_key, "compression") == 0 || + g_strcmp0 (option_key, "icc-profile") == 0) This is wrong: while (keys[i]) { if (g_str_equal (keys[i], "bits-per-sample")) bits_per_sample = values[i]; else if (g_str_equal (keys[i], "compression")) compression = values[i]; else if (g_str_equal (keys[i], "icc-profile")) icc_profile = values[i]; else if (g_str_equal (keys[i], "x-dpi")) x_dpi = values[i]; else if (g_str_equal (keys[i], "y-dpi")) y_dpi = values[i]; i++; } There's many more options than just those 2.
Review of attachment 289224 [details] [review]: That looks fine, overall.
Created attachment 291486 [details] [review] modules: implement is_option_supported() for builtin modules -- This should do it.
Bastien, any chance we can get it in this cycle?
Attachment 289224 [details] pushed as 6385d35 - gdk-pixbuf: add gdk_pixbuf_format_is_save_option_supported() API Attachment 291486 [details] pushed as 1883b72 - modules: implement is_option_supported() for builtin modules