[virt-tools-list] [virt-viewer PATCH] Show errors generated by connection dialog

[Christophe de Dinechin]

Thanks for the comments.

> On 9 Feb 2017, at 11:23, Pavel Grunt <pgrunt at redhat.com> wrote:
> 
> Hello Christophe,
> 
> On Wed, 2017-02-08 at 17:48 +0100, Christophe de Dinechin wrote:
>> When running 'remote-viewer' without arguments,
>> and selecting a non-supported protocol, e.g. ssh://foo,
>> the generated error was silently swallowed by the retry loop.
>> This was introduced in 06b2c382468876a19600374bd59475e66d488af8.
>> ---
>> src/remote-viewer.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
>> index 13c6e7c..c9ef4bb 100644
>> --- a/src/remote-viewer.c
>> +++ b/src/remote-viewer.c
>> @@ -1196,6 +1196,9 @@ cleanup:
>>    type = NULL;
>> 
>>    if (!ret && priv->open_recent_dialog) {
>> +        if (error) {
> in case of pointers we prefer an explicit comparison to NULL (just a
> style - which is not followed…)

OK

> 
> so it can be error != NULL && error->message != NULL to make everyone
> happy.

If the message is NULL, we should still display something like “unknown error”. If we think it’s worth testing for error->message != NULL, then I suggest another patch just for that, that fixes all places and not just this one.


> Although as you said the error->message is never checked...
> otoh if the message is NULL then an empty dialog would appear.
> 
>> +            virt_viewer_app_simple_message_dialog(&self->parent, 
> the first param can be app

OK.

> 
>> "%s", error->message);
> 
> the string will be displayed to the user, so it should be translated -
> in case of the empty message, it should say something.

The incoming string is already translated, I think.

BTW, I did not find where the localization strings for the project were stored. How does localization happen ? If I add a new message, who should I warn to have the message translated?

> 
> maybe "Unable to connect" error->message 

Today, the message is for example
	Unsupported graphic type ‘tap’

Is it really an improvement to have:
	Unable to connect: Unsupported graphic type ‘tap’

?

I personally don’t mind either way.