From dcfaa5521e25c29f2218bd7e1b8666e66df4dd70 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Mon, 11 Dec 2017 02:23:15 -0500
Subject: [PATCH] Don't show note in right-hand pane when editing in separate
 window

- When a child note is opened in a separate window, the parent window is
  selected. (This used to work but was broken in 5.0.)
- When a top-level note is opened (via double-click), the right-hand pane
  changes to show "Editing in separate window".
- If a note that's currently open in a separate window is clicked on,
  the right-hand pane shows "Editing in a separate window".
- If a note window is closed and the item is selected, the note editor
  reappears in the right-hand pane after the note is saved.

This will avoid unnecessary UI updates and data loss bugs from the two
notes getting out of sync (and is just generally cleaner).

Also:

- General cleanup of note display code
---
 chrome/content/zotero/itemPane.js            | 52 +++++++++++++
 chrome/content/zotero/itemPane.xul           |  4 +-
 chrome/content/zotero/note.js                | 72 +++++++++---------
 chrome/content/zotero/note.xul               |  1 -
 chrome/content/zotero/zoteroPane.js          | 79 +++++++++-----------
 chrome/locale/en-US/zotero/zotero.properties |  1 +
 6 files changed, 126 insertions(+), 83 deletions(-)

diff --git a/chrome/content/zotero/itemPane.js b/chrome/content/zotero/itemPane.js
index 1d12df393..789755a93 100644
--- a/chrome/content/zotero/itemPane.js
+++ b/chrome/content/zotero/itemPane.js
@@ -222,6 +222,58 @@ var ZoteroItemPane = new function() {
 	});
 	
 	
+	this.onNoteSelected = function (item, editable) {
+		// If an external note window is open for this item, don't show the editor
+		if (ZoteroPane.findNoteWindow(item.id)) {
+			this.showNoteWindowMessage();
+			return;
+		}
+		
+		var noteEditor = document.getElementById('zotero-note-editor');
+		noteEditor.mode = editable ? 'edit' : 'view';
+		
+		noteEditor.parent = null;
+		noteEditor.item = item;
+		
+		// If loading new or different note, disable undo while we repopulate the text field
+		// so Undo doesn't end up clearing the field. This also ensures that Undo doesn't
+		// undo content from another note into the current one.
+		var clearUndo = noteEditor.item ? noteEditor.item.id != item.id : false;
+		if (clearUndo) {
+			noteEditor.clearUndo();
+		}
+		
+		document.getElementById('zotero-view-note-button').hidden = !editable;
+		document.getElementById('zotero-item-pane-content').selectedIndex = 2;
+	};
+	
+	
+	this.showNoteWindowMessage = function () {
+		ZoteroPane.setItemPaneMessage(Zotero.getString('pane.item.notes.editingInWindow'));
+	};
+	
+	
+	/**
+	 * Select the parent item and open the note editor
+	 */
+	this.openNoteWindow = async function () {
+		var noteEditor = document.getElementById('zotero-note-editor');
+		var item = noteEditor.item;
+		// We don't want to show the note in two places, since it causes unnecessary UI updates
+		// and can result in weird bugs where note content gets lost.
+		//
+		// If this is a child note, select the parent
+		if (item.parentID) {
+			await ZoteroPane.selectItem(item.parentID);
+		}
+		// Otherwise, hide note and replace with a message that we're editing externally
+		else {
+			this.showNoteWindowMessage();
+		}
+		ZoteroPane.openNoteWindow(item.id);
+	};
+	
+	
 	this.addNote = function (popup) {
 		ZoteroPane_Local.newNote(popup, _lastItem.key);
 	}
diff --git a/chrome/content/zotero/itemPane.xul b/chrome/content/zotero/itemPane.xul
index 8f40a346b..6bc6a5678 100644
--- a/chrome/content/zotero/itemPane.xul
+++ b/chrome/content/zotero/itemPane.xul
@@ -114,7 +114,9 @@
 				<zoteronoteeditor id="zotero-note-editor" flex="1" notitle="1"
 					previousfocus="zotero-items-tree"
 					onerror="ZoteroPane.displayErrorMessage(); this.mode = 'view'"/>
-				<button id="zotero-view-note-button" label="&zotero.notes.separate;" oncommand="ZoteroPane_Local.openNoteWindow(this.getAttribute('noteID')); if(this.hasAttribute('sourceID')) ZoteroPane_Local.selectItem(this.getAttribute('sourceID'));"/>
+				<button id="zotero-view-note-button"
+						label="&zotero.notes.separate;"
+						oncommand="ZoteroItemPane.openNoteWindow()"/>
 			</groupbox>
 			
 			<!-- Attachment item -->
diff --git a/chrome/content/zotero/note.js b/chrome/content/zotero/note.js
index bc8618e06..57dd99589 100644
--- a/chrome/content/zotero/note.js
+++ b/chrome/content/zotero/note.js
@@ -26,7 +26,7 @@
 var noteEditor;
 var notifierUnregisterID;
 
-function onLoad() {
+async function onLoad() {
 	noteEditor = document.getElementById('zotero-note-editor');
 	noteEditor.mode = 'edit';
 	noteEditor.focus();
@@ -37,42 +37,41 @@ function onLoad() {
 	if (window.arguments) {
 		var io = window.arguments[0];
 	}
+	
 	var itemID = parseInt(io.itemID);
 	var collectionID = parseInt(io.collectionID);
 	var parentItemKey = io.parentItemKey;
 	
-	return Zotero.spawn(function* () {
-		if (itemID) {
-			var ref = yield Zotero.Items.getAsync(itemID);
-			
-			var clearUndo = noteEditor.item ? noteEditor.item.id != ref.id : false;
-			
-			noteEditor.item = ref;
-			
-			// If loading new or different note, disable undo while we repopulate the text field
-			// so Undo doesn't end up clearing the field. This also ensures that Undo doesn't
-			// undo content from another note into the current one.
-			if (clearUndo) {
-				noteEditor.clearUndo();
-			}
-			
-			document.title = ref.getNoteTitle();
-		}
-		else {
-			if (parentItemKey) {
-				var ref = Zotero.Items.getByLibraryAndKey(parentItemKey);
-				noteEditor.parentItem = ref;
-			}
-			else {
-				if (collectionID && collectionID != '' && collectionID != 'undefined') {
-					noteEditor.collection = Zotero.Collections.get(collectionID);
-				}
-			}
-			noteEditor.refresh();
+	if (itemID) {
+		var ref = await Zotero.Items.getAsync(itemID);
+		
+		var clearUndo = noteEditor.item ? noteEditor.item.id != ref.id : false;
+		
+		noteEditor.item = ref;
+		
+		// If loading new or different note, disable undo while we repopulate the text field
+		// so Undo doesn't end up clearing the field. This also ensures that Undo doesn't
+		// undo content from another note into the current one.
+		if (clearUndo) {
+			noteEditor.clearUndo();
 		}
 		
-		notifierUnregisterID = Zotero.Notifier.registerObserver(NotifyCallback, 'item');
-	});
+		document.title = ref.getNoteTitle();
+	}
+	else {
+		if (parentItemKey) {
+			var ref = Zotero.Items.getByLibraryAndKey(parentItemKey);
+			noteEditor.parentItem = ref;
+		}
+		else {
+			if (collectionID && collectionID != '' && collectionID != 'undefined') {
+				noteEditor.collection = Zotero.Collections.get(collectionID);
+			}
+		}
+		noteEditor.refresh();
+	}
+	
+	notifierUnregisterID = Zotero.Notifier.registerObserver(NotifyCallback, 'item', 'noteWindow');
 }
 
 // If there's an error saving a note, close the window and crash the app
@@ -86,12 +85,13 @@ function onError() {
 	window.close();
 }
 
-function onUnload()
-{
-	if(noteEditor && noteEditor.value)
-		noteEditor.save();
-	
+
+function onUnload() {
 	Zotero.Notifier.unregisterObserver(notifierUnregisterID);
+	
+	if (noteEditor.item) {
+		window.opener.ZoteroPane.onNoteWindowClosed(noteEditor.item.id, noteEditor.value);
+	}
 }
 
 var NotifyCallback = {
diff --git a/chrome/content/zotero/note.xul b/chrome/content/zotero/note.xul
index 1f2b2f161..8d58acce7 100644
--- a/chrome/content/zotero/note.xul
+++ b/chrome/content/zotero/note.xul
@@ -15,7 +15,6 @@
     xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
 
 	<script src="include.js"/>
-	<script src="itemPane.js"/>
 	<script src="note.js"/>
 
 	<keyset>
diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js
index 352ca7206..7dd8f139f 100644
--- a/chrome/content/zotero/zoteroPane.js
+++ b/chrome/content/zotero/zoteroPane.js
@@ -53,7 +53,6 @@ var ZoteroPane = new function()
 	this.setItemsPaneMessage = setItemsPaneMessage;
 	this.clearItemsPaneMessage = clearItemsPaneMessage;
 	this.contextPopupShowing = contextPopupShowing;
-	this.openNoteWindow = openNoteWindow;
 	this.viewSelectedAttachment = viewSelectedAttachment;
 	this.reportErrors = reportErrors;
 	this.displayErrorMessage = displayErrorMessage;
@@ -1479,37 +1478,7 @@ var ZoteroPane = new function()
 				var item = selectedItems[0];
 				
 				if (item.isNote()) {
-					var noteEditor = document.getElementById('zotero-note-editor');
-					noteEditor.mode = this.collectionsView.editable ? 'edit' : 'view';
-					
-					var clearUndo = noteEditor.item ? noteEditor.item.id != item.id : false;
-					
-					noteEditor.parent = null;
-					noteEditor.item = item;
-					
-					// If loading new or different note, disable undo while we repopulate the text field
-					// so Undo doesn't end up clearing the field. This also ensures that Undo doesn't
-					// undo content from another note into the current one.
-					if (clearUndo) {
-						noteEditor.clearUndo();
-					}
-					
-					var viewButton = document.getElementById('zotero-view-note-button');
-					if (this.collectionsView.editable) {
-						viewButton.hidden = false;
-						viewButton.setAttribute('noteID', item.id);
-						if (!item.isTopLevelItem()) {
-							viewButton.setAttribute('parentItemID', item.parentItemID);
-						}
-						else {
-							viewButton.removeAttribute('parentItemID');
-						}
-					}
-					else {
-						viewButton.hidden = true;
-					}
-					
-					document.getElementById('zotero-item-pane-content').selectedIndex = 2;
+					ZoteroItemPane.onNoteSelected(item, this.collectionsView.editable);
 				}
 				
 				else if (item.isAttachment()) {
@@ -3640,8 +3609,7 @@ var ZoteroPane = new function()
 	
 	
 	
-	function openNoteWindow(itemID, col, parentKey)
-	{
+	this.openNoteWindow = function (itemID, col, parentKey) {
 		if (!this.canEdit()) {
 			this.displayCannotEditLibraryMessage();
 			return;
@@ -3650,22 +3618,17 @@ var ZoteroPane = new function()
 		var name = null;
 		
 		if (itemID) {
+			let w = this.findNoteWindow(itemID);
+			if (w) {
+				w.focus();
+				return;
+			}
+			
 			// Create a name for this window so we can focus it later
 			//
 			// Collection is only used on new notes, so we don't need to
 			// include it in the name
 			name = 'zotero-note-' + itemID;
-			
-			var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
-					.getService(Components.interfaces.nsIWindowMediator);
-			var e = wm.getEnumerator('zotero:note');
-			while (e.hasMoreElements()) {
-				var w = e.getNext();
-				if (w.name == name) {
-					w.focus();
-					return;
-				}
-			}
 		}
 		
 		var io = { itemID: itemID, collectionID: col, parentItemKey: parentKey };
@@ -3673,6 +3636,32 @@ var ZoteroPane = new function()
 	}
 	
 	
+	this.findNoteWindow = function (itemID) {
+		var name = 'zotero-note-' + itemID;
+		var wm = Services.wm;
+		var e = wm.getEnumerator('zotero:note');
+		while (e.hasMoreElements()) {
+			var w = e.getNext();
+			if (w.name == name) {
+				return w;
+			}
+		}
+	};
+	
+	
+	this.onNoteWindowClosed = async function (itemID, noteText) {
+		var item = Zotero.Items.get(itemID);
+		item.setNote(noteText);
+		await item.saveTx();
+		
+		// If note is still selected, show the editor again when the note window closes
+		var selectedItems = this.getSelectedItems(true);
+		if (selectedItems.length == 1 && itemID == selectedItems[0]) {
+			ZoteroItemPane.onNoteSelected(item, this.collectionsView.editable);
+		}
+	};
+	
+	
 	this.addAttachmentFromURI = Zotero.Promise.method(function (link, itemID) {
 		if (!this.canEdit()) {
 			this.displayCannotEditLibraryMessage();
diff --git a/chrome/locale/en-US/zotero/zotero.properties b/chrome/locale/en-US/zotero/zotero.properties
index b1770531b..e64723ae8 100644
--- a/chrome/locale/en-US/zotero/zotero.properties
+++ b/chrome/locale/en-US/zotero/zotero.properties
@@ -339,6 +339,7 @@ pane.item.notes.delete.confirm  = Are you sure you want to delete this note?
 pane.item.notes.count.zero      = %S notes:
 pane.item.notes.count.singular  = %S note:
 pane.item.notes.count.plural    = %S notes:
+pane.item.notes.editingInWindow = Editing in separate window
 pane.item.attachments.rename.title	= New title:
 pane.item.attachments.rename.renameAssociatedFile		= Rename associated file
 pane.item.attachments.rename.error					= An error occurred while renaming the file.