Replace attachmentPageIndex with synced-setting-based mechanism

The page index needs to be per-person in group libraries, and it should
still work in read-only libraries, so it doesn't make sense to store it
on the item. This uses a synced setting in the user's library instead.

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

@@ -50,7 +50,6 @@ Zotero.Item = function(itemTypeOrID) {
 	this._attachmentSyncedModificationTime = null;
 	this._attachmentSyncedHash = null;
 	this._attachmentLastProcessedModificationTime = null;
-	this._attachmentPageIndex = null;
 	
 	// loadCreators
 	this._creators = [];
@@ -342,7 +341,6 @@ Zotero.Item.prototype._parseRowData = function(row) {
 			case 'attachmentSyncedModificationTime':
 			case 'attachmentSyncedHash':
 			case 'attachmentLastProcessedModificationTime':
-			case 'attachmentPageIndex':
 			case 'createdByUserID':
 			case 'lastModifiedByUserID':
 				break;
@@ -1728,13 +1726,13 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		let sql = "";
 		let cols = [
 			'parentItemID', 'linkMode', 'contentType', 'charsetID', 'path', 'syncState',
-			'storageModTime', 'storageHash', 'lastProcessedModificationTime', 'pageIndex'
+			'storageModTime', 'storageHash', 'lastProcessedModificationTime'
 		];
 		// TODO: Replace with UPSERT after SQLite 3.24.0
 		if (isNew) {
 			sql = "INSERT INTO itemAttachments "
 				+ "(itemID, " + cols.join(", ") + ") "
-				+ "VALUES (?,?,?,?,?,?,?,?,?,?,?)";
+				+ "VALUES (?,?,?,?,?,?,?,?,?,?)";
 		}
 		else {
 			sql = "UPDATE itemAttachments SET " + cols.join("=?, ") + "=? WHERE itemID=?";
@@ -1749,7 +1747,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		let storageModTime = this.attachmentSyncedModificationTime;
 		let storageHash = this.attachmentSyncedHash;
 		let lastProcessedModificationTime = this.attachmentLastProcessedModificationTime;
-		let pageIndex = this.attachmentPageIndex;
 		
 		if (linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE && libraryType != 'user') {
 			throw new Error("Linked files can only be added to user library");
@@ -1765,7 +1762,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			storageModTime !== undefined ? storageModTime : null,
 			storageHash || null,
 			lastProcessedModificationTime || null,
-			typeof pageIndex === 'number' ? pageIndex : null
 		];
 		if (isNew) {
 			params.unshift(itemID);
@@ -3222,7 +3218,7 @@ Zotero.defineProperty(Zotero.Item.prototype, 'attachmentSyncedHash', {
 //
 // PDF attachment properties
 //
-for (let name of ['lastProcessedModificationTime', 'pageIndex']) {
+for (let name of ['lastProcessedModificationTime']) {
 	let prop = 'attachment' + Zotero.Utilities.capitalize(name);
 	
 	Zotero.defineProperty(Zotero.Item.prototype, prop, {
@@ -3257,13 +3253,6 @@ for (let name of ['lastProcessedModificationTime', 'pageIndex']) {
 							+ "-- " + val + " given");
 					}
 					break;
-				
-				case 'pageIndex':
-					if (typeof val != 'number') {
-						Zotero.debug(val, 2);
-						throw new Error(`${prop} must be a number`);
-					}
-					break;
 			}
 			
 			if (val == this['_' + prop]) {
@@ -3280,6 +3269,59 @@ for (let name of ['lastProcessedModificationTime', 'pageIndex']) {
 }
 
 
+Zotero.Item.prototype.getAttachmentPageIndex = function () {
+	if (!this.isFileAttachment()) {
+		throw new Error("getAttachmentPageIndex() can only be called on file attachments");
+	}
+	
+	var id = this._getPageIndexSettingKey();
+	var val = Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, id);
+	if (val !== null && typeof val != 'number' || val != parseInt(val)) {
+		Zotero.logError(`Setting contains an invalid attachment page index ('${val}') -- discarding`);
+		return null;
+	}
+	return val;
+};
+
+Zotero.Item.prototype.setAttachmentPageIndex = async function (val) {
+	if (!this.isFileAttachment()) {
+		throw new Error("setAttachmentPageIndex() can only be called on file attachments");
+	}
+	
+	if (typeof val != 'number' || val != parseInt(val)) {
+		Zotero.debug(val, 2);
+		throw new Error(`setAttachmentPageIndex() must be passed an integer`);
+	}
+	
+	var id = this._getPageIndexSettingKey();
+	if (val === null) {
+		return Zotero.SyncedSettings.clear(id);
+	}
+	return Zotero.SyncedSettings.set(Zotero.Libraries.userLibraryID, id, val);
+};
+
+
+// pageIndex_u_ABCD2345
+Zotero.Item.prototype._getPageIndexSettingKey = function () {
+	var library = Zotero.Libraries.get(this.libraryID);
+	var id = 'pageIndex_';
+	switch (library.libraryType) {
+		case 'user':
+			id += 'u';
+			break;
+		
+		case 'group':
+			id += 'g' + library.libraryTypeID;
+			break;
+		
+		default:
+			throw new Error(`Can't get page index id for ${library.libraryType} item`);
+	}
+	id += "_" + this.key;
+	return id;
+};
+
+
 /**
  * Modification time of an attachment file
  *
@@ -4560,6 +4602,11 @@ Zotero.Item.prototype._eraseData = Zotero.Promise.coroutine(function* (env) {
 		
 		// Zotero.Sync.EventListeners.ChangeListener needs to know if this was a storage file
 		env.notifierData[this.id].storageDeleteLog = this.isStoredFileAttachment();
+		
+		if (this.isFileAttachment()) {
+			let id = this._getPageIndexSettingKey();
+			yield Zotero.SyncedSettings.clear(Zotero.Libraries.userLibraryID, id);
+		}
 	}
 	// Delete cached file for image annotation
 	else if (this.isAnnotation()) {
@@ -4760,10 +4807,6 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 			this['attachment' + field[0].toUpperCase() + field.substr(1)] = val;
 			break;
 			
-		case 'attachmentPageIndex':
-			this[field] = val;
-			break;
-		
 		//
 		// Annotation fields
 		//
@@ -5032,18 +5075,6 @@ Zotero.Item.prototype.toJSON = function (options = {}) {
 					//obj.md5 = (yield this.attachmentHash) || null;
 				}
 			}
-			
-			// PDF attachment properties
-			if (this.isFileAttachment()) {
-				let props = ['pageIndex'];
-				for (let prop of props) {
-					let fullProp = 'attachment' + Zotero.Utilities.capitalize(prop);
-					let val = this[fullProp];
-					if (val !== null && val !== undefined) {
-						obj[fullProp] = val;
-					}
-				}
-			}
 		}
 		
 		// Notes and embedded attachment notes

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

@@ -78,7 +78,6 @@ Zotero.Items = function() {
 				attachmentSyncedModificationTime: "IA.storageModTime AS attachmentSyncedModificationTime",
 				attachmentSyncedHash: "IA.storageHash AS attachmentSyncedHash",
 				attachmentLastProcessedModificationTime: "IA.lastProcessedModificationTime AS attachmentLastProcessedModificationTime",
-				attachmentPageIndex: "IA.pageIndex AS attachmentPageIndex"
 			};
 		}
 	}, {lazy: true});

chrome/content/zotero/xpcom/schema.js

@@ -3241,7 +3241,6 @@ Zotero.Schema = new function(){
 				
 				yield Zotero.DB.queryAsync("ALTER TABLE itemAttachments ADD COLUMN lastProcessedModificationTime INT");
 				yield Zotero.DB.queryAsync("CREATE INDEX itemAttachments_lastProcessedModificationTime ON itemAttachments(lastProcessedModificationTime)");
-				yield Zotero.DB.queryAsync("ALTER TABLE itemAttachments ADD COLUMN pageIndex INT");
 			}
 			
 			// If breaking compatibility or doing anything dangerous, clear minorUpdateFrom

resource/schema/userdata.sql

@@ -105,7 +105,6 @@ CREATE TABLE itemAttachments (
     storageModTime INT,
     storageHash TEXT,
     lastProcessedModificationTime INT,
-    pageIndex INT,
     FOREIGN KEY (itemID) REFERENCES items(itemID) ON DELETE CASCADE,
     FOREIGN KEY (parentItemID) REFERENCES items(itemID) ON DELETE CASCADE,
     FOREIGN KEY (charsetID) REFERENCES charsets(charsetID) ON DELETE SET NULL

test/tests/itemTest.js

@@ -1205,6 +1205,42 @@ describe("Zotero.Item", function () {
 	});
 	
 	
+	describe("Attachment Page Index", function () {
+		describe("#getAttachmentPageIndex()", function () {
+			it("should get the page index", async function () {
+				var attachment = await importFileAttachment('test.pdf');
+				assert.isNull(attachment.getAttachmentPageIndex());
+				await attachment.setAttachmentPageIndex(2);
+				assert.equal(2, attachment.getAttachmentPageIndex());
+			});
+			
+			it("should throw an error if called on a regular item", async function () {
+				var item = createUnsavedDataObject('item');
+				assert.throws(
+					() => item.getAttachmentPageIndex(),
+					"getAttachmentPageIndex() can only be called on file attachments"
+				);
+			});
+			
+			it("should discard invalid page index", async function () {
+				var attachment = await importFileAttachment('test.pdf');
+				var id = attachment._getPageIndexSettingKey();
+				await Zotero.SyncedSettings.set(Zotero.Libraries.userLibraryID, id, '"1"');
+				assert.isNull(attachment.getAttachmentPageIndex());
+			});
+		});
+		
+		it("should be cleared when item is deleted", async function () {
+			var attachment = await importFileAttachment('test.pdf');
+			await attachment.setAttachmentPageIndex(2);
+			var id = attachment._getPageIndexSettingKey();
+			assert.equal(2, Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, id));
+			await attachment.eraseTx();
+			assert.isNull(Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, id));
+		});
+	});
+	
+	
 	describe("Annotations", function () {
 		var item;
 		var attachment;
@@ -1808,13 +1844,6 @@ describe("Zotero.Item", function () {
 					assert.isNull(json.md5);
 				})
 				
-				it("should include PDF properties", async function () {
-					var item = await importPDFAttachment();
-					item.attachmentPageIndex = 4;
-					var json = item.toJSON();
-					assert.propertyVal(json, 'attachmentPageIndex', 4);
-				});
-				
 				it("shouldn't include filename, path, or PDF properties for linked_url attachments", function* () {
 					var item = new Zotero.Item('attachment');
 					item.attachmentLinkMode = 'linked_url';
@@ -1822,7 +1851,6 @@ describe("Zotero.Item", function () {
 					var json = item.toJSON();
 					assert.notProperty(json, "filename");
 					assert.notProperty(json, "path");
-					assert.notProperty(json, 'attachmentPageIndex');
 				});
 				
 				it("shouldn't include various properties on embedded-image attachments", async function () {
@@ -1838,7 +1866,6 @@ describe("Zotero.Item", function () {
 					assert.notProperty(json, 'note');
 					assert.notProperty(json, 'charset');
 					assert.notProperty(json, 'path');
-					assert.notProperty(json, 'attachmentPageIndex');
 				});
 			});
 			
@@ -2502,24 +2529,6 @@ describe("Zotero.Item", function () {
 			assert.propertyVal(item, 'attachmentCharset', 'utf-8');
 		});
 		
-		it("should import PDF fields", async function () {
-			var attachment = await importPDFAttachment();
-			var json = attachment.toJSON();
-			
-			var item = new Zotero.Item();
-			item.libraryID = attachment.libraryID;
-			item.fromJSON(json, { strict: true });
-			assert.propertyVal(item, 'attachmentPageIndex', null);
-			
-			json.attachmentPageIndex = 4;
-			
-			item = new Zotero.Item();
-			item.libraryID = attachment.libraryID;
-			item.fromJSON(json, { strict: true });
-			
-			assert.propertyVal(item, 'attachmentPageIndex', json.attachmentPageIndex);
-		});
-		
 		it("should import annotation fields", async function () {
 			var attachment = await importPDFAttachment();