GNOME Bugzilla – Bug 701179
Add config variable to specify default file extension
Last modified: 2013-07-17 15:29:31 UTC
Created attachment 245526 [details] [review] 0001-Add-config-variable-to-specify-default-file-extensio.patch This is a first attempt to allow making non-png screenshots by default. Comments, suggestions very welcome. What this patch does is add a new setting org.gnome.gnome-screenshot default-file-type which defines the file type extension screenshots should be taken with. The rest is handled by gnome-screenshot transparently. So if you set this to jpg, screenshots should be taken in jpg format instead of png.
Review of attachment 245526 [details] [review]: Thanks, this looks mostly good to me; I only have a couple of minor comments below. ::: src/gnome-screenshot.convert @@ +6,3 @@ include-pointer = /apps/gnome-screenshot/include_pointer +border-effect = /apps/gnome-screenshot/border_effect +default-file-type = /apps/gnome-screenshot/default_file_type This option never existed in GConf, so it doesn't need to be migrated. ::: src/screenshot-filename-builder.c @@ +142,3 @@ + file_type = g_strdup (screenshot_config->file_type); + else + file_type = g_strdup ("png"); I don't think you need to strdup these; you can just make file_type a const gchar * and avoid the g_free() later.
Created attachment 245708 [details] [review] 0001-Add-config-variable-to-specify-default-file-extensio.patch v2 Thanks for the review and comments. The reason I used the g_strdup() is that I wanted to make sure we have a safe default in case the gconf setting is empty. I hope the new code is what you had in mind.
you can make it safe by making the setting an enum - then you know all possible values, and can safely assume you will always get 'png' or 'jpeg'
ok, I'll look into that option. If you have an example of how to use an enum that would be appreciated.
Created attachment 245742 [details] [review] 0001-Add-config-variable-to-specify-default-file-extensio.patch v3 Here's v3 with an enum.
Review of attachment 245742 [details] [review]: Thanks, this looks good except a minor comment. ::: src/org.gnome.gnome-screenshot.gschema.xml.in @@ +3,3 @@ + <value nick="bmp" value="0"/> + <value nick="jpg" value="1"/> + <value nick="jpeg" value="2"/> I don't think we need two different types for JPEG.
Created attachment 245811 [details] [review] 0001-Add-config-variable-to-specify-default-file-extensio.patch v4
Review of attachment 245811 [details] [review]: Looks good to me, thanks.
Cosimo, I don't see the v4 patch in the git repo. Any reason why you haven't committed it yet?
I wasn't aware you didn't have a GNOME git account to push this. I'll commit the patch for you later today.
Thanks a lot!
Pushed now.