GTK 3 offers application chooser dialog - GtkAppChooserDialog, see https://developer.gnome.org/gtk3/stable/GtkAppChooserDialog.html It would be nice to allow to use this dialog instead of filepicker when opening a file.

Please have a look.

Comment on attachment 8559732 [details] [diff] [review] GtkAppChooserDialog wrapper v1 It should be reasonable to assume that a "@mozilla.org/applicationchooser;1" can be created in GTK3 builds, so no need to have the filepicker fallback there. We're trying to avoid API that pretends to make async actions synchronous by running nested event loops. This causes problems because callers don't expect state to change while they are not watching, and nested events loops unroll only in FILO order. So I'd prefer to have an async API on nsIApplicationChooser like in nsIColorPicker or open() in nsIFilePicker, and it looks like nsHelperAppDlg.js could me modified to use this reasonably easily. I think it would make sense to create the nsIHandlerApp in the nsApplicationChooser, rather than providing chooser API for info about the app and creating the nsIHandlerApp in nsHelperAppDlg. nsIHandlerApp would correspond more closely than an nsIFile to what the nsIApplicationChooser finds. updateTypeInfo() in helperApp.js uses the name of the preferredApplicationHandler, and this could support doing something better than getFileDisplayName() in nsHelperAppDlg.js in the future. chooseApp() in nsHelperAppDlg.js and nsMIMEInfoBase::LaunchWithFile() seem to expect an nsILocalHandlerApp, but perhaps in the future this can be a nsGIOMimeApp implementing nsIHandlerApp.

(In reply to Karl Tomlinson (:karlt) from comment #2) > Comment on attachment 8559732 [details] [diff] [review] > GtkAppChooserDialog wrapper v1 > > It should be reasonable to assume that a "@mozilla.org/applicationchooser;1" > can be created in GTK3 builds, so no need to have the filepicker fallback > there. > > We're trying to avoid API that pretends to make async actions synchronous by > running nested event loops. This causes problems because callers don't > expect > state to change while they are not watching, and nested events loops unroll > only in FILO order. So I'd prefer to have an async API on > nsIApplicationChooser like in nsIColorPicker or open() in nsIFilePicker, and > it looks like nsHelperAppDlg.js could me modified to use this reasonably > easily. > > I think it would make sense to create the nsIHandlerApp in the > nsApplicationChooser, rather than providing chooser API for info about the > app > and creating the nsIHandlerApp in nsHelperAppDlg. nsIHandlerApp would > correspond more closely than an nsIFile to what the nsIApplicationChooser > finds. updateTypeInfo() in helperApp.js uses the name of the > preferredApplicationHandler, and this could support doing something > better than getFileDisplayName() in nsHelperAppDlg.js in the future. > chooseApp() in nsHelperAppDlg.js and nsMIMEInfoBase::LaunchWithFile() seem to > expect an nsILocalHandlerApp, but perhaps in the future this can be a > nsGIOMimeApp implementing nsIHandlerApp. Thanks for the feedback. The fallback code is for GTK2 builds. I'll look into the async code and attach patch ready for review later.

Attaching async version of patch.

Comment on attachment 8576618 [details] [diff] [review] apppicker async v1 This is the approach I had in mind, thank you very much, but there are a few details to touch up and the patch is missing nsApplicationChooser.h. (In reply to Jan Horak from comment #3) > The fallback code is for GTK2 builds. Can the preprocessor conditional be changed from XP_LINUX to test the GTK version? That way GTK2 can use the fallback path and there is no need to have a duplicate nsIFilePicker block. >+ finishAppChoose: function() { I would call this finishChooseApp for consistency with chooseApp. >+ if (this.chosenApp.name) { >+ otherHandler.label = this.chosenApp.name; >+ } else { >+ otherHandler.label = this.getFileDisplayName(this.chosenApp.executable); >+ } The name getters on the nsLocalHandlerApp classes will return a non-empty string if the executable property is set. It looks like that should be fine for nsLocalHandlerAppMac, but nsMIMEInfoWin::GetLocalHandlerApp() [1] doesn't do anything special and so will use to GetLeafName() [2]. [1] http://hg.mozilla.org/mozilla-central/annotate/cbd0efcd976c/uriloader/exthandler/win/nsMIMEInfoWin.cpp#l279 [2] https://hg.mozilla.org/mozilla-central/annotate/cbd0efcd976c/uriloader/exthandler/nsLocalHandlerApp.cpp#l18 I think an "#ifdef XP_WIN" is required. >+nsApplicationChooser::Init(nsIDOMWindow* aParent, const nsACString & title) Convention is "const nsACString& aTitle". Similarly for contentType below. Some APIs are not bothering with the 'a' prefix, but adding the prefix would make the naming consistent with other parameters in these methods. >+ NS_ENSURE_TRUE(aParent, NS_ERROR_FAILURE); >+ nsCOMPtr<nsIWidget> widget = widget::WidgetUtils::DOMWindowToWidget(aParent); >+ NS_ENSURE_TRUE(widget, NS_ERROR_FAILURE); >+ mParentWidget = widget; Assign directly to mParent, and NS_ENSURE_TRUE on that, so there is no need for the |widget| temporary. DOMWindowToWidget() is null-safe, so only one NS_ENSURE_TRUE is really needed. >+NS_IMETHODIMP >+nsApplicationChooser::Open(const nsACString & contentType, nsIApplicationChooserFinishedCallback *aCallback) >+ MOZ_ASSERT(aCallback); >+ mCallback = aCallback; Can you add a check to return failure if mCallback is already set please? (That would mean that a chooser is already in progress.) >+ GtkWidget* chooser = gtk_app_chooser_dialog_new_for_content_type(parent_widget, >+ (GtkDialogFlags) (GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT | GTK_DIALOG_USE_HEADER_BAR), >+ contentType.BeginReading()); By breaking the line after "chooser =", it is easier to fit in 80 columns. When you need a pointer to a nul-terminated string from Gecko's string API, use get(). BeginReading() is for use with EndReading() and doesn't imply nul-termination. However, |contentType| is an nsACString, so use PromiseFlatCString(aContentType).get(). gtk_app_chooser_dialog_init() calls gtk_dialog_set_use_header_bar_from_setting() for us, so I think it is better not to pass GTK_DIALOG_USE_HEADER_BAR, but instead honour the setting. The flag also needs GTK 3.12 to build, but only 3.4 is required by configure. >+ g_signal_handlers_disconnect_by_func(chooser, FuncToGpointer(OnDestroy), this); Please keep the code comment that is in the other files: 'A "response" signal won't be sent again but "destroy" will be.' >+ GAppInfo *app_info = gtk_app_chooser_get_app_info(GTK_APP_CHOOSER(chooser)); app_info needs to be released with g_object_unref. >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Cannot create local filename."); >+ return; >+ } This function should not return early, but should still call Done() with a null handler and release this. >+ nsCString fileWithFullPath; >+ fileWithFullPath.Adopt(g_find_program_in_path(g_app_info_get_executable(app_info))); The result of g_find_program_in_path() should really be freed with g_free, which is not what nsCString will do. Instead use nsDependentCString to pass the result to NS_NewNativeLocalFile(). >+ rv = NS_NewNativeLocalFile(fileWithFullPath, false, getter_AddRefs(localExecutable)); >+ nsCOMPtr<nsIHandlerApp> handler; >+ handler = do_QueryInterface(localHandler); >+ handler->SetName(NS_ConvertUTF8toUTF16(g_app_info_get_display_name(app_info))); Is it necessary to QI to use a base class like this? I expect localHandler->SetName() should work. >+ nsCOMPtr<nsIApplicationChooser> chooser; >+ chooser = new nsApplicationChooser; Please declare and initialize in the same statement: nsCOMPtr<nsIApplicationChooser> chooser = new nsApplicationChooser;

Thanks for looking at it. Sorry for the delay, I've been busy with some other stuff. Please have a look. I've tried to address all issues you mentioned.

Comment on attachment 8615301 [details] [diff] [review] apppicker async v2 This patch is still missing nsApplicationChooser.h. >+ var nsIApplicationChooser = Components.interfaces.nsIApplicationChooser; >+ if (nsIApplicationChooser) { >+ } else { >+ return NS_ERROR_NO_INTERFACE; >+ } I don't think it makes sense to return these values from javascript functions. Perhaps throwing is an option, but I don't know. I suggest just assuming that nsIApplicationChooser exists and skipping the null check, as is currently done for nsIFilePicker. >+ GAppInfo *app_info = gtk_app_chooser_get_app_info(GTK_APP_CHOOSER(chooser)); >+ localHandler = do_CreateInstance(NS_LOCALHANDLERAPP_CONTRACTID, &rv); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Out of memory."); >+ g_object_unref(app_info); >+ return; >+ } This function should not return early, but should still call Done() with a null handler and release this. Allocate localHandler before setting app_info, so that this g_object_unref() is not required, and break instead of return. >+ /*nsCOMPtr<nsIHandlerApp> handler; >+ handler = do_QueryInterface(localHandler);*/ Remove this now.

Thanks for the review, attaching fixed patch. Try run is there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef1573951720

Comment on attachment 8615974 [details] [diff] [review] apppicker async v3 >+interface nsIApplicationChooser : nsISupports >+{ >+ /** >+ * Open application chooser dialog. >+ * >+ * @param contentType content type of file to open >+ * @param applicationChooserFinishedCallback callback fuction to run when dialog is closed >+ */ >+ void open(in ACString contentType, in nsIApplicationChooserFinishedCallback applicationChooserFinishedCallback); >+ >+ /** >+ * Initialize the application chooser picker widget. The application chooser >+ * is not valid until this method is called. >+ * >+ * @param parent nsIDOMWindow parent. This dialog will be dependent >+ * on this parent. parent must be non-null. >+ * @param title The title for the file widget >+ * >+ */ >+ void init(in nsIDOMWindow parent, in ACString title); Please declare init() first, before open() so that readers are less likely to miss it. Convention is to declare init and shutdown methods first.

Should be fine now, copying review+ from previous review by Karl. Another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa142a4f970e Thanks for the review!

This is causing elm builds to fail: https://treeherder.mozilla.org/#/jobs?repo=elm > /builds/slave/elm-lx-d-000000000000000000000/build/src/widget/gtk/nsApplicationChooser.cpp:18:2354: error: deleting object of polymorphic class type 'nsApplicationChooser' which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor] > gmake[5]: *** [Unified_cpp_widget_gtk0.o] Error 1 The try run in this case was only using gtk2 builds, so this was never tested at all on an actual gtk3 build on the try server. While it doesn't seem to affect my local builds, it does break my try builds. To do gtk3 try runs, you need to have the following patch applied when you push: https://bug1016641.bugzilla.mozilla.org/attachment.cgi?id=8579124 Jan, can you please resolve the build issue?

(In reply to Lee Salzman [:eihrul] from comment #14) > This is causing elm builds to fail: > https://treeherder.mozilla.org/#/jobs?repo=elm > > > /builds/slave/elm-lx-d-000000000000000000000/build/src/widget/gtk/nsApplicationChooser.cpp:18:2354: error: deleting object of polymorphic class type 'nsApplicationChooser' which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor] > > gmake[5]: *** [Unified_cpp_widget_gtk0.o] Error 1 > > The try run in this case was only using gtk2 builds, so this was never > tested at all on an actual gtk3 build on the try server. While it doesn't > seem to affect my local builds, it does break my try builds. > > To do gtk3 try runs, you need to have the following patch applied when you > push: https://bug1016641.bugzilla.mozilla.org/attachment.cgi?id=8579124 > > Jan, can you please resolve the build issue? Oops, this was fixed already by :glandium in c13. I missed that. Sorry. My bad. :) But still, probably better if we get in the habit of doing GTK3 try builds to root these out sooner...

(In reply to Lee Salzman [:eihrul] from comment #15) > (In reply to Lee Salzman [:eihrul] from comment #14) > > This is causing elm builds to fail: > > https://treeherder.mozilla.org/#/jobs?repo=elm > > > > > /builds/slave/elm-lx-d-000000000000000000000/build/src/widget/gtk/nsApplicationChooser.cpp:18:2354: error: deleting object of polymorphic class type 'nsApplicationChooser' which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor] > > > gmake[5]: *** [Unified_cpp_widget_gtk0.o] Error 1 > > > > The try run in this case was only using gtk2 builds, so this was never > > tested at all on an actual gtk3 build on the try server. While it doesn't > > seem to affect my local builds, it does break my try builds. > > > > To do gtk3 try runs, you need to have the following patch applied when you > > push: https://bug1016641.bugzilla.mozilla.org/attachment.cgi?id=8579124 > > > > Jan, can you please resolve the build issue? > > Oops, this was fixed already by :glandium in c13. I missed that. Sorry. My > bad. :) > > But still, probably better if we get in the habit of doing GTK3 try builds > to root these out sooner... Thaks for pointing that out. Can I use your patch for every gtk3 try run?

While a native GtkAppChooserDialog is preferable over browsing for a file manually, why does it need to maintain applicationa assocatiotions anyway? I would strongly prefer having the dialog offer 'Open' or 'Save', and when I click 'Open', delegate it to xdg-open. It's the DE's responsibility to manager file assocations, not the browsers. Any way to make a mapping '*/*' -> /usr/bin/xdg-open ?