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

[Victor Toso]

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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20200218/25849314/attachment.sig>