Re: [virt-tools-list] [virt-viewer v3] spice: do not show error on cancel/close of auth dialog
On Fri, Jun 02, 2017 at 11:59:04AM -0300, Eduardo Lima (Etrunko) wrote:
> On 02/06/17 09:57, Pavel Grunt wrote:
> > On Fri, 2017-06-02 at 14:05 +0200, Victor Toso wrote:
> >> From: Victor Toso
> >>
> >> Mainly an issue for kiosk mode.
> >>
> >> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1446161
> >> Signed-off-by: Victor Toso
> >> ---
> >> src/virt-viewer-session-spice.c | 4
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-
> >> session-spice.c
> >> index 5f326aa..106abd1 100644
> >> --- a/src/virt-viewer-session-spice.c
> >> +++ b/src/virt-viewer-session-spice.c
> >> @@ -725,6 +725,10 @@
> >> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel,
> >> g_free(host);
> >> if (!ret) {
> >> g_signal_emit_by_name(session, "session-cancelled");
> >> +/* ret is false when dialog did not return
> >> GTK_RESPONSE_OK. We
> >> + * should ignore auth error dialog if user has
> >> cancelled or closed
> >> + * the dialog */
> >> +self->priv->pass_try = 0;
> >
> > I'd first change the value and after that emit the signal.
> >
> > Acked-by: Pavel Grunt
> >
>
> The patch per se is 'ok-ish' for me, but I don't really understand why
> would we not want to show the message dialog before the authentication
> dialog again? I think this could be explained with more details on the
> commit message.
Sure, let me know what you think about:
"
Mainly an issue for kiosk mode due the fact that it'll not quit the
application on cancel. That means that authentication dialog can't
really be canceled and showing an input error such as "wrong password"
is misleading (no password should be taken in consideration on Cancel).
"
> Other than that, I also think the case described in the bug is trying a
> little bit too hard to be considered a valid use case.
Its still valid IMHO. Canceling a dialog should not pop-up and error
about user's input.
Cheers,
toso
signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer v3] spice: do not show error on cancel/close of auth dialog
Hi,
On Mon, Jun 05, 2017 at 10:35:30AM -0300, Eduardo Lima (Etrunko) wrote:
> On 05/06/17 09:12, Victor Toso wrote:
> > On Fri, Jun 02, 2017 at 11:59:04AM -0300, Eduardo Lima (Etrunko) wrote:
> >> On 02/06/17 09:57, Pavel Grunt wrote:
> >>> On Fri, 2017-06-02 at 14:05 +0200, Victor Toso wrote:
> From: Victor Toso
>
> Mainly an issue for kiosk mode.
>
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1446161
> Signed-off-by: Victor Toso
> ---
> src/virt-viewer-session-spice.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-
> session-spice.c
> index 5f326aa..106abd1 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -725,6 +725,10 @@
> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel,
> g_free(host);
> if (!ret) {
> g_signal_emit_by_name(session, "session-cancelled");
> +/* ret is false when dialog did not return
> GTK_RESPONSE_OK. We
> + * should ignore auth error dialog if user has
> cancelled or closed
> + * the dialog */
> +self->priv->pass_try = 0;
> >>>
> >>> I'd first change the value and after that emit the signal.
> >>>
> >>> Acked-by: Pavel Grunt
> >>>
> >>
> >> The patch per se is 'ok-ish' for me, but I don't really understand why
> >> would we not want to show the message dialog before the authentication
> >> dialog again? I think this could be explained with more details on the
> >> commit message.
> >
> > Sure, let me know what you think about:
> >
> > "
> > Mainly an issue for kiosk mode due the fact that it'll not quit the
> > application on cancel. That means that authentication dialog can't
> > really be canceled and showing an input error such as "wrong password"
> > is misleading (no password should be taken in consideration on Cancel).
> > "
> >
>
> Yeah, much better :)
>
> Acked-by: Eduardo Lima (Etrunko)
>
> >> Other than that, I also think the case described in the bug is trying a
> >> little bit too hard to be considered a valid use case.
> >
> > Its still valid IMHO. Canceling a dialog should not pop-up and error
> > about user's input.
>
> Well, I don't really see as an error, but more informative, but maybe
> just me. As for the bug report, I was talking about the case of shutting
> down and bringing the vm up back again while the dialog is running.
Ah, okay :)
Pushed as 6480e52f62bc7c87bbdca1aa50f7ff08b09e9845
>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etru...@redhat.com
signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer v3] spice: do not show error on cancel/close of auth dialog
On 05/06/17 09:12, Victor Toso wrote:
> On Fri, Jun 02, 2017 at 11:59:04AM -0300, Eduardo Lima (Etrunko) wrote:
>> On 02/06/17 09:57, Pavel Grunt wrote:
>>> On Fri, 2017-06-02 at 14:05 +0200, Victor Toso wrote:
From: Victor Toso
Mainly an issue for kiosk mode.
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1446161
Signed-off-by: Victor Toso
---
src/virt-viewer-session-spice.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-
session-spice.c
index 5f326aa..106abd1 100644
--- a/src/virt-viewer-session-spice.c
+++ b/src/virt-viewer-session-spice.c
@@ -725,6 +725,10 @@
virt_viewer_session_spice_main_channel_event(SpiceChannel *channel,
g_free(host);
if (!ret) {
g_signal_emit_by_name(session, "session-cancelled");
+/* ret is false when dialog did not return
GTK_RESPONSE_OK. We
+ * should ignore auth error dialog if user has
cancelled or closed
+ * the dialog */
+self->priv->pass_try = 0;
>>>
>>> I'd first change the value and after that emit the signal.
>>>
>>> Acked-by: Pavel Grunt
>>>
>>
>> The patch per se is 'ok-ish' for me, but I don't really understand why
>> would we not want to show the message dialog before the authentication
>> dialog again? I think this could be explained with more details on the
>> commit message.
>
> Sure, let me know what you think about:
>
> "
> Mainly an issue for kiosk mode due the fact that it'll not quit the
> application on cancel. That means that authentication dialog can't
> really be canceled and showing an input error such as "wrong password"
> is misleading (no password should be taken in consideration on Cancel).
> "
>
Yeah, much better :)
Acked-by: Eduardo Lima (Etrunko)
>> Other than that, I also think the case described in the bug is trying a
>> little bit too hard to be considered a valid use case.
>
> Its still valid IMHO. Canceling a dialog should not pop-up and error
> about user's input.
Well, I don't really see as an error, but more informative, but maybe
just me. As for the bug report, I was talking about the case of shutting
down and bringing the vm up back again while the dialog is running.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer v3] spice: do not show error on cancel/close of auth dialog
On 02/06/17 09:57, Pavel Grunt wrote:
> On Fri, 2017-06-02 at 14:05 +0200, Victor Toso wrote:
>> From: Victor Toso
>>
>> Mainly an issue for kiosk mode.
>>
>> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1446161
>> Signed-off-by: Victor Toso
>> ---
>> src/virt-viewer-session-spice.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-
>> session-spice.c
>> index 5f326aa..106abd1 100644
>> --- a/src/virt-viewer-session-spice.c
>> +++ b/src/virt-viewer-session-spice.c
>> @@ -725,6 +725,10 @@
>> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel,
>> g_free(host);
>> if (!ret) {
>> g_signal_emit_by_name(session, "session-cancelled");
>> +/* ret is false when dialog did not return
>> GTK_RESPONSE_OK. We
>> + * should ignore auth error dialog if user has
>> cancelled or closed
>> + * the dialog */
>> +self->priv->pass_try = 0;
>
> I'd first change the value and after that emit the signal.
>
> Acked-by: Pavel Grunt
>
The patch per se is 'ok-ish' for me, but I don't really understand why
would we not want to show the message dialog before the authentication
dialog again? I think this could be explained with more details on the
commit message.
Other than that, I also think the case described in the bug is trying a
little bit too hard to be considered a valid use case.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer v3] spice: do not show error on cancel/close of auth dialog
On Fri, 2017-06-02 at 14:05 +0200, Victor Toso wrote:
> From: Victor Toso
>
> Mainly an issue for kiosk mode.
>
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1446161
> Signed-off-by: Victor Toso
> ---
> src/virt-viewer-session-spice.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-
> session-spice.c
> index 5f326aa..106abd1 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -725,6 +725,10 @@
> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel,
> g_free(host);
> if (!ret) {
> g_signal_emit_by_name(session, "session-cancelled");
> +/* ret is false when dialog did not return
> GTK_RESPONSE_OK. We
> + * should ignore auth error dialog if user has
> cancelled or closed
> + * the dialog */
> +self->priv->pass_try = 0;
I'd first change the value and after that emit the signal.
Acked-by: Pavel Grunt
Thanks,
Pavel Grunt
> } else {
> gboolean openfd;
>
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
