GNOME Bugzilla – Bug 781831
places-menu: Uses blocking Gio.AppInfo.launch_default_for_uri for opening network shares
Last modified: 2017-04-28 13:39:33 UTC
When opening bookmarks on network shares than the call to Gio.AppInfo.launch_default_for_uri may block indefinitely. The async API should be used instead to prevent the whole shell from locking up pending on an unresponsive network share. This is similar to bug #781765.
Created attachment 350620 [details] [review] make launching asynchronous Not as straight forward as bug #781765 because of all the nesting that is going on. I also used the "self = this" trick instead of having nested Lang.bind. Hope that is ok.
Created attachment 350621 [details] [review] report errors for mounting of volumes Technically not part of the bug, but I noticed we don't report any errors when mounting fails,.
Review of attachment 350620 [details] [review]: ::: extensions/places-menu/placeDisplay.js @@ +45,3 @@ }, + _launch_async_handler: function (launchContext, try_mount) { Style nit: camelCase. I'm not very happy with the launchAsyncHandler name either, as the function is not a handler, but returns one. _createLaunchCallback() maybe? @@ +47,3 @@ + _launch_async_handler: function (launchContext, try_mount) { + let self = this; + return function(_ignored, result) { Did you try arrow notation instead of the "self = this" hack? return (ignored, result) => { ... }
Review of attachment 350621 [details] [review]: Sure
Created attachment 350637 [details] [review] make launching asynchronous Did not know about the arrow notation - I am not much of a JS guru (yet?). Happy to report that it works. ;)
Created attachment 350638 [details] [review] report errors for mounting of volumes Update to use arrow notation.
NB: g_app_info_launch_default_for_uri_async requires glib 2.50.
Created attachment 350639 [details] [review] make launching asynchronous Also update the name, doh! Completely agree that _createLaunchCallback() is reflecting more what is going on.
Review of attachment 350639 [details] [review]: ::: extensions/places-menu/placeDisplay.js @@ +45,3 @@ }, + _createLaunchCallback: function(launchContext, try_mount) { Style nit: camelCase (sorry, should have mentioned that the comment in the last review referred to both the method name and the try_mount parameter)
Review of attachment 350638 [details] [review]: Still ACN
Created attachment 350640 [details] [review] make launching asynchronous Sorry, mea culpa, I should have seen that. Hope it is ok now.
(In reply to Christian Kellner from comment #7) > NB: g_app_info_launch_default_for_uri_async requires glib 2.50. Yeah, I saw that - the build system glue that generates the extension metadata marks extensions as compatible only with the matching gnome-shell version. So users on older gnome versions won't see the update, and assuming the GIO version shipped with 3.22 for 3.25.x users looks perfectly reasonable to me.
Review of attachment 350640 [details] [review]: LGTM
Committed to master, thanks for you patience. ;)