Set automatic titles in more or less all cases (#4369)

By moving the setAutoAttachmentTitle() calls to importFromFile() /
_addToDB().

Also:

- Chop off file extension when setting the parent's title based on the
  filename in Create Parent Item -> Manual Entry.
- Fix Manual Entry not renaming the attachment correctly by awaiting
  createEmptyParent().

chrome/content/zotero/itemTree.jsx

@@ -2498,8 +2498,6 @@ var ItemTree = class ItemTree extends LibraryTree {
 					}
 
 					if (item) {
-						item.setAutoAttachmentTitle();
-						await item.saveTx();
 						addedItems.push(item);
 					}
 				}

chrome/content/zotero/xpcom/attachments.js

@@ -97,7 +97,12 @@ Zotero.Attachments = new function () {
 				else if (libraryID) {
 					attachmentItem.libraryID = libraryID;
 				}
-				attachmentItem.setField('title', title != undefined ? title : newName);
+				// If we have an explicit title, set it now
+				// Otherwise do it below once we've set the other attachment properties
+				// and can generate a title via setAutoAttachmentTitle()
+				if (title != undefined) {
+					attachmentItem.setField('title', title);
+				}
 				attachmentItem.parentID = parentItemID;
 				attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE;
 				if (collections) {
@@ -130,6 +135,9 @@ Zotero.Attachments = new function () {
 					attachmentItem.attachmentCharset = charset;
 				}
 				attachmentItem.attachmentPath = newFile.path;
+				if (title == undefined) {
+					attachmentItem.setAutoAttachmentTitle();
+				}
 				await attachmentItem.save(saveOptions);
 			}.bind(this));
 			try {
@@ -187,7 +195,7 @@ Zotero.Attachments = new function () {
 		
 		var item = yield _addToDB({
 			file,
-			title: title != undefined ? title : file.leafName,
+			title,
 			linkMode: this.LINK_MODE_LINKED_FILE,
 			contentType,
 			charset,
@@ -2873,7 +2881,7 @@ Zotero.Attachments = new function () {
 	 * @param {Object} options
 	 * @param {nsIFile|String} [file]
 	 * @param {String} [url]
-	 * @param {String} title
+	 * @param {String} [title]
 	 * @param {Number} linkMode
 	 * @param {String} contentType
 	 * @param {String} [charset]
@@ -2904,7 +2912,6 @@ Zotero.Attachments = new function () {
 				}
 				attachmentItem.libraryID = parentLibraryID;
 			}
-			attachmentItem.setField('title', title);
 			if (linkMode == self.LINK_MODE_IMPORTED_URL || linkMode == self.LINK_MODE_LINKED_URL) {
 				attachmentItem.setField('url', url);
 				attachmentItem.setField('accessDate', "CURRENT_TIMESTAMP");
@@ -2921,6 +2928,14 @@ Zotero.Attachments = new function () {
 			if (collections) {
 				attachmentItem.setCollections(collections);
 			}
+
+			if (title == undefined) {
+				attachmentItem.setAutoAttachmentTitle();
+			}
+			else {
+				attachmentItem.setField('title', title);
+			}
+
 			await attachmentItem.save(saveOptions);
 			
 			return attachmentItem;

chrome/content/zotero/xpcom/data/item.js

@@ -3881,13 +3881,14 @@ Zotero.Item.prototype.setAutoAttachmentTitle = function () {
 	if (!this.isAttachment()) {
 		throw new Error("setAutoAttachmentTitle() can only be called on attachment items");
 	}
-	if (!this.isFileAttachment() || !this.parentItemID) {
+	if (!this.isFileAttachment()) {
 		return;
 	}
 	
 	// If this is the only attachment of its type on the parent item, give it
 	// a default title ("PDF", "Webpage", etc.)
-	let isFirstOfType = this.parentItem.numFileAttachmentsWithContentType(this.attachmentContentType) <= 1;
+	let isFirstOfType = this.parentItemID
+		&& this.parentItem.numFileAttachmentsWithContentType(this.attachmentContentType) <= 1;
 	if (isFirstOfType) {
 		let defaultTitle = this._getDefaultTitleForAttachmentContentType();
 		if (defaultTitle !== null) {

chrome/content/zotero/zoteroPane.js

@@ -4416,13 +4416,6 @@ var ZoteroPane = new function()
 				});
 			}
 			
-			try {
-				item.setAutoAttachmentTitle();
-				await item.saveTx();
-			}
-			catch (e) {
-				Zotero.logError(e);
-			}
 			addedItems.push(item);
 		}
 		
@@ -5343,7 +5336,7 @@ var ZoteroPane = new function()
 			}
 			// If they clicked manual entry then make a dummy parent
 			else {
-				this.createEmptyParent(item);
+				await this.createEmptyParent(item);
 			}
 		}
 
@@ -5560,11 +5553,19 @@ var ZoteroPane = new function()
 				var parent = new Zotero.Item('document');
 			}
 			parent.libraryID = item.libraryID;
-			parent.setField('title', item.getField('title'));
+			
+			let title = item.getField('title');
+			// If the attachment was named after its filename, remove the extension
+			if (title === item.attachmentFilename) {
+				title = title.replace(/\.[^.]+$/, '');
+			}
+			parent.setField('title', title);
+			
 			if (item.isWebAttachment()) {
 				parent.setField('accessDate', item.getField('accessDate'));
 				parent.setField('url', item.getField('url'));
 			}
+			
 			let itemID = await parent.save();
 			item.parentID = itemID;
 			await item.save();

test/content/support.js

@@ -1003,9 +1003,11 @@ function importFileAttachment(filename, options = {}) {
 	let importOptions = {
 		file,
 		parentItemID: options.parentID,
-		title: options.title
 	};
 	Object.assign(importOptions, options);
+	// Override default behavior - don't set title based on type,
+	// just use extension-less leafName
+	importOptions.title ??= file.leafName.replace(/\.[^.]+$/, '');
 	return Zotero.Attachments.importFromFile(importOptions);
 }
 

test/tests/annotationsTest.js

@@ -414,7 +414,7 @@ describe("Create a note from annotations from multiple items and attachments", f
 		let annotation2 = await createAnnotation('highlight', attachment);
 		annotations.push(annotation2);
 		let note = await Zotero.EditorInstance.createNoteFromAnnotations(annotations);
-		assert.equal(note.note.split('test.pdf').length - 1, 1);
+		assert.equal(note.note.split('test').length - 1, 1);
 		assert.equal(note.note.split(annotation1.annotationText).length - 1, 1);
 		assert.equal(note.note.split(annotation2.annotationText).length - 1, 1);
 	});
@@ -429,7 +429,7 @@ describe("Create a note from annotations from multiple items and attachments", f
 		let annotation2 = await createAnnotation('highlight', attachment2);
 		annotations.push(annotation2);
 		let note = await Zotero.EditorInstance.createNoteFromAnnotations(annotations);
-		assert.equal(note.note.split('test.pdf').length - 1, 2);
+		assert.equal(note.note.split('test').length - 1, 2);
 		assert.equal(note.note.split('>' + item.getField('title') + '<').length - 1, 0);
 		assert.equal(note.note.split(annotation1.annotationText).length - 1, 1);
 		assert.equal(note.note.split(annotation2.annotationText).length - 1, 1);
@@ -446,7 +446,7 @@ describe("Create a note from annotations from multiple items and attachments", f
 		let annotation2 = await createAnnotation('highlight', attachment2);
 		annotations.push(annotation2);
 		let note = await Zotero.EditorInstance.createNoteFromAnnotations(annotations);
-		assert.equal(note.note.split('test.pdf').length - 1, 0);
+		assert.equal(note.note.split('test').length - 1, 0);
 		assert.equal(note.note.split('>' + item1.getField('title') + '<').length - 1, 1);
 		assert.equal(note.note.split('>' + item2.getField('title') + '<').length - 1, 1);
 		assert.equal(note.note.split(annotation1.annotationText).length - 1, 1);
@@ -470,7 +470,7 @@ describe("Create a note from annotations from multiple items and attachments", f
 		annotations.push(annotation4);
 		let note = await Zotero.EditorInstance.createNoteFromAnnotations(annotations);
 		Zotero.debug(note.note);
-		assert.equal(note.note.split('test.pdf').length - 1, 2);
+		assert.equal(note.note.split('test').length - 1, 2);
 		assert.equal(note.note.split('>' + item1.getField('title') + '<').length - 1, 1);
 		assert.equal(note.note.split('>' + item2.getField('title') + '<').length - 1, 1);
 		assert.equal(note.note.split(annotation1.annotationText).length - 1, 1);

test/tests/attachmentsTest.js

@@ -92,6 +92,28 @@ describe("Zotero.Attachments", function() {
 			// Clean up
 			yield Zotero.Items.erase(item.id);
 		});
+
+		it("should set a top-level item's title to the filename, minus its extension", async function () {
+			let file = getTestDataDirectory();
+			file.append('test.pdf');
+			let attachment = await Zotero.Attachments.importFromFile({
+				file: file,
+			});
+			assert.equal(attachment.getField('title'), 'test');
+			await attachment.eraseTx();
+		});
+
+		it("should set a child item's title to the filename, minus its extension", async function () {
+			let file = getTestDataDirectory();
+			file.append('test.pdf');
+			let parent = await createDataObject('item');
+			let attachment = await Zotero.Attachments.importFromFile({
+				file: file,
+				parentItemID: parent.id,
+			});
+			assert.equal(attachment.getField('title'), Zotero.getString('fileTypes.pdf'));
+			await parent.eraseTx();
+		});
 	})
 	
 	describe("#linkFromFile()", function () {
@@ -111,6 +133,28 @@ describe("Zotero.Attachments", function() {
 		it.skip("should throw an error for a non-user library", function* () {
 			// Should create a group library for use by all tests
 		})
+
+		it("should set a top-level item's title to the filename, minus its extension", async function () {
+			let file = getTestDataDirectory();
+			file.append('test.pdf');
+			let attachment = await Zotero.Attachments.linkFromFile({
+				file: file,
+			});
+			assert.equal(attachment.getField('title'), 'test');
+			await attachment.eraseTx();
+		});
+
+		it("should set a child item's title to the filename, minus its extension", async function () {
+			let file = getTestDataDirectory();
+			file.append('test.pdf');
+			let parent = await createDataObject('item');
+			let attachment = await Zotero.Attachments.linkFromFile({
+				file: file,
+				parentItemID: parent.id,
+			});
+			assert.equal(attachment.getField('title'), Zotero.getString('fileTypes.pdf'));
+			await parent.eraseTx();
+		});
 	})
 	
 	

test/tests/translateTest.js

@@ -2185,7 +2185,7 @@ describe("Zotero.Translate.ItemGetter", function() {
 				yield Zotero.Attachments.linkFromFile({"file":file}), // Standalone link to file
 				yield Zotero.Attachments.importFromFile({"file":file, "parentItemID":item.id}), // Attached stored file
 				yield Zotero.Attachments.linkFromFile({"file":file, "parentItemID":item.id}), // Attached link to file
-				yield Zotero.Attachments.linkFromURL({"url":'http://example.com', "parentItemID":item.id, "contentType":'application/pdf', "title":'empty.pdf'}) // Attached link to URL
+				yield Zotero.Attachments.linkFromURL({"url":'http://example.com', "parentItemID":item.id, "contentType":'application/pdf', "title":'empty'}) // Attached link to URL
 			];
 			
 			yield Zotero.DB.executeTransaction(async function () {
@@ -2293,7 +2293,7 @@ describe("Zotero.Translate.ItemGetter", function() {
 					
 					// Set fields
 					assert.equal(attachment.itemType, 'attachment', prefix + 'itemType is correct' + suffix);
-					assert.equal(attachment.title, 'empty.pdf', prefix + 'title is correct' + suffix);
+					assert.include([Zotero.getString('fileTypes.pdf'), 'empty'], attachment.title, prefix + 'title is correct' + suffix);
 					assert.equal(attachment.url, 'http://example.com', prefix + 'url is correct' + suffix);
 					assert.equal(attachment.note, 'note', prefix + 'note is correct' + suffix);
 					
@@ -2325,10 +2325,10 @@ describe("Zotero.Translate.ItemGetter", function() {
 						assert.isString(attachment.localPath, prefix + 'localPath is set' + suffix);
 						let attachmentFile = Zotero.File.pathToFile(attachment.localPath);
 						assert.isTrue(attachmentFile.exists(), prefix + 'localPath points to a file' + suffix);
-						assert.isTrue(attachmentFile.equals(attachments[j].getFile()), prefix + 'localPath points to the correct file' + suffix);
+						assert.equal(attachmentFile.spec, zoteroItem.getFile().spec, prefix + 'localPath points to the correct file' + suffix);
 						
 						assert.equal(attachment.filename, 'empty.pdf', prefix + 'filename is correct' + suffix);
-						assert.equal(attachment.defaultPath, 'files/' + attachments[j].id + '/' + attachment.filename, prefix + 'defaultPath is correct' + suffix);
+						assert.equal(attachment.defaultPath, 'files/' + zoteroItem.id + '/' + attachment.filename, prefix + 'defaultPath is correct' + suffix);
 						
 						// saveFile function
 						assert.isFunction(attachment.saveFile, prefix + 'has saveFile function' + suffix);

test/tests/zoteroPaneTest.js

@@ -391,7 +391,7 @@ describe("ZoteroPane", function() {
 			var doc = dp.parseFromString(note.getNote(), 'text/html');
 			assert.sameMembers(
 				[...doc.querySelectorAll('h3')].map(x => x.textContent),
-				[attachment1.attachmentFilename, attachment2.attachmentFilename]
+				[attachment1.getField('title'), attachment2.getField('title')]
 			);
 			assert.lengthOf([...doc.querySelectorAll('h3 + p')], 2);
 			assert.lengthOf([...doc.querySelectorAll('span.highlight')], 4);
@@ -415,7 +415,7 @@ describe("ZoteroPane", function() {
 			var doc = dp.parseFromString(note.getNote(), 'text/html');
 			assert.sameMembers(
 				[...doc.querySelectorAll('h3')].map(x => x.textContent),
-				[attachment1.attachmentFilename, attachment2.attachmentFilename]
+				[attachment1.getField('title'), attachment2.getField('title')]
 			);
 			// No item titles
 			assert.lengthOf([...doc.querySelectorAll('h2 + p')], 0);
@@ -493,10 +493,10 @@ describe("ZoteroPane", function() {
 			assert.sameMembers(
 				[...doc.querySelectorAll('h3')].map(x => x.textContent),
 				[
-					attachment1.attachmentFilename,
+					attachment1.getField('title'),
-					attachment2.attachmentFilename,
+					attachment2.getField('title'),
-					attachment3.attachmentFilename,
+					attachment3.getField('title'),
-					attachment4.attachmentFilename
+					attachment4.getField('title')
 				]
 			);
 			assert.lengthOf([...doc.querySelectorAll('h3 + p')], 4);