Re: [virt-tools-list] [PATCH] remote-viewer: add a default extension to screenshot filenames
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
>
> 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é
> ---
> 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
>
signature.asc
Description: PGP signature
Re: [virt-tools-list] [PATCH] remote-viewer: add a default extension to screenshot filenames
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
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é
---
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
Re: [virt-tools-list] [PATCH] remote-viewer: add a default extension to screenshot filenames
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
> >>
> >> 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é
> >> ---
> >> 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
>
signature.asc
Description: PGP signature
Re: [virt-tools-list] [PATCH] remote-viewer: add a default extension to screenshot filenames
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
>>
>> 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é
>> ---
>> 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
Re: [virt-tools-list] [PATCH] remote-viewer: add a default extension to screenshot filenames
Hi,
On Fri, Dec 13, 2019 at 11:16:23AM +0100, Julien Ropé wrote:
> From: Julien ROPE
>
> 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é
> ---
> 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")) {
> +// 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
>
> ___
> virt-tools-list mailing list
> virt-tools-list@redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
signature.asc
Description: PGP signature
