Created attachment 107050 [details] [review]
add more image type support

reassigning bug to the correct module

this patch should go in after we switched from the current custom dialog to a GtkFileChooserDialog, so we can use the extra widget field of that dialog.

the patch logic looks fine.

*** Bug 325187 has been marked as a duplicate of this bug. ***

*** Bug 552148 has been marked as a duplicate of this bug. ***

Last duplicate bug 552148 contains an UI mockup for this feature.

*** Bug 637651 has been marked as a duplicate of this bug. ***

*** Bug 346336 has been marked as a duplicate of this bug. ***

Created attachment 217573 [details] [review]
To determine image format by filename extension.

I made this patch to determine image format by filename extension.

Review of attachment 217573 [details] [review]:

thanks for your patch.

could you please attach the next iteration as a patch generated by `git format-patch`? see: https://live.gnome.org/Git/Developers#Contributing_patches - this would allow us to credit you properly.

rather than have format detection using the extension, I'd probably add a combo box with all the supported formats that can be used for writing to a file, though.

again, thanks for the patch!

::: gnome-screenshot-3.4.1.orig/src/screenshot-application.c
@@ +222,3 @@
+    extension++;
+
+  GSList *result = gdk_pixbuf_get_formats();

you should not be using C99 style declarations after statements.

@@ +223,3 @@
+
+  GSList *result = gdk_pixbuf_get_formats();
+  void foreach_print (gpointer data, gpointer user_data) {

internal functions are a GCC extensions; you should move it out and make it a regular function.

@@ +233,3 @@
+      }
+  }
+  g_slist_foreach (result, foreach_print, NULL);

the list returned by gdk_pixbuf_get_formats() should be freed using g_slist_free().

Created attachment 218569 [details] [review]
Revised Patch

Thanks for your review.
I made a revised patch.
Any feedback is welcome.

I think  removing '"tEXt::Software", "gnome-screenshot"' when creating the pixbuf is not a good idea. It should be kept as is for PNG. And eventually adapted for JPEG, TIFF.

Review of attachment 218569 [details] [review]:

Thanks for the updated patch. I have some comments below:

::: src/screenshot-application.c
@@ +213,3 @@
+  while (*ptr != NULL) {
+    if (g_strcmp0 (*ptr, *format) == 0) {
+  gchar **format  = (gchar **) user_data;

This function duplicates the string, so we'll leak it when the g_strcmp0() succeeds (we never call g_free() on extension in the following function).

@@ +237,3 @@
+    extension++;
+
+  if (extension == NULL)

Coding style: we usually declare variables at the top of a block...

@@ +238,3 @@
+
+  GSList *formats = gdk_pixbuf_get_formats();
+    extension = "png";

...but I don't really like this.

First of all, in case the extension is NULL or "png", we don't need to go through the whole format list, as the case is basically equivalent to the old code.
Also, I would remove the separate foreach() function and just use a for loop here, breaking the loop as soon as we find a format name that matches the extension.
Finally, I think we also need an error dialog in case the specified extension is not supported.

Another option would be to move the extension validation code to the save dialog, and make the "Save" action insensitive as long as the typed-in extension is not supported (possibly adding an inline message saying so to the dialog). This would avoid the need of a separate dialog in case of error, and is probably a better user experience too. If we do this, we should also treat the case where the user types in no extension at all by automatically saving to PNG.

@@ -239,2 @@
                                      save_pixbuf_ready_cb, self,
-                                     "tEXt::Software", "gnome-screenshot",

As already mentioned, we don't want to lose this information; it should still be saved in case the image is PNG. What happens if you pass this to GdkPixbuf when saving e.g. a JPEG image? If saving works and the option is just ignored by the backend, we might want to set it in all cases.

Review of attachment 218569 [details] [review]:

::: src/screenshot-application.c
@@ +209,3 @@
+{
+  gchar **format  = (gchar **) user_data;
+  gchar **extensions = gdk_pixbuf_format_get_extensions ((GdkPixbufFormat*) data);

Oh, another thing I think we should do is checking gdk_pixbuf_format_is_writable() before using it for the later checks.

"tEXt::Software" will make some formats failed to save, so I remove it from my previous patch.

(In reply to comment #15)
> "tEXt::Software" will make some formats failed to save, so I remove it from my
> previous patch.

Okay, then we should only still set it when the format is PNG.

I don't know why g_strcmp0() will leak it. http://developer.gnome.org/glib/2.32/glib-String-Utility-Functions.html#g-strcmp0

Created attachment 222948 [details] [review]
Determine image format by extension instead of a fixed PNG format.

I have revised my patch.
Please help to review it.
Any feedback is welcome, thanks.

(In reply to comment #17)
> I don't know why g_strcmp0() will leak it.
> http://developer.gnome.org/glib/2.32/glib-String-Utility-Functions.html#g-strcmp0

It's not g_strcmp0() that's leaking the string directly...you are doing

+    if (g_strcmp0 (*ptr, *format) == 0) {
+      *format = gdk_pixbuf_format_get_name ((GdkPixbufFormat*) data);
+      break;
+    }

That assigns a duplicated string to the memory location pointed to by gchar **format, which is the same pointer you pass here

+  g_slist_foreach (formats, find_format_from_extension, (gpointer) &extension);

But then you never call g_free() on extension.

Review of attachment 222948 [details] [review]:

::: src/screenshot-application.c
@@ +219,3 @@
+        {
+          *name = gdk_pixbuf_format_get_name (format);
+  gchar          **ptr        = extensions;

Still has the leak I pointed out in the previous comment

@@ +242,3 @@
+    extension = "png";
+  else
+    extension++;

You didn't address most of the comments I had about this part of the code in my review in comment #13.

@@ +265,3 @@
   if (self->priv->icc_profile_base64 != NULL)
+    {
+      if (g_strcmp0 (extension, "png") == 0)

I don't like the fact that we end up calling this function in 4 different blocks, but since we don't have any API In GdkPixbuf for non-varargs saving to stream I don't see any way around this unfortunately; also, the color profile is only supported by the PNG and TIFF backends in GdkPixbuf.

It would still be simpler (and more robust) to reverse the checks and do something like (pseudo-code)

if (is_png)
  {
    if (has_profile)
      save_with_description_and_profile()
    else
      save_with_description()
  }
else
  {
    save_with_no_profile_or_description()
  }

Created attachment 223435 [details] [review]
Determine image format by extension instead of a fixed PNG format.

I have revised my patch.
Please help to review it.
Any feedback is welcome, thanks.

Created attachment 223437 [details] [review]
Determine image format by extension instead of a fixed PNG format.

Sorry, I do a little modification.
Please help to review this patch.
Any feedback is welcome, thanks.

Review of attachment 223437 [details] [review]:

Thanks for the updated patch, looks better. I opened (with patches) bug 683063 for the non-varargs version of save_to_stream that would make this patch easier.
I have additional comments below:

::: src/screenshot-application.c
@@ +353,3 @@
+    {
+      if (has_profile (self))
+        save_with_profile (self, os, format);

As I explained in a previous comment, setting icc-profile won't work generically for all formats (even if gdk-pixbuf gained support for this in JPEG just yesterday).
We would also probably need API in GdkPixbuf to find out whether a specific format supports a given option.

In the meantime, my suggestion is to do one of these:
- change find_out_writable_format_by_extension() to also return a should_save_profile gboolean (which also takes into account whether self->priv->icc_profile_base64 is not NULL) - you'll need to move the list iteration inside the function for this
- such a boolean flag should be TRUE for the "tiff", "jpeg" and "png" formats
- in order for that to work on JPEGs we need to bump the required gdk-pixbuf version to git master (2.26.4)

or

- just call save_with_no_profile_or_description() if the image is not PNG for now, and leave everything else unchanged (probably the easiest)

Created attachment 223489 [details] [review]
Determine image format by extension instead of a fixed PNG format.

I suggest to choose the easier one, and polish it later.

Please help to review the patch in Comment 24.
Any feedback is welcome, thanks.

Review of attachment 223489 [details] [review]:

Thanks! This looks good to commit once we exit the feature freeze and branch for 3.6.

Thanks a lot. :)

Hi, when will exit the feature freeze and branch for 3.6?

In general: https://live.gnome.org/ThreePointSeven/ . We are out of hardcode freeze now, but branching is up to each maintainer.

Comment on attachment 223489 [details] [review]
Determine image format by extension instead of a fixed PNG format.

gnome-screenshot has branched already, this can be pushed to master now.

What should I do to push this commit to master?

(In reply to comment #31)
> What should I do to push this commit to master?

Have GNOME Git commit access. If you don't have please mention, so somebody else can commit for you.

*** Bug 699358 has been marked as a duplicate of this bug. ***

Attachment 223489 [details] pushed as 4e03093 - Determine image format by extension instead of a fixed PNG format.

Hello,

I have version 3.6.1 installed and still doesn't have this feature.

That's because it is supposed to be in a version newer than 3.6.