GNOME Bugzilla – Bug 684224
Add error handling to Machine::connect_display
Last modified: 2016-03-31 13:59:51 UTC
Currently, when the user clicks on a box to connect to it, if the connection fails for some reason, there is no feedback at all. The UI seems to be frozen on a zoomed in thumbnail. The user can go back to the collection view, but that makes the user experience weird. This patch adds a way for ::connect_display to report errors so that we can switch back to the collection view and display an error popup. May be risky for 3.5.92 though, and needs a string freeze break approval...
Created attachment 224516 [details] [review] Allow Machine::connect_display to report errors If an error occurs while we connect to a remote display, we currently have no way of reporting this to the user. When this happens, Boxes is stuck on showing a zoomed in box snapshot in the middle of the screen, and nothing else happens. This commit allows Machine::connect_display to throw exceptions, and catches them to report errors. However, this is just preparatory work as the various Machine::connect_display needs to be modified to actually report the errors they encounter.
Created attachment 224517 [details] [review] Make LibvirtMachine::start private It's only used in LibvirtMachine.
Created attachment 224518 [details] [review] Allow LibvirtMachine::start to throw exceptions It's only used in two places, once in connect_display() where we want to get an exception when something happens, and once in notify_reboot_required() where only .begin() is called, so this does not change behaviour as the exception would be reported when calling .end()
Created attachment 224519 [details] [review] Throw errors from Libvirt::update_display We want LibvirtMachine::connect_display to be able to report connection errors.
Created attachment 224520 [details] [review] Allow Display::connect_it to throw errors It's only called from Machine::connect_display overrides from where we want to be able to forward connection errors to the caller.
Created attachment 224521 [details] [review] Don't silently ignore errors in ::connect_it impl
(In reply to comment #0) > May be risky for 3.5.92 though, and needs a string freeze break approval... The break is a new string in the first patch: _("Connection to '%s' failed")
Review of attachment 224521 [details] [review]: ::: src/spice-display.vala @@ +151,3 @@ hide (display.channel_id); }); + } catch (Boxes.Error error) { rethrow_error = error; warning (error.message); } This doesn't seem right. You can't just set the rethrow_error local variable from a callback, the original call might have exited a long time ago already. @@ +166,3 @@ + session.password = password; + session.connect (); + } catch (GLib.Error error) { Same here, any error in the channel_destroy callback is not gonna be reported to the connect_it caller.
I agree that this is important, getting stuck on error isn't very robust...
Created attachment 224584 [details] [review] Allow Display::connect_it to throw errors It's only called from Machine::connect_display overrides from where we want to be able to forward connection errors to the caller.
Created attachment 224585 [details] [review] Don't silently ignore errors in ::connect_it impl
Created attachment 224586 [details] [review] SpiceDisplay::get_display cannot throw Spice::get_display() throws an exception when spice_display_new returns null. However, this function is a wrapper over g_object_new so it cannot return null. We can thus mark Spice::get_display() as not throwing, and update its callers accordingly.
Review of attachment 224521 [details] [review]: ::: src/spice-display.vala @@ +151,3 @@ hide (display.channel_id); }); + } catch (Boxes.Error error) { rethrow_error = error; warning (error.message); } Oops, thanks for the catch, dunno what I was thinking ;) On closer look, it appears that get_display can never throw, so it's possible to clean up all of this, see updated patches @@ +166,3 @@ + session.password = password; + session.connect (); + } catch (GLib.Error error) { Yeah, this is useless anyway as vala tells me that nothing can throw in this code block, I've killed it.
Review of attachment 224518 [details] [review]: ::: src/libvirt-machine.vala @@ -573,3 @@ - } - } catch (GLib.Error error) { - warning ("Failed to start '%s': %s", domain.get_name (), error.message); If you're not calling .end in the other case, we lose this warning though?
Created attachment 224588 [details] [review] Allow Machine::connect_display to report errors If an error occurs while we connect to a remote display, we currently have no way of reporting this to the user. When this happens, Boxes is stuck on showing a zoomed in box snapshot in the middle of the screen, and nothing else happens. This commit allows Machine::connect_display to throw exceptions, and catches them to report errors. However, this is just preparatory work as the various Machine::connect_display needs to be modified to actually report the errors they encounter.
Created attachment 224589 [details] [review] Make LibvirtMachine::start private It's only used in LibvirtMachine.
Created attachment 224590 [details] [review] Allow LibvirtMachine::start to throw exceptions It's only used in two places, once in connect_display() where we want to get an exception when something happens, and once in notify_reboot_required() where only .begin() is called, so this does not change behaviour as the exception would be reported when calling .end()
Created attachment 224591 [details] [review] Throw errors from Libvirt::update_display We want LibvirtMachine::connect_display to be able to report connection errors.
Created attachment 224592 [details] [review] Allow Display::connect_it to throw errors It's only called from Machine::connect_display overrides from where we want to be able to forward connection errors to the caller.
Created attachment 224593 [details] [review] Don't silently ignore errors in ::connect_it impl
Created attachment 224594 [details] [review] SpiceDisplay::get_display cannot throw Spice::get_display() throws an exception when spice_display_new returns null. However, this function is a wrapper over g_object_new so it cannot return null. We can thus mark Spice::get_display() as not throwing, and update its callers accordingly.
Created attachment 224595 [details] [review] Add error popup on box connection errors This commit adds a new string and we are in string freeze break, so approval is needed to get this change in. However, the patch series is still a good improvement even without the added string, so I'm splitting this out to be able to commit the rest of the series (thanks Alex for the suggestion!). However, without this error message, Boxes starting to connect to a box and then giving up can be quite confusing, so adding this message is a worthwhile user-friendliness improvement.
Here is a rediffed series with the string breaking the freeze split in a separate commit, and with the warning readded in commit "Allow LibvirtMachine::start to throw exceptions"
Review of attachment 224588 [details] [review]: ok
Review of attachment 224589 [details] [review]: ok
Review of attachment 224590 [details] [review]: ok
Review of attachment 224591 [details] [review]: ok
Review of attachment 224592 [details] [review]: ok
Review of attachment 224593 [details] [review]: ok
Review of attachment 224594 [details] [review]: ok
Review of attachment 224595 [details] [review]: ::: src/app.vala @@ +695,1 @@ warning ("connect display failed: %s", e.message); Should also remove warning?
Review of attachment 224595 [details] [review]: ::: src/app.vala @@ +695,1 @@ warning ("connect display failed: %s", e.message); I'll move it to debug(), error.message can be useful while debugging.
Created attachment 224605 [details] [review] Add error popup on box connection errors This commit adds a new string and we are in string freeze break, so approval is needed to get this change in. However, the patch series is still a good improvement even without the added string, so I'm splitting this out to be able to commit the rest of the series (thanks Alex for the suggestion!). However, without this error message, Boxes starting to connect to a box and then giving up can be quite confusing, so adding this message is a worthwhile user-friendliness improvement.
Attachment 224588 [details] pushed as 8745a86 - Allow Machine::connect_display to report errors Attachment 224589 [details] pushed as 07e4d67 - Make LibvirtMachine::start private Attachment 224590 [details] pushed as 7aeeb81 - Allow LibvirtMachine::start to throw exceptions Attachment 224591 [details] pushed as 5aa069b - Throw errors from Libvirt::update_display Attachment 224592 [details] pushed as 49cbb6f - Allow Display::connect_it to throw errors Attachment 224593 [details] pushed as 7843467 - Don't silently ignore errors in ::connect_it impl Attachment 224594 [details] pushed as 313c482 - SpiceDisplay::get_display cannot throw
Comment on attachment 224605 [details] [review] Add error popup on box connection errors Got 2 ACKs from gnome-i18n, patch pushed before 3.5.92