GNOME Bugzilla – Bug 695111
Don't pass GError::message directly to got_error
Last modified: 2016-03-31 13:22:07 UTC
The attached patch makes some errors we show to the user more friendly. It also adds 2 new translatable string which we'll need to announce. Because of these strings it's better if we get it in today.
Created attachment 237945 [details] [review] Don't pass GError::message directly to got_error The error message stored in a GError generally comes from underlying libraries, and is not good enough to be shown to the user most of the time. This commit replaces uses of got_error with GError::message with more generic messages complemented by a debug() with the actual error message. I noticed this after getting a "Error connecting XXX: no free USB channel" in the UI when trying to redirect an USB device.
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); 'Error inserting CD' sounds like bad english to me. I think we want to talk to user in full sentences. Thats just my opinion so you might want to consult an native english speaker on this. :) @@ +255,3 @@ } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); + debug("Error inserting CD: %s", e.message); coding-style nitpick: space before '(' @@ +269,3 @@ label.set_markup (Markup.printf_escaped ("<i>%s</i>", _("empty"))); } catch (GLib.Error e) { + machine.got_error (_("Error ejecting CD")); Above comments also apply to these two lines. ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); same here. In here, we should at least specify what we were trying to connect to. @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); + debug("Error connecting %s: %s", dev.get_description ("%1$s %2$s"), err.message); how come you *only* forget space after 'debug'? :) @@ +52,2 @@ manager.device_error.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); same comments about these two lines. @@ +307,3 @@ } catch (GLib.Error err) { dev_toggle.active = false; + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); And same here.
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); I've adapted the 'Error connecting XXX' message from spice-display.vala which was preexisting. I'm not particularly interested in the message by itself, so just tell me what you want to be used here if you want more than mechanical c&p. ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); What do you mean? This is printing the name of the device being connected. @@ +307,3 @@ } catch (GLib.Error err) { dev_toggle.active = false; + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); Note that here I'm shortening an existing message, which answers more of your comments on this file.
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); How about: "Failed to insert '%s' as CD/DVD into box '%s'.'", file_path, machine.name @@ +269,3 @@ label.set_markup (Markup.printf_escaped ("<i>%s</i>", _("empty"))); } catch (GLib.Error e) { + machine.got_error (_("Error ejecting CD")); In here: How about: "Failed to eject CD/DVD from box '%s'.'", machine.name ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); You connect something *to something*. How about: "Failed to connect '%s' to box '%s'.'", device, machine.name/display.name @@ +307,3 @@ } catch (GLib.Error err) { dev_toggle.active = false; + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); I don't see how it answers my comments. AFAICT the point of this patch is to be friendlier to user. Not showing cryptic errors is what you had i mind but I also want to speak proper language to users. In here, the same suggestion goes as above: "Failed to connect '%s' to box '%s'.'", device, machine.name/display.name
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); Which is not a full sentence either. I suspect file_path will look ugly in there. I'll see how this looks. ::: src/spice-display.vala @@ +307,3 @@ } catch (GLib.Error err) { dev_toggle.active = false; + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); This answers 'how come you *only* forget space after 'debug'? :)', this answers your 'bad english' comments with 'this was broken before'. I'm only saying this _explains_ where all of this comes from, not that this _fixes_ what you raised.
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); How is that not a full sentence? Feel free to turn it into a full sentence then. I'm not authority in English. :) ::: src/spice-display.vala @@ +307,3 @@ } catch (GLib.Error err) { dev_toggle.active = false; + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); Ah ok. We should fix bad English elsewhere too but for this patch, we just try not to add more bad English.
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); Both 'Error connecting XXX' and 'Failed to insert...' are grammatically correct nominal English sentences, the latter being more detailed than the former. I thought that by 'full sentence' you meant one with a subject and a verb. ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); No clue how to get the VM name from spice-display.vala
Review of attachment 237945 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +254,3 @@ empty = false; } catch (GLib.Error e) { + machine.got_error (_("Error inserting CD")); Well subject is still missing so yeah, its still not a full sentence. We could write 'Boxes failed to.." if we want to be really complete. ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Error connecting %s").printf (dev.get_description ("%1$s %2$s"))); config.last_seen_name ? If my assumption that this error does/can popup asynchronously, we need to find a way to get the name here, otherwise you can ignore my comment.
Created attachment 237986 [details] [review] Don't pass GError::message directly to got_error The error message stored in a GError generally comes from underlying libraries, and is not good enough to be shown to the user most of the time. This commit replaces uses of got_error with GError::message with more generic messages complemented by a debug() with the actual error message. I noticed this after getting a "Error connecting XXX: no free USB channel" in the UI when trying to redirect an USB device.
Review of attachment 237986 [details] [review]: ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Automatic redirection of USB device '%s' failed").printf (dev.get_description ("%1$s %2$s"))); So I take it that my my assumption that this error does/can popup asynchronously is wrong?
Review of attachment 237986 [details] [review]: ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Automatic redirection of USB device '%s' failed").printf (dev.get_description ("%1$s %2$s"))); No clue to be honest, I haven't really checked the code, I've tried to address all the other comments first. Even if this can be asynchronous, the error message will not be worse than what was there before. If the release happens tonight, I won't have time to do more on that front, and after the release, strings are frozen.
Review of attachment 237986 [details] [review]: ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Automatic redirection of USB device '%s' failed").printf (dev.get_description ("%1$s %2$s"))); Its true that it error would be much better than before but as you said we are entering string freeze, hence the reason me being strict about them. If you don't mind, I'll adjust this patch and push for you?
Review of attachment 237986 [details] [review]: ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Automatic redirection of USB device '%s' failed").printf (dev.get_description ("%1$s %2$s"))); Depends what you want to change ;) If you want to add the VM names there, this probably can be done by an additional commit on top of this one?
Review of attachment 237986 [details] [review]: ACK ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Automatic redirection of USB device '%s' failed").printf (dev.get_description ("%1$s %2$s"))); Yeah.
Comment on attachment 237986 [details] [review] Don't pass GError::message directly to got_error Attachment 237986 [details] pushed as 1411ba9 - Don't pass GError::message directly to got_error
Review of attachment 237986 [details] [review]: ::: src/spice-display.vala @@ +46,3 @@ var manager = UsbDeviceManager.get (session); manager.auto_connect_failed.connect ( (dev, err) => { + got_error (_("Automatic redirection of USB device '%s' failed").printf (dev.get_description ("%1$s %2$s"))); Thanks, I've pushed this patch and left the bug open.
Created attachment 238039 [details] [review] Coding style fixes to last commit (1411ba9) Coding style fixes to 1411ba9 ("Don't pass GError::message directly to got_error"). Mainly, code was exceeding 120 chars.
Created attachment 238040 [details] [review] spice-dispaly: Include name of Box in error messages
Review of attachment 238040 [details] [review]: ::: src/spice-display.vala @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { var device_description = dev.get_description ("%1$s %2$s"); + var box_name = config.last_seen_name?? "Unknown"; Thanks for looking into this! I think it would be better to just use "Automatic redirection of USB device '%s' failed" would be better than including 'unknown' in this string. A warning() when last_seen_name is unset might be useful (?)
Review of attachment 238040 [details] [review]: ::: src/spice-display.vala @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { var device_description = dev.get_description ("%1$s %2$s"); + var box_name = config.last_seen_name?? "Unknown"; Looking at the code that sets 'last_seen_name', it being null seems like an unlikely scenerio so I decided not to complicate the code here.
Review of attachment 238039 [details] [review]: Whatever, this is unreviewable in splinter, I trust you as this is just mechanical according to the changelog
Review of attachment 238040 [details] [review]: spice-display in the shortlog, I'd spell 'box' instead of 'Box'. This needs to be announced to gnome-i18n as I haven't done it. ::: src/spice-display.vala @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { var device_description = dev.get_description ("%1$s %2$s"); + var box_name = config.last_seen_name?? "Unknown"; warn_if_fail(config.last_seen_name != NULL) then? Though not sure what you mean by 'unlikely' ;)
Review of attachment 238040 [details] [review]: ::: src/spice-display.vala @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { var device_description = dev.get_description ("%1$s %2$s"); + var box_name = config.last_seen_name?? "Unknown"; AFAICT, it shouldn't happen but if it does, its not to be considered an error. I think its fine to display 'Uknown' as long as its rare. I'm trying to keep the same string here and not complicate the code unless needed.
Attachment 238039 [details] pushed as 317aff9 - Coding style fixes to last commit (1411ba9) Attachment 238040 [details] pushed as f0f3d77 - spice-display: Include name of box in error messages
Review of attachment 238040 [details] [review]: ::: src/spice-display.vala @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { var device_description = dev.get_description ("%1$s %2$s"); + var box_name = config.last_seen_name?? "Unknown"; If you missed something and this can happen more often than you thought, adding a new string is harder than removing an existing one ;)
Review of attachment 238040 [details] [review]: ::: src/spice-display.vala @@ +47,3 @@ manager.auto_connect_failed.connect ( (dev, err) => { var device_description = dev.get_description ("%1$s %2$s"); + var box_name = config.last_seen_name?? "Unknown"; If thats the case, the thing to fix would then be to ensure this scenerio becomes impossible/rare. Besides its not like boxes is going to crash or anything such if this scenerio realizes.