GNOME Bugzilla – Bug 740978
Blacklist "open in archive manager" from menus
Last modified: 2015-03-25 13:50:58 UTC
This is a useless thing to do with epub and CBZ/CBR files.
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.