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

[Victor Toso]

Hi,

On Tue, Feb 18, 2020 at 05:36:29PM +0100, Julien ROPE wrote:
> 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?

No, most likely I'm overcomplicating it. It is fine to fix the
filename before checking the formats and error out if a bad
format is given indeed.

Merged:

    https://pagure.io/virt-viewer/c/e4bacb8fde16cd21b8b8f095be720ad1a6c2d0e5?branch=master

Cheers,

> 
> 
> 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
> 
-------------- 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/20200219/85d17fee/attachment.sig>