From 647463cc616c0d817a133ea6fa8388d49f5d7272 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Wed, 7 Mar 2018 04:20:26 -0500
Subject: [PATCH] Reorganize file prefs and add UI for choosing PDF handler

- Add a File Handling section to the General pane of the preferences,
  and move several prefs from Miscellaneous to there
- Add a UI to that section for choosing the PDF handler
- Update Zotero.launchFileWithApplication() to use /usr/bin/open on
  macOS if the handler is an .app (which it will be if set through the
  chooser, since it's limited to Mac applications, though an executable
  can still be set via the hidden pref if desired for some reason)
- Move prefs for style/translator updates and translator error reporting
  (which isn't particularly relevant anymore in the client) to Advanced

By default the PDF handler chooser says "System Default", which isn't
the nicest thing, and there's probably a way to get/guess the system
default, since Firefox seems to know it (at least on macOS), but I
couldn't quickly find a way to do it.

Addresses #1450
---
 .../preferences/preferences_advanced.js       | 22 +++++
 .../preferences/preferences_advanced.xul      | 12 +++
 .../zotero/preferences/preferences_general.js | 95 ++++++++++++++++---
 .../preferences/preferences_general.xul       | 33 ++++---
 chrome/content/zotero/xpcom/zotero.js         | 12 ++-
 chrome/locale/en-US/zotero/preferences.dtd    | 13 ++-
 chrome/locale/en-US/zotero/zotero.properties  |  2 +
 7 files changed, 158 insertions(+), 31 deletions(-)

diff --git a/chrome/content/zotero/preferences/preferences_advanced.js b/chrome/content/zotero/preferences/preferences_advanced.js
index e6e6987f61..210f449e3b 100644
--- a/chrome/content/zotero/preferences/preferences_advanced.js
+++ b/chrome/content/zotero/preferences/preferences_advanced.js
@@ -42,6 +42,28 @@ Zotero_Preferences.Advanced = {
 	},
 	
 	
+	updateTranslators: Zotero.Promise.coroutine(function* () {
+		var updated = yield Zotero.Schema.updateFromRepository(Zotero.Schema.REPO_UPDATE_MANUAL);
+		var button = document.getElementById('updateButton');
+		if (button) {
+			if (updated===-1) {
+				var label = Zotero.getString('zotero.preferences.update.upToDate');
+			}
+			else if (updated) {
+				var label = Zotero.getString('zotero.preferences.update.updated');
+			}
+			else {
+				var label = Zotero.getString('zotero.preferences.update.error');
+			}
+			button.setAttribute('label', label);
+			
+			if (updated && Zotero_Preferences.Cite) {
+				yield Zotero_Preferences.Cite.refreshStylesList();
+			}
+		}
+	}),
+	
+	
 	migrateDataDirectory: Zotero.Promise.coroutine(function* () {
 		var currentDir = Zotero.DataDirectory.dir;
 		var defaultDir = Zotero.DataDirectory.defaultDir;
diff --git a/chrome/content/zotero/preferences/preferences_advanced.xul b/chrome/content/zotero/preferences/preferences_advanced.xul
index b811225b38..05ff72d9d8 100644
--- a/chrome/content/zotero/preferences/preferences_advanced.xul
+++ b/chrome/content/zotero/preferences/preferences_advanced.xul
@@ -37,12 +37,16 @@
 			onpaneload="Zotero_Preferences.Advanced.init()"
 			helpTopic="advanced">
 		<preferences>
+			<preference id="pref-automaticScraperUpdates" name="extensions.zotero.automaticScraperUpdates" type="bool"/>
+			<preference id="pref-reportTranslationFailure" name="extensions.zotero.reportTranslationFailure" type="bool"/>
+			
 			<preference id="pref-baseAttachmentPath" name="extensions.zotero.baseAttachmentPath" type="string"/>
 			<preference id="pref-useDataDir" name="extensions.zotero.useDataDir" type="bool"/>
 			<preference id="pref-dataDir" name="extensions.zotero.dataDir" type="string"/>
 			<preference id="pref-debug-output-enableAfterRestart" name="extensions.zotero.debug.store" type="bool"/>
 			<preference id="pref-openURL-resolver" name="extensions.zotero.openURL.resolver" type="string"/>
 			<preference id="pref-openURL-version" name="extensions.zotero.openURL.version" type="string"/>
+			
 			<preference id="pref-keys-openZotero" name="extensions.zotero.keys.openZotero" type="string"/>
 			<preference id="pref-keys-saveToZotero" name="extensions.zotero.keys.saveToZotero" type="string"/>
 			<preference id="pref-keys-library" name="extensions.zotero.keys.library" type="string"/>
@@ -74,6 +78,14 @@
 					<groupbox id="zotero-prefpane-advanced-miscellaneous">
 						<caption label="&zotero.preferences.miscellaneous;"/>
 						
+						<hbox align="center">
+							<checkbox label="&zotero.preferences.autoUpdate;" preference="pref-automaticScraperUpdates"/>
+							<button id="updateButton" style="margin-top:0" label="&zotero.preferences.updateNow;"
+								oncommand="Zotero_Preferences.Advanced.updateTranslators()"/>
+						</hbox>
+						
+						<checkbox label="&zotero.preferences.reportTranslationFailure;" preference="pref-reportTranslationFailure"/>
+						
 						<hbox align="center">
 							<label value="&zotero.bibliography.locale.label;"/>
 							<menulist id="locale-menu"
diff --git a/chrome/content/zotero/preferences/preferences_general.js b/chrome/content/zotero/preferences/preferences_general.js
index 2cbb081a6d..668fb3b721 100644
--- a/chrome/content/zotero/preferences/preferences_general.js
+++ b/chrome/content/zotero/preferences/preferences_general.js
@@ -25,6 +25,9 @@
 
 "use strict";
 
+Components.utils.import("resource://gre/modules/Services.jsm");
+Components.utils.import("resource://gre/modules/osfile.jsm");
+
 Zotero_Preferences.General = {
 	init: function () {
 		// JS-based strings
@@ -36,27 +39,89 @@ Zotero_Preferences.General = {
 		}
 		
 		document.getElementById('noteFontSize').value = Zotero.Prefs.get('note.fontSize');
+		
+		this._updateFileHandlerUI();
 	},
 	
+	//
+	// File handlers
+	//
+	chooseFileHandler: function (type) {
+		var pref = this._getFileHandlerPref(type);
+		var currentPath = Zotero.Prefs.get(pref);
+		
+		var nsIFilePicker = Components.interfaces.nsIFilePicker;
+		var fp = Components.classes["@mozilla.org/filepicker;1"]
+			.createInstance(nsIFilePicker);
+		if (currentPath) {
+			fp.displayDirectory = Zotero.File.pathToFile(OS.Path.dirname(currentPath));
+		}
+		fp.init(
+			window,
+			Zotero.getString('zotero.preferences.chooseFileHandler'),
+			nsIFilePicker.modeOpen
+		);
+		fp.appendFilters(nsIFilePicker.filterApps);
+		if (fp.show() != nsIFilePicker.returnOK) {
+			this._updateFileHandlerUI();
+			return false;
+		}
+		var newPath = OS.Path.normalize(fp.file.path);
+		this.setFileHandler(type, newPath);
+	},
 	
-	updateTranslators: Zotero.Promise.coroutine(function* () {
-		var updated = yield Zotero.Schema.updateFromRepository(Zotero.Schema.REPO_UPDATE_MANUAL);
-		var button = document.getElementById('updateButton');
-		if (button) {
-			if (updated===-1) {
-				var label = Zotero.getString('zotero.preferences.update.upToDate');
+	setFileHandler: function (type, handler) {
+		var pref = this._getFileHandlerPref(type);
+		if (handler) {
+			Zotero.Prefs.set(pref, handler);
+		}
+		else {
+			Zotero.Prefs.clear(pref);
+		}
+		this._updateFileHandlerUI();
+	},
+	
+	_updateFileHandlerUI: function () {
+		var handler = Zotero.Prefs.get('fileHandler.pdf');
+		var menulist = document.getElementById('fileHandler-pdf');
+		var customMenuItem = document.getElementById('fileHandler-custom');
+		if (handler) {
+			let icon;
+			try {
+				let fph = Services.io.getProtocolHandler("file")
+					.QueryInterface(Components.interfaces.nsIFileProtocolHandler);
+				let urlspec = fph.getURLSpecFromFile(Zotero.File.pathToFile(handler));
+				icon = "moz-icon://" + urlspec + "?size=16";
 			}
-			else if (updated) {
-				var label = Zotero.getString('zotero.preferences.update.updated');
+			catch (e) {
+				Zotero.logError(e);
+			}
+			
+			let handlerFilename = OS.Path.basename(handler);
+			if (Zotero.isMac) {
+				handlerFilename = handlerFilename.replace(/\.app$/, '');
+			}
+			customMenuItem.setAttribute('label', handlerFilename);
+			if (icon) {
+				customMenuItem.className = 'menuitem-iconic';
+				customMenuItem.setAttribute('image', icon);
 			}
 			else {
-				var label = Zotero.getString('zotero.preferences.update.error');
-			}
-			button.setAttribute('label', label);
-			
-			if (updated && Zotero_Preferences.Cite) {
-				yield Zotero_Preferences.Cite.refreshStylesList();
+				customMenuItem.className = '';
 			}
+			customMenuItem.hidden = false;
+			menulist.selectedIndex = 0;
 		}
-	})
+		else {
+			customMenuItem.hidden = true;
+			menulist.selectedIndex = 1;
+		}
+	},
+	
+	_getFileHandlerPref: function (type) {
+		if (type != 'pdf') {
+			throw new Error(`Unknown file type ${type}`);
+		}
+		return 'fileHandler.pdf';
+	}
 }
diff --git a/chrome/content/zotero/preferences/preferences_general.xul b/chrome/content/zotero/preferences/preferences_general.xul
index 1a991cb43d..86bd25e64a 100644
--- a/chrome/content/zotero/preferences/preferences_general.xul
+++ b/chrome/content/zotero/preferences/preferences_general.xul
@@ -33,12 +33,12 @@
 		<preferences id="zotero-prefpane-general-preferences">
 			<preference id="pref-fontSize" name="extensions.zotero.fontSize" type="string"/>
 			<preference id="pref-layout" name="extensions.zotero.layout" type="string"/>
-			<preference id="pref-automaticScraperUpdates" name="extensions.zotero.automaticScraperUpdates" type="bool"/>
-			<preference id="pref-reportTranslationFailure" name="extensions.zotero.reportTranslationFailure" type="bool"/>
+			
 			<preference id="pref-automaticSnapshots" name="extensions.zotero.automaticSnapshots" type="bool"/>
 			<preference id="pref-downloadAssociatedFiles" name="extensions.zotero.downloadAssociatedFiles" type="bool"/>
 			<preference id="pref-autoRecognizeFiles" name="extensions.zotero.autoRecognizeFiles" type="bool"/>
 			<preference id="pref-autoRenameFiles" name="extensions.zotero.autoRenameFiles" type="bool"/>
+			
 			<preference id="pref-automaticTags" name="extensions.zotero.automaticTags" type="bool"/>
 			<preference id="pref-trashAutoEmptyDays" name="extensions.zotero.trashAutoEmptyDays" type="int"/>
 			
@@ -110,22 +110,33 @@
 			</grid>
 		</groupbox>
 		
-		<groupbox id="zotero-prefpane-miscellaneous-groupbox">
-			<caption label="&zotero.preferences.miscellaneous;"/>
+		<groupbox id="zotero-prefpane-file-handling-groupbox">
+			<caption label="&zotero.preferences.fileHandling;"/>
 			
-			<hbox align="center">
-				<checkbox label="&zotero.preferences.autoUpdate;" preference="pref-automaticScraperUpdates"/>
-				<button id="updateButton" style="margin-top:0" label="&zotero.preferences.updateNow;"
-					oncommand="Zotero_Preferences.General.updateTranslators()"/>
-			</hbox>
-			
-			<checkbox label="&zotero.preferences.reportTranslationFailure;" preference="pref-reportTranslationFailure"/>
 			<checkbox id="automaticSnapshots-checkbox"
 				label="&zotero.preferences.automaticSnapshots;"
 				preference="pref-automaticSnapshots"/>
 			<checkbox label="&zotero.preferences.downloadAssociatedFiles;" preference="pref-downloadAssociatedFiles"/>
 			<checkbox label="&zotero.preferences.autoRecognizeFiles;" preference="pref-autoRecognizeFiles"/>
 			<checkbox label="&zotero.preferences.autoRenameFiles;" preference="pref-autoRenameFiles"/>
+			
+			<hbox align="center">
+				<label value="&zotero.preferences.fileHandler.openPDFsUsing;" control="file-handler-pdf"/>
+				<menulist id="fileHandler-pdf">
+					<menupopup>
+						<menuitem id="fileHandler-custom"/>
+						<menuitem label="&zotero.preferences.fileHandler.systemDefault;"
+								oncommand="Zotero_Preferences.General.setFileHandler('pdf', false)"/>
+						<menuitem label="&zotero.preferences.custom;"
+								oncommand="Zotero_Preferences.General.chooseFileHandler('pdf')"/>
+					</menupopup>
+				</menulist>
+			</hbox>
+		</groupbox>
+		
+		<groupbox id="zotero-prefpane-miscellaneous-groupbox">
+			<caption label="&zotero.preferences.miscellaneous;"/>
+			
 			<checkbox label="&zotero.preferences.automaticTags;" preference="pref-automaticTags"/>
 			<hbox align="center">
 				<label value="&zotero.preferences.trashAutoEmptyDaysPre;"/>
diff --git a/chrome/content/zotero/xpcom/zotero.js b/chrome/content/zotero/xpcom/zotero.js
index 2a1088e6e6..84cc45d741 100644
--- a/chrome/content/zotero/xpcom/zotero.js
+++ b/chrome/content/zotero/xpcom/zotero.js
@@ -1141,8 +1141,18 @@ Services.scriptloader.loadSubScript("resource://zotero/polyfill.js");
 			throw new Error("'" + applicationPath + "' does not exist");
 		}
 		
+		var args;
+		// On macOS, if we only have an .app, launch it using 'open'
+		if (Zotero.isMac && applicationPath.endsWith('.app')) {
+			args = [filePath, '-a', applicationPath];
+			applicationPath = '/usr/bin/open';
+		}
+		else {
+			args = [filePath];
+		}
+		
 		// Async, but we don't want to block
-		Zotero.Utilities.Internal.exec(applicationPath, [filePath]);
+		Zotero.Utilities.Internal.exec(applicationPath, args);
 	};
 	
 	
diff --git a/chrome/locale/en-US/zotero/preferences.dtd b/chrome/locale/en-US/zotero/preferences.dtd
index 8d4fbd0bd9..0b5dbd8d27 100644
--- a/chrome/locale/en-US/zotero/preferences.dtd
+++ b/chrome/locale/en-US/zotero/preferences.dtd
@@ -4,6 +4,7 @@
 <!ENTITY zotero.preferences.items						 "items">
 <!ENTITY zotero.preferences.period						 ".">
 <!ENTITY zotero.preferences.settings					"Settings">
+<!ENTITY zotero.preferences.custom                    "Custom…">
 
 <!ENTITY zotero.preferences.prefpane.general				"General">
 
@@ -18,6 +19,14 @@
 <!ENTITY zotero.preferences.fontSize.xlarge			"X-Large">
 <!ENTITY zotero.preferences.fontSize.notes				"Note font size:">
 
+<!ENTITY zotero.preferences.fileHandling				"File Handling">
+<!ENTITY zotero.preferences.automaticSnapshots			"Automatically take snapshots when creating items from web pages">
+<!ENTITY zotero.preferences.downloadAssociatedFiles		"Automatically attach associated PDFs and other files when saving items">
+<!ENTITY zotero.preferences.autoRecognizeFiles        "Automatically retrieve metadata for PDFs">
+<!ENTITY zotero.preferences.autoRenameFiles     "Automatically rename attachment files using parent metadata">
+<!ENTITY zotero.preferences.fileHandler.openPDFsUsing "Open PDFs using">
+<!ENTITY zotero.preferences.fileHandler.systemDefault "System Default">
+
 <!ENTITY zotero.preferences.miscellaneous				"Miscellaneous">
 <!ENTITY zotero.preferences.autoUpdate					"Automatically check for updated translators and styles">
 <!ENTITY zotero.preferences.updateNow					"Update now">
@@ -25,10 +34,6 @@
 <!ENTITY zotero.preferences.zoteroDotOrgVersionHeader			"Allow zotero.org to customize content based on current Zotero version">
 <!ENTITY zotero.preferences.zoteroDotOrgVersionHeader.tooltip	"If enabled, the current Zotero version will be included in HTTP requests to zotero.org.">
 <!ENTITY zotero.preferences.parseRISRefer				"Use Zotero for downloaded BibTeX/RIS/Refer files">
-<!ENTITY zotero.preferences.automaticSnapshots			"Automatically take snapshots when creating items from web pages">
-<!ENTITY zotero.preferences.downloadAssociatedFiles		"Automatically attach associated PDFs and other files when saving items">
-<!ENTITY zotero.preferences.autoRecognizeFiles        "Automatically retrieve metadata for PDFs">
-<!ENTITY zotero.preferences.autoRenameFiles     "Automatically rename attachment files using parent metadata">
 <!ENTITY zotero.preferences.automaticTags				"Automatically tag items with keywords and subject headings">
 <!ENTITY zotero.preferences.trashAutoEmptyDaysPre		"Automatically remove items in the trash deleted more than">
 <!ENTITY zotero.preferences.trashAutoEmptyDaysPost		"days ago">
diff --git a/chrome/locale/en-US/zotero/zotero.properties b/chrome/locale/en-US/zotero/zotero.properties
index 9bf92d8b44..dbbd18feb3 100644
--- a/chrome/locale/en-US/zotero/zotero.properties
+++ b/chrome/locale/en-US/zotero/zotero.properties
@@ -608,6 +608,8 @@ db.integrityCheck.errorsFixed = The errors in your Zotero database have been cor
 db.integrityCheck.errorsNotFixed = Zotero was unable to correct all the errors in your database.
 db.integrityCheck.reportInForums = You can report this problem in the Zotero Forums.
 
+zotero.preferences.chooseFileHandler = Choose File Handler
+
 zotero.preferences.update.updated = Updated
 zotero.preferences.update.upToDate = Up to date
 zotero.preferences.update.error = Error