Thanks for the comments. > On 9 Feb 2017, at 11:23, Pavel Grunt <pgrunt@xxxxxxxxxx> 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. _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list