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

[Julien ROPE]

Hi,



[...]
> But get_image_format() is just a helper for
> virt_viewer_window_save_screenshot() and there we handle the case
> where if the file format is not recognized, we error out.
> 
> If the goal is to not error out anymore, we will need to handle
> that code anyway as with this patch, it'll become dead code,
> correct?


I don't think so. The code you mention will stay, and continue to error out when a non-supported image format is given. We just want to avoid the (annoying) error when you just forget to say what file format you want, which can happen very easily as there is no default provided in the file selection dialog.

So we're just adding the .png extension when none is provided, but we continue to check the file format after that, in case the user requested something the system doesn't support.

That's what I get from comment #3 from Daniel.


I may be wrong here. It seemed like a very simple change to me, but maybe it's actually too simple?


Regards,

Julien


Le 18/02/2020 à 10:01, Victor Toso a écrit :
> Hi,
>
> On Fri, Jan 24, 2020 at 09:51:03AM +0100, Julien ROPE wrote:
>> 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.
> Right, I could have been more clear on checking supported formats
>
>> So we have to check that there is an extension, any of them,
>> not just .png.
> Yep
>
>> 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.
> But get_image_format() is just a helper for
> virt_viewer_window_save_screenshot() and there we handle the case
> where if the file format is not recognized, we error out.
>
> If the goal is to not error out anymore, we will need to handle
> that code anyway as with this patch, it'll become dead code,
> correct?
>
> cheers,
>
>
>>>
>>>> +            // 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