[virt-tools-list] [PATCH] remote-viewer: add a default extension to screenshot filenames

[Julien ROPE]

Hi,

Thanks for reviewing :)


Le 21/01/2020 à 13:55, Victor Toso a écrit :
> Hi,
>
> On Fri, Dec 13, 2019 at 11:16:23AM +0100, Julien Ropé wrote:
>> From: Julien ROPE <jrope at redhat.com>
>>
>> When doing a screenshot, if the user provides a filename without a file
>> extension, an error occurs because the image format could not be determined.
>> This patch adds a .png extension to such filenames, so that there is a default
>> file format for screenshots.
> Sure,
>
>> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1752514
>>
>> Signed-off-by: Julien Ropé <jrope at redhat.com>
>> ---
>>  src/virt-viewer-window.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>> index 4c08423..f58ebad 100644
>> --- a/src/virt-viewer-window.c
>> +++ b/src/virt-viewer-window.c
>> @@ -1069,6 +1069,14 @@ virt_viewer_window_menu_file_screenshot(GtkWidget *menu G_GNUC_UNUSED,
>>          GError *error = NULL;
>>  
>>          filename = gtk_file_chooser_get_filename(GTK_FILE_CHOOSER (dialog));
>> +        if (g_strrstr(filename, ".") == NULL) {
> Looks fine. In another way, I'd say the following diff might fix
> some other corner cases too.
>
> -        if (g_strrstr(filename, ".") == NULL) {
> +        if (!g_str_has_suffix(filename, ".png")) {

Well, we don't want to enforce the .png extension. We want to add the .png extension if none is provided. But .jpg, .svg, etc, are valid extensions, as long as they're supported by gdk_pixbuf.

So we have to check that there is an extension, any of them, not just .png.

Looking for the first '.' starting from the end of the string is actually how we check the file type in get_image_format() later on. I didn't want to make my change into this function though, as the filename is already 'const' there, and I thought get_image_format is more generic, and should not be used to enforce the format.



>
>
>> +            // no extension provided: add the .png default
>> +            char *tmp_filename ;
>> +            tmp_filename = g_strdup_printf("%s.png", filename) ;
>> +            g_free(filename) ;
>> +            filename = tmp_filename ;
>> +        }
>> +
>>          if (!virt_viewer_window_save_screenshot(self, filename, &error)) {
>>              virt_viewer_app_simple_message_dialog(self->priv->app,
>>                                                    error->message);
>> -- 
>> 2.21.0