Created attachment 292803 [details] [review]
application: Use recommended app as the external viewer

This avoids ZIP-based documents from being opened with the Archive
Manager. This is especially important for ePubs and CBZ files.

Review of attachment 292803 [details] [review]:

::: src/documents.js
@@ +662,3 @@
+            apps = Gio.app_info_get_recommended_for_type (this.mimeType);
+            if (apps.length > 0)
+                defaultApp = apps[0];

Would be good to retain the app_info_supports_uris check.

Created attachment 292830 [details] [review]
application: Use recommended app as the external viewer

This avoids ZIP-based documents from being opened with the Archive
Manager. This is especially important for ePubs and CBZ files.

Review of attachment 292830 [details] [review]:

::: src/documents.js
@@ +661,3 @@
+        if (this.mimeType) {
+            let apps = Gio.app_info_get_recommended_for_type (this.mimeType);
+            if (apps.length > 0) {

The if can be dropped because the for loop below makes it redundant.

Created attachment 292832 [details] [review]
application: Use recommended app as the external viewer

This avoids ZIP-based documents from being opened with the Archive
Manager. This is especially important for ePubs and CBZ files.

Created attachment 292833 [details] [review]
application: Don't open docs without a recommended app

When a document type doesn't have a recommended application (or no
external application at all), don't try to force open it.

This prevents selected but unhandled files from being opened if one
of the selected files can be opened (eg. one ePub + one PDF should only
open the PDF file, and ePub shouldn't get opened in file-roller).

Review of attachment 292832 [details] [review]:

Perfect. Minor nitpick: the subject of the commit message should probably have "documents: " as prefix instead of "application: ".

Review of attachment 292833 [details] [review]:

Looks good to me, apart from the subject that should probably be prefixed with "selections: " instead of "application: ".

One thing that we are missing is removing the "open" entry from the gear / hamburger menu if there is no default application.

::: src/selections.js
@@ +894,3 @@
                 let doc = Application.documentManager.getItemById(urn);
+                if (doc.defaultAppName)
+                    doc.open(widget.get_screen(), Gtk.get_current_event_time());

Maybe we should move the check inside the open method? Just a thought. Not a blocker.

(In reply to comment #8)
> Review of attachment 292833 [details] [review]:
> 
> Looks good to me, apart from the subject that should probably be prefixed with
> "selections: " instead of "application: ".

Sure.

> One thing that we are missing is removing the "open" entry from the gear /
> hamburger menu if there is no default application.

I think that the code in _getPreviewMenu() (preview.js) already does that. I don't think we have any documents of that type for which we can do previews but no external apps handle them, do we?

> ::: src/selections.js
> @@ +894,3 @@
>                  let doc = Application.documentManager.getItemById(urn);
> +                if (doc.defaultAppName)
> +                    doc.open(widget.get_screen(),
> Gtk.get_current_event_time());
> 
> Maybe we should move the check inside the open method? Just a thought. Not a
> blocker.

I've done that now. And changed the prefix to documents for the commit subject.

(In reply to comment #9)
> (In reply to comment #8)
> > One thing that we are missing is removing the "open" entry from the gear /
> > hamburger menu if there is no default application.
> 
> I think that the code in _getPreviewMenu() (preview.js) already does that. I
> don't think we have any documents of that type for which we can do previews but
> no external apps handle them, do we?

Yes, ignore me. I was thinking of epubs, but then we can't even preview them, so that is OK.

One thing though, even if the button and menu entry says "Open with Document Viewer" for CBZs, it still opens in file-roller if you actually click it. We need to fix the open method.

Created attachment 293086 [details] [review]
documents: Fix opening docs with broken defaults

For local and OwnCloud documents, remember the application with which
we'll be opening the document, and which matches the label we were already
setting.

For other sources, continue using Gtk.show_uri()

Review of attachment 293086 [details] [review]:

::: src/documents.js
@@ +590,3 @@
             return;
 
+        //Without a defaultApp, launch in the web browser,

Please add a space after "//"

@@ +594,2 @@
         try {
+            if (!this.defaultApp)

Can you flip this so that the positive test comes first?

@@ +940,1 @@
             this.defaultAppName = defaultApp.get_name();

Should be this.defaultApp here too

Created attachment 293446 [details] [review]
documents: Fix opening docs with broken defaults

For local and OwnCloud documents, remember the application with which
we'll be opening the document, and which matches the label we were already
setting.

For other sources, continue using Gtk.show_uri()

Review of attachment 293446 [details] [review]:

Looks good to me. Thanks, Bastien.

Attachment 293446 [details] pushed as f44fbfa - documents: Fix opening docs with broken defaults

Reassigning to new default assignee.