Jan Horak [:jhorak] Assignee
	Description • 9 years ago

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.

	Jan Horak [:jhorak] Assignee
	Comment 1 • 9 years ago

Please have a look.

	Karl Tomlinson (:karlt)
	Comment 2 • 9 years ago

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.

	Jan Horak [:jhorak] Assignee
	Comment 3 • 9 years ago

(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.

	Jan Horak [:jhorak] Assignee
	Comment 4 • 9 years ago

Attaching async version of patch.

	Karl Tomlinson (:karlt)
	Comment 5 • 9 years ago

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;

	Jan Horak [:jhorak] Assignee
	Comment 6 • 9 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 7 • 9 years ago

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.

	Jan Horak [:jhorak] Assignee
	Comment 8 • 9 years ago

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

	Karl Tomlinson (:karlt)
	Comment 9 • 9 years ago

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.

	Jan Horak [:jhorak] Assignee
	Comment 10 • 9 years ago

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!

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e68096bfbf39

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e68096bfbf39

	Pulsebot
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/317521c81848

	Lee Salzman [:lsalzman]
	Comment 14 • 9 years ago

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?

	Lee Salzman [:lsalzman]
	Comment 15 • 9 years ago

(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...

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/317521c81848

	Jan Horak [:jhorak] Assignee
	Comment 17 • 9 years ago

(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?

	ewal
	Comment 19 • 8 years ago

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 ?