From d6d4e4b33e8a88cda4e5ef83b6bfff5b67b26871 Mon Sep 17 00:00:00 2001
From: Adomas Ven <adomas.ven@gmail.com>
Date: Tue, 13 Dec 2016 16:07:43 +0200
Subject: [PATCH] Saner feed database management (#1131)

* Mark feedItems read in a single batch SQL update
* Automatically remove old feed items
* User-facing preference globally and per-feed for feed item expiration
---
 chrome/content/zotero/feedSettings.js         | 19 +++--
 chrome/content/zotero/feedSettings.xul        | 11 ++-
 .../preferences/preferences_advanced.xul      | 16 +++-
 chrome/content/zotero/xpcom/data/feed.js      | 73 ++++++++++---------
 chrome/content/zotero/xpcom/data/feedItems.js | 24 +++---
 chrome/content/zotero/xpcom/data/feeds.js     | 23 ++++--
 chrome/content/zotero/xpcom/schema.js         | 10 ++-
 chrome/content/zotero/zoteroPane.js           | 12 ++-
 chrome/locale/en-US/zotero/zotero.dtd         |  6 +-
 defaults/preferences/zotero.js                |  3 +-
 resource/schema/userdata.sql                  |  5 +-
 test/content/support.js                       |  3 +-
 test/tests/feedTest.js                        | 42 +++++++----
 test/tests/feedsTest.js                       |  3 +-
 14 files changed, 157 insertions(+), 93 deletions(-)

diff --git a/chrome/content/zotero/feedSettings.js b/chrome/content/zotero/feedSettings.js
index 7eedd14331..34b7576a0e 100644
--- a/chrome/content/zotero/feedSettings.js
+++ b/chrome/content/zotero/feedSettings.js
@@ -72,9 +72,13 @@ var Zotero_Feed_Settings = new function() {
 		}
 		document.getElementById('feed-ttl').value = ttl;
 		
-		let cleanupAfter = data.cleanupAfter;
-		if (cleanupAfter === undefined) cleanupAfter = Zotero.Prefs.get('feeds.defaultCleanupAfter');
-		document.getElementById('feed-cleanupAfter').value = cleanupAfter;
+		let cleanupReadAfter = data.cleanupReadAfter;
+		if (cleanupReadAfter === undefined) cleanupReadAfter = Zotero.Prefs.get('feeds.defaultCleanupReadAfter');
+		document.getElementById('feed-cleanupReadAfter').value = cleanupReadAfter;
+		
+		let cleanupUnreadAfter = data.cleanupUnreadAfter;
+		if (cleanupUnreadAfter === undefined) cleanupUnreadAfter = Zotero.Prefs.get('feeds.defaultCleanupUnreadAfter');
+		document.getElementById('feed-cleanupUnreadAfter').value = cleanupUnreadAfter;
 		
 		if (data.url && !data.urlIsValid) {
 			yield this.validateURL();
@@ -93,7 +97,8 @@ var Zotero_Feed_Settings = new function() {
 		urlIsValid = false;
 		document.getElementById('feed-title').disabled = true;
 		document.getElementById('feed-ttl').disabled = true;
-		document.getElementById('feed-cleanupAfter').disabled = true;
+		document.getElementById('feed-cleanupReadAfter').disabled = true;
+		document.getElementById('feed-cleanupUnreadAfter').disabled = true;
 		document.documentElement.getButton('accept').disabled = true;
 	};
 	
@@ -130,7 +135,8 @@ var Zotero_Feed_Settings = new function() {
 			urlIsValid = true;
 			title.disabled = false;
 			ttl.disabled = false;
-			document.getElementById('feed-cleanupAfter').disabled = false;
+			document.getElementById('feed-cleanupReadAfter').disabled = false;
+			document.getElementById('feed-cleanupUnreadAfter').disabled = false;
 			document.documentElement.getButton('accept').disabled = false;
 		}
 		catch (e) {
@@ -145,7 +151,8 @@ var Zotero_Feed_Settings = new function() {
 		data.url = document.getElementById('feed-url').value;
 		data.title = document.getElementById('feed-title').value;
 		data.ttl = document.getElementById('feed-ttl').value * 60;
-		data.cleanupAfter = document.getElementById('feed-cleanupAfter').value * 1;
+		data.cleanupReadAfter = document.getElementById('feed-cleanupReadAfter').value * 1;
+		data.cleanupUnreadAfter = document.getElementById('feed-cleanupUnreadAfter').value * 1;
 		return true;
 	};
 	
diff --git a/chrome/content/zotero/feedSettings.xul b/chrome/content/zotero/feedSettings.xul
index 241ba3022e..451b595e56 100644
--- a/chrome/content/zotero/feedSettings.xul
+++ b/chrome/content/zotero/feedSettings.xul
@@ -53,9 +53,14 @@
 				<label value="&zotero.feedSettings.refresh.label2;" control="feed-ttl"/>
 			</hbox>
 			<hbox align="center">
-				<label value="&zotero.feedSettings.cleanupAfter.label1;" control="feed-cleanupAfter"/>
-				<textbox id="feed-cleanupAfter" type="number" min="0" increment="1" size="2"/>
-				<label value="&zotero.feedSettings.cleanupAfter.label2;" control="feed-cleanupAfter"/>
+				<label value="&zotero.feedSettings.cleanupReadAfter.label1;" control="feed-cleanupReadAfter"/>
+				<textbox id="feed-cleanupReadAfter" type="number" min="0" increment="1" size="2"/>
+				<label value="&zotero.feedSettings.cleanupReadAfter.label2;" control="feed-cleanupReadAfter"/>
+			</hbox>
+			<hbox align="center">
+				<label value="&zotero.feedSettings.cleanupUnreadAfter.label1;" control="feed-cleanupUnreadAfter"/>
+				<textbox id="feed-cleanupUnreadAfter" type="number" min="0" increment="1" size="2"/>
+				<label value="&zotero.feedSettings.cleanupUnreadAfter.label2;" control="feed-cleanupUnreadAfter"/>
 			</hbox>
 		</vbox>
 	</vbox>
diff --git a/chrome/content/zotero/preferences/preferences_advanced.xul b/chrome/content/zotero/preferences/preferences_advanced.xul
index fabd06535d..3f9a5080bd 100644
--- a/chrome/content/zotero/preferences/preferences_advanced.xul
+++ b/chrome/content/zotero/preferences/preferences_advanced.xul
@@ -59,7 +59,8 @@
 
 			<preference id="pref-feeds-sortAscending" name="extensions.zotero.feeds.sortAscending" type="bool"/>
 			<preference id="pref-feeds-defaultTTL" name="extensions.zotero.feeds.defaultTTL" type="int"/>
-			<preference id="pref-feeds-defaultCleanupAfter" name="extensions.zotero.feeds.defaultCleanupAfter" type="int"/>
+			<preference id="pref-feeds-defaultCleanupUnreadAfter" name="extensions.zotero.feeds.defaultCleanupUnreadAfter" type="int"/>
+			<preference id="pref-feeds-defaultCleanupReadAfter" name="extensions.zotero.feeds.defaultCleanupReadAfter" type="int"/>
 		</preferences>
 		
 		<tabbox id="zotero-prefpane-advanced-tabs">
@@ -319,9 +320,16 @@
 						</hbox>
 						<hbox>
 							<hbox align="center">
-								<label value="&zotero.feedSettings.cleanupAfter.label1;"/>
-								<textbox type="number" min="0" increment="1" size="2" preference="pref-feeds-defaultCleanupAfter"/>
-								<label value="&zotero.feedSettings.cleanupAfter.label2;"/>
+								<label value="&zotero.feedSettings.cleanupReadAfter.label1;"/>
+								<textbox type="number" min="0" increment="1" size="2" preference="pref-feeds-defaultCleanupReadAfter"/>
+								<label value="&zotero.feedSettings.cleanupReadAfter.label2;"/>
+							</hbox>
+						</hbox>
+						<hbox>
+							<hbox align="center">
+								<label value="&zotero.feedSettings.cleanupUnreadAfter.label1;"/>
+								<textbox type="number" min="0" increment="1" size="2" preference="pref-feeds-defaultCleanupUnreadAfter"/>
+								<label value="&zotero.feedSettings.cleanupUnreadAfter.label2;"/>
 							</hbox>
 						</hbox>
 					</groupbox>
diff --git a/chrome/content/zotero/xpcom/data/feed.js b/chrome/content/zotero/xpcom/data/feed.js
index d4784df6a4..2d992517a9 100644
--- a/chrome/content/zotero/xpcom/data/feed.js
+++ b/chrome/content/zotero/xpcom/data/feed.js
@@ -29,7 +29,8 @@
  * Custom parameters:
  * - name - name of the feed displayed in the collection tree
  * - url
- * - cleanupAfter - number of days after which read items should be removed
+ * - cleanupReadAfter - number of days after which read items should be removed
+ * - cleanupUnreadAfter - number of days after which unread items should be removed
  * - refreshInterval - in terms of hours
  * 
  * @param params
@@ -39,8 +40,9 @@
 Zotero.Feed = function(params = {}) {
 	params.libraryType = 'feed';
 	Zotero.Feed._super.call(this, params);
-	
-	this._feedCleanupAfter = null;
+
+	this._feedCleanupReadAfter = null;
+	this._feedCleanupUnreadAfter = null;
 	this._feedRefreshInterval = null;
 
 	// Feeds are not editable by the user. Remove the setter
@@ -56,7 +58,7 @@ Zotero.Feed = function(params = {}) {
 	});
 	
 	Zotero.Utilities.assignProps(this, params, 
-		['name', 'url', 'refreshInterval', 'cleanupAfter']);
+		['name', 'url', 'refreshInterval', 'cleanupReadAfter', 'cleanupUnreadAfter']);
 	
 	// Return a proxy so that we can disable the object once it's deleted
 	return new Proxy(this, {
@@ -86,7 +88,7 @@ Zotero.defineProperty(Zotero.Feed, '_unreadCountSQL', {
 
 Zotero.defineProperty(Zotero.Feed, '_dbColumns', {
 	value: Object.freeze(['name', 'url', 'lastUpdate', 'lastCheck',
-		'lastCheckError', 'cleanupAfter', 'refreshInterval'])
+		'lastCheckError', 'cleanupUnreadAfter', 'cleanupReadAfter', 'refreshInterval'])
 });
 
 Zotero.defineProperty(Zotero.Feed, '_primaryDataSQLParts');
@@ -122,7 +124,7 @@ Zotero.defineProperty(Zotero.Feed.prototype, 'updating', {
 
 (function() {
 // Create accessors
-let accessors = ['name', 'url', 'refreshInterval', 'cleanupAfter'];
+let accessors = ['name', 'url', 'refreshInterval', 'cleanupUnreadAfter', 'cleanupReadAfter'];
 for (let i=0; i<accessors.length; i++) {
 	let name = accessors[i];
 	let prop = Zotero.Feed._colToProp(name);
@@ -183,12 +185,13 @@ Zotero.Feed.prototype._set = function (prop, val) {
 			this._previousURL = this.url;
 			break;
 		case '_feedRefreshInterval':
-		case '_feedCleanupAfter':
+		case '_feedCleanupReadAfter':
+		case '_feedCleanupUnreadAfter':
 			if (val === null) break;
 			
 			let newVal = Number.parseInt(val, 10);
 			if (newVal != val || !newVal || newVal <= 0) {
-				throw new Error(prop + " must be null or a positive integer");
+				throw new Error(`${prop} must be null or a positive integer, but is ${val}`);
 			}
 			break;
 		case '_feedLastCheckError':
@@ -198,7 +201,7 @@ Zotero.Feed.prototype._set = function (prop, val) {
 			}
 			
 			if (typeof val !== 'string') {
-				throw new Error(prop + " must be null or a string");
+				throw new Error(`${prop} must be null or a string, but is ${val}`);
 			}
 			break;
 	}
@@ -214,7 +217,8 @@ Zotero.Feed.prototype._loadDataFromRow = function(row) {
 	this._feedLastCheckError = row._feedLastCheckError || null;
 	this._feedLastCheck = row._feedLastCheck || null;
 	this._feedLastUpdate = row._feedLastUpdate || null;
-	this._feedCleanupAfter = parseInt(row._feedCleanupAfter) || null;
+	this._feedCleanupReadAfter = parseInt(row._feedCleanupReadAfter) || null;
+	this._feedCleanupUnreadAfter = parseInt(row._feedCleanupUnreadAfter) || null;
 	this._feedRefreshInterval = parseInt(row._feedRefreshInterval) || null;
 	this._feedUnreadCount = parseInt(row._feedUnreadCount);
 }
@@ -236,7 +240,8 @@ Zotero.Feed.prototype._initSave = Zotero.Promise.coroutine(function* (env) {
 	if (!this._feedName) throw new Error("Feed name not set");
 	if (!this._feedUrl) throw new Error("Feed URL not set");
 	if (!this.refreshInterval) this.refreshInterval = Zotero.Prefs.get('feeds.defaultTTL') * 60;
-	if (!this.cleanupAfter) this.cleanupAfter = Zotero.Prefs.get('feeds.defaultCleanupAfter');
+	if (!this.cleanupReadAfter) this.cleanupReadAfter = Zotero.Prefs.get('feeds.defaultCleanupReadAfter');
+	if (!this.cleanupUnreadAfter) this.cleanupUnreadAfter = Zotero.Prefs.get('feeds.defaultCleanupUnreadAfter');
 	
 	if (env.isNew) {
 		// Make sure URL is unique
@@ -291,7 +296,7 @@ Zotero.Feed.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 
 Zotero.Feed.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
 	let syncedDataChanged = 
-		['_feedName', '_feedCleanupAfter', '_feedRefreshInterval'].some((val) => this._changed[val]);
+		['_feedName', '_feedCleanupReadAfter', '_feedCleanupUnreadAfter', '_feedRefreshInterval'].some((val) => this._changed[val]);
 
 	yield Zotero.Feed._super.prototype._finalizeSave.apply(this, arguments);
 	
@@ -341,7 +346,7 @@ Zotero.Feed.prototype.erase = Zotero.Promise.coroutine(function* (options = {})
 
 Zotero.Feed.prototype.storeSyncedSettings = Zotero.Promise.coroutine(function* () {
 	let syncedFeeds = Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, 'feeds') || {};
-	syncedFeeds[this.url] = [this.name, this.cleanupAfter, this.refreshInterval];
+	syncedFeeds[this.url] = [this.name, this.cleanupReadAfter, this.cleanupUnreadAfter, this.refreshInterval];
 	return Zotero.SyncedSettings.set(Zotero.Libraries.userLibraryID, 'feeds', syncedFeeds);
 });
 
@@ -349,15 +354,17 @@ Zotero.Feed.prototype.getExpiredFeedItemIDs = Zotero.Promise.coroutine(function*
 	let sql = "SELECT itemID AS id FROM feedItems "
 		+ "LEFT JOIN items I USING (itemID) "
 		+ "WHERE I.libraryID=? "
-		+ "AND readTime IS NOT NULL "
-		+ "AND julianday('now', 'utc') - (julianday(readTime, 'utc') + ?) > 0";
-	return Zotero.DB.columnQueryAsync(sql, [this.id, {int: this.cleanupAfter}]);
+		+ "AND ("
+			+ "(readTime IS NOT NULL AND julianday('now', 'utc') - (julianday(readTime, 'utc') + ?) > 0) "
+			+ "OR (readTime IS NULL AND julianday('now', 'utc') - (julianday(dateModified, 'utc') + ?) > 0)"
+		+ ")";
+	return Zotero.DB.columnQueryAsync(sql, [this.id, {int: this.cleanupReadAfter}, {int: this.cleanupUnreadAfter}]);
 });
 
 /**
  * Clearing conditions for an item:
- * - Has been read at least feed.cleanupAfter earlier AND
- * - Does not exist in the RSS feed anymore
+ * - Has been read at least feed.cleanupReadAfter earlier OR is unread and older than feed.cleanupUnreadAfter
+ * - AND Does not exist in the RSS feed anymore
  * 
  * If we clear items once they've been read, we may potentially end up
  * with empty feeds for those that do not update very frequently.
@@ -366,24 +373,22 @@ Zotero.Feed.prototype.clearExpiredItems = Zotero.Promise.coroutine(function* (it
 	itemsInFeedIDs = itemsInFeedIDs || new Set();
 	try {
 		// Clear expired items
-		if (this.cleanupAfter) {
-			let expiredItems = yield this.getExpiredFeedItemIDs();
-			let toClear = expiredItems;
-			if (itemsInFeedIDs.size) {
-				toClear = [];
-				for (let id of expiredItems) {
-					if (!itemsInFeedIDs.has(id)) {
-						toClear.push(id);
-					}
+		let expiredItems = yield this.getExpiredFeedItemIDs();
+		let toClear = expiredItems;
+		if (itemsInFeedIDs.size) {
+			toClear = [];
+			for (let id of expiredItems) {
+				if (!itemsInFeedIDs.has(id)) {
+					toClear.push(id);
 				}
 			}
-			Zotero.debug("Clearing up read feed items...");
-			if (toClear.length) {
-				Zotero.debug(toClear.join(', '));
-				yield Zotero.FeedItems.erase(toClear);
-			} else {
-				Zotero.debug("No expired feed items");
-			}
+		}
+		Zotero.debug("Clearing up read feed items...");
+		if (toClear.length) {
+			Zotero.debug(toClear.join(', '));
+			yield Zotero.FeedItems.erase(toClear);
+		} else {
+			Zotero.debug("No expired feed items");
 		}
 	} catch(e) {
 		Zotero.debug("Error clearing expired feed items");
diff --git a/chrome/content/zotero/xpcom/data/feedItems.js b/chrome/content/zotero/xpcom/data/feedItems.js
index c6ba8dbef3..3771bbb58f 100644
--- a/chrome/content/zotero/xpcom/data/feedItems.js
+++ b/chrome/content/zotero/xpcom/data/feedItems.js
@@ -128,7 +128,6 @@ Zotero.FeedItems = new Proxy(function() {
 	});
 	
 	this.toggleReadByID = Zotero.Promise.coroutine(function* (ids, state) {
-		var feedsToUpdate = new Set();
 		if (!Array.isArray(ids)) {
 			if (typeof ids != 'string') throw new Error('ids must be a string or array in Zotero.FeedItems.toggleReadByID');
 			
@@ -146,20 +145,21 @@ Zotero.FeedItems = new Proxy(function() {
 				}
 			}
 		}
-		
-		
-		yield Zotero.DB.executeTransaction(function() {
-			for (let i=0; i<items.length; i++) {
-				items[i].isRead = state;
 
-				yield items[i].save();
-				let feed = Zotero.Feeds.get(items[i].libraryID);
-				feedsToUpdate.add(feed);
-			}
-		});
+		let feedsToUpdate = new Set();
+		let readTime = state ? Zotero.Date.dateToSQL(new Date(), true) : null;
+		for (let i=0; i<items.length; i++) {
+			items[i]._feedItemReadTime = readTime;
+
+			let feed = Zotero.Feeds.get(items[i].libraryID);
+			feedsToUpdate.add(feed);
+		}
+		
+		yield Zotero.DB.queryAsync(`UPDATE feedItems SET readTime=? WHERE itemID IN (${ids.join(', ')})`, readTime);
+		yield Zotero.Notifier.trigger('modify', 'item', ids, {});
 
 		for (let feed of feedsToUpdate) {
-			yield Zotero.Promise.all([feed.updateUnreadCount(), feed.storeSyncedSettings()]);
+			yield feed.updateUnreadCount();
 		}
 	});
 	
diff --git a/chrome/content/zotero/xpcom/data/feeds.js b/chrome/content/zotero/xpcom/data/feeds.js
index 19780edaef..10e0873a3c 100644
--- a/chrome/content/zotero/xpcom/data/feeds.js
+++ b/chrome/content/zotero/xpcom/data/feeds.js
@@ -164,8 +164,12 @@ Zotero.Feeds = new function() {
 			if (json[feed.url]) {
 				Zotero.debug("Feed " + feed.url + " exists remotely and locally");
 				feed.name = json[feed.url][0];
-				feed.cleanupAfter = json[feed.url][1];
-				feed.refreshInterval = json[feed.url][2];
+				feed.cleanupReadAfter = json[feed.url][1];
+				// TEMP after adding cleanupUnreadAfter for unread items
+				if (json[feed.url].length == 4) {
+					feed.cleanupUnreadAfter = json[feed.url][2];
+				}
+				feed.refreshInterval = json[feed.url][json[feed.url].length-1];
 				delete json[feed.url];
 			} else {
 				Zotero.debug("Feed " + feed.url + " does not exist in remote JSON. Deleting");
@@ -175,12 +179,17 @@ Zotero.Feeds = new function() {
 		// Because existing json[feed.url] got deleted, `json` now only contains new feeds
 		for (let url in json) {
 			Zotero.debug("Feed " + url + " exists remotely but not locally. Creating");
-			let feed = new Zotero.Feed({
+			let obj = {
 				url, 
 				name: json[url][0], 
-				cleanupAfter: json[url][1], 
-				refreshInterval: json[url[2]]
-			});
+				cleanupReadAfter: json[url][1], 
+				refreshInterval: json[url][json[url].length-1]
+			};
+			// TEMP after adding cleanupUnreadAfter for unread items
+			if (json[url].length == 4) {
+				obj.cleanupUnreadAfter = json[url][2];
+			}
+			let feed = new Zotero.Feed(obj);
 			yield feed.save();
 		}
 	});
@@ -292,7 +301,7 @@ Zotero.Feeds = new function() {
 			if(Array.isArray(json[url])) {
 				continue;
 			}
-			json[url] = [json[url].name, json[url].cleanupAfter, json[url].refreshInterval];
+			json[url] = [json[url].name, json[url].cleanupReadAfter, json[url].cleanupUnreadAfter, json[url].refreshInterval];
 		}
 		return json;
 	};
diff --git a/chrome/content/zotero/xpcom/schema.js b/chrome/content/zotero/xpcom/schema.js
index bdf6ff13e3..e04bbaf42e 100644
--- a/chrome/content/zotero/xpcom/schema.js
+++ b/chrome/content/zotero/xpcom/schema.js
@@ -34,7 +34,7 @@ Zotero.Schema = new function(){
 	var _dbVersions = [];
 	var _schemaVersions = [];
 	// Update when adding _updateCompatibility() line to schema update step
-	var _maxCompatibility = 3;
+	var _maxCompatibility = 4;
 	var _repositoryTimer;
 	var _remoteUpdateInProgress = false, _localUpdateInProgress = false;
 	
@@ -2356,6 +2356,14 @@ Zotero.Schema = new function(){
 					yield Zotero.DB.queryAsync("INSERT OR IGNORE INTO itemRelations VALUES (?, ?, ?)", [newSubjectID, predicateID, newObjectURI]);
 				}
 			}
+			
+			else if (i == 90) {
+				yield _updateCompatibility(4);
+				yield Zotero.DB.queryAsync("ALTER TABLE feeds RENAME TO feedsOld");
+				yield Zotero.DB.queryAsync("CREATE TABLE feeds (\n    libraryID INTEGER PRIMARY KEY,\n    name TEXT NOT NULL,\n    url TEXT NOT NULL UNIQUE,\n    lastUpdate TIMESTAMP,\n    lastCheck TIMESTAMP,\n    lastCheckError TEXT,\n    cleanupReadAfter INT,\n    cleanupUnreadAfter INT,\n    refreshInterval INT,\n    FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE\n)");
+				yield Zotero.DB.queryAsync("INSERT INTO feeds SELECT libraryID, name, url, lastUpdate, lastCheck, lastCheckError, 30, cleanupAfter, refreshInterval FROM feedsOld");
+				yield Zotero.DB.queryAsync("DROP TABLE feedsOld");
+			}
 		}
 		
 		yield _updateDBVersion('userdata', toVersion);
diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js
index d078b29924..ebf7bfc5d2 100644
--- a/chrome/content/zotero/zoteroPane.js
+++ b/chrome/content/zotero/zoteroPane.js
@@ -915,7 +915,8 @@ var ZoteroPane = new function()
 			feed.url = data.url;
 			feed.name = data.title;
 			feed.refreshInterval = data.ttl;
-			feed.cleanupAfter = data.cleanupAfter;
+			feed.cleanupReadAfter = data.cleanupReadAfter;
+			feed.cleanupUnreadAfter = data.cleanupUnreadAfter;
 			yield feed.saveTx();
 			yield feed.updateFeed();
 		}
@@ -930,7 +931,8 @@ var ZoteroPane = new function()
 			feed.url = data.url;
 			feed.name = data.title;
 			feed.refreshInterval = data.ttl;
-			feed.cleanupAfter = data.cleanupAfter;
+			feed.cleanupReadAfter = data.cleanupReadAfter;
+			feed.cleanupUnreadAfter = data.cleanupUnreadAfter;
 			yield feed.saveTx();
 			yield feed.updateFeed();
 		}
@@ -2066,7 +2068,8 @@ var ZoteroPane = new function()
 			url: feed.url,
 			title: feed.name,
 			ttl: feed.refreshInterval,
-			cleanupAfter: feed.cleanupAfter
+			cleanupReadAfter: feed.cleanupReadAfter,
+			cleanupUnreadAfter: feed.cleanupUnreadAfter
 		};
 		
 		window.openDialog('chrome://zotero/content/feedSettings.xul', 
@@ -2075,7 +2078,8 @@ var ZoteroPane = new function()
 		
 		feed.name = data.title;
 		feed.refreshInterval = data.ttl;
-		feed.cleanupAfter = data.cleanupAfter;
+		feed.cleanupReadAfter = data.cleanupReadAfter;
+		feed.cleanupUnreadAfter = data.cleanupUnreadAfter;
 		yield feed.saveTx();
 	});
 	
diff --git a/chrome/locale/en-US/zotero/zotero.dtd b/chrome/locale/en-US/zotero/zotero.dtd
index f9d90b4b4d..0b15084565 100644
--- a/chrome/locale/en-US/zotero/zotero.dtd
+++ b/chrome/locale/en-US/zotero/zotero.dtd
@@ -272,8 +272,10 @@
 <!ENTITY zotero.feedSettings.title.label                "Title:">
 <!ENTITY zotero.feedSettings.refresh.label1             "Update feed every">
 <!ENTITY zotero.feedSettings.refresh.label2             "hour(s)">
-<!ENTITY zotero.feedSettings.cleanupAfter.label1         "Remove read articles after ">
-<!ENTITY zotero.feedSettings.cleanupAfter.label2         "day(s)">
+<!ENTITY zotero.feedSettings.cleanupUnreadAfter.label1         "Remove unread feed items after ">
+<!ENTITY zotero.feedSettings.cleanupUnreadAfter.label2         "day(s)">
+<!ENTITY zotero.feedSettings.cleanupReadAfter.label1         "Remove read feed items after ">
+<!ENTITY zotero.feedSettings.cleanupReadAfter.label2         "day(s)">
 
 
 <!ENTITY zotero.recognizePDF.recognizing.label			"Retrieving Metadata…">
diff --git a/defaults/preferences/zotero.js b/defaults/preferences/zotero.js
index 01f2972c4e..375a586c8a 100644
--- a/defaults/preferences/zotero.js
+++ b/defaults/preferences/zotero.js
@@ -54,7 +54,8 @@ pref("extensions.zotero.groups.copyTags", true);
 
 pref("extensions.zotero.feeds.sortAscending", false);
 pref("extensions.zotero.feeds.defaultTTL", 1);
-pref("extensions.zotero.feeds.defaultCleanupAfter", 2);
+pref("extensions.zotero.feeds.defaultCleanupReadAfter", 3);
+pref("extensions.zotero.feeds.defaultCleanupUnreadAfter", 30);
 
 pref("extensions.zotero.backup.numBackups", 2);
 pref("extensions.zotero.backup.interval", 1440);
diff --git a/resource/schema/userdata.sql b/resource/schema/userdata.sql
index 0749e3c293..7e171224c4 100644
--- a/resource/schema/userdata.sql
+++ b/resource/schema/userdata.sql
@@ -1,4 +1,4 @@
--- 89
+-- 90
 
 -- Copyright (c) 2009 Center for History and New Media
 --                    George Mason University, Fairfax, Virginia, USA
@@ -203,7 +203,8 @@ CREATE TABLE feeds (
     lastUpdate TIMESTAMP,
     lastCheck TIMESTAMP,
     lastCheckError TEXT,
-    cleanupAfter INT,
+    cleanupReadAfter INT,
+    cleanupUnreadAfter INT,
     refreshInterval INT,
     FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE
 );
diff --git a/test/content/support.js b/test/content/support.js
index 592d9787be..9b13a5d847 100644
--- a/test/content/support.js
+++ b/test/content/support.js
@@ -321,7 +321,8 @@ var createFeed = Zotero.Promise.coroutine(function* (props = {}) {
 	feed.description = props.description || "";
 	feed.url = props.url || 'http://www.' + Zotero.Utilities.randomString() + '.com/feed.rss';
 	feed.refreshInterval = props.refreshInterval || 12;
-	feed.cleanupAfter = props.cleanupAfter || 2;
+	feed.cleanupReadAfter = props.cleanupReadAfter || 2;
+	feed.cleanupUnreadAfter = props.cleanupUnreadAfter || 30;
 	yield feed.saveTx();
 	return feed;
 });
diff --git a/test/tests/feedTest.js b/test/tests/feedTest.js
index cce50346b6..516e39f111 100644
--- a/test/tests/feedTest.js
+++ b/test/tests/feedTest.js
@@ -75,7 +75,8 @@ describe("Zotero.Feed", function() {
 				name: 'Test ' + Zotero.randomString(),
 				url: 'http://' + Zotero.randomString() + '.com/',
 				refreshInterval: 30,
-				cleanupAfter: 1
+				cleanupReadAfter: 1,
+				cleanupUnreadAfter: 30
 			};
 			
 			let feed = yield createFeed(props);
@@ -83,7 +84,8 @@ describe("Zotero.Feed", function() {
 			assert.equal(feed.name, props.name, "name is correct");
 			assert.equal(feed.url.toLowerCase(), props.url.toLowerCase(), "url is correct");
 			assert.equal(feed.refreshInterval, props.refreshInterval, "refreshInterval is correct");
-			assert.equal(feed.cleanupAfter, props.cleanupAfter, "cleanupAfter is correct");
+			assert.equal(feed.cleanupReadAfter, props.cleanupReadAfter, "cleanupReadAfter is correct");
+			assert.equal(feed.cleanupUnreadAfter, props.cleanupUnreadAfter, "cleanupUnreadAfter is correct");
 			
 			assert.isNull(feed.lastCheck, "lastCheck is null");
 			assert.isNull(feed.lastUpdate, "lastUpdate is null");
@@ -159,7 +161,7 @@ describe("Zotero.Feed", function() {
 			assert.notOk(syncedFeeds[oldUrl]);
 			assert.ok(syncedFeeds[feed.url]);
 		});
-		it('should update syncedSettings if `name`, `url`, `refreshInterval` or `cleanupAfter` was modified', function* () {
+		it('should update syncedSettings if `name`, `url`, `refreshInterval` or `cleanupUnreadAfter` was modified', function* () {
 			let feed = yield createFeed();
 			let syncedSetting = Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, 'feeds');
 			yield Zotero.SyncedSettings.set(Zotero.Libraries.userLibraryID, 'feeds', syncedSetting, 0, true);
@@ -168,7 +170,7 @@ describe("Zotero.Feed", function() {
 			yield feed.saveTx();
 			assert.isFalse(Zotero.SyncedSettings.getMetadata(Zotero.Libraries.userLibraryID, 'feeds').synced)
 		});
-		it('should not update syncedSettings if `name`, `url`, `refreshInterval` or `cleanupAfter` were not modified', function* () {
+		it('should not update syncedSettings if `name`, `url`, `refreshInterval` or `cleanupUnreadAfter` were not modified', function* () {
 			let feed = yield createFeed();
 			let syncedSetting = Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, 'feeds');
 			yield Zotero.SyncedSettings.set(Zotero.Libraries.userLibraryID, 'feeds', syncedSetting, 0, true);
@@ -220,12 +222,13 @@ describe("Zotero.Feed", function() {
 	describe("#storeSyncedSettings", function() {
 		it("should store settings for feed in compact format", function* () {
 			let url = 'http://' + Zotero.Utilities.randomString().toLowerCase() + '.com/feed.rss';
-			let settings = [Zotero.Utilities.randomString(), 1, 1];
+			let settings = [Zotero.Utilities.randomString(), 1, 30, 1];
 			let feed = yield createFeed({
 				url,
 				name: settings[0],
-				cleanupAfter: settings[1],
-				refreshInterval: settings[2]
+				cleanupReadAfter: settings[1],
+				cleanupUnreadAfter: settings[2],
+				refreshInterval: settings[3]
 			});
 			
 			let syncedFeeds = Zotero.SyncedSettings.get(Zotero.Libraries.userLibraryID, 'feeds');
@@ -234,17 +237,25 @@ describe("Zotero.Feed", function() {
 	});
 	
 	describe("#clearExpiredItems()", function() {
-		var feed, expiredFeedItem, readFeedItem, feedItem, readStillInFeed, feedItemIDs;
+		var feed, readExpiredFI, unreadExpiredFI, readFeedItem, feedItem, readStillInFeed, feedItemIDs;
 		
 		before(function* (){
-			feed = yield createFeed({cleanupAfter: 1});
+			feed = yield createFeed({cleanupReadAfter: 1, cleanupUnreadAfter: 3});
 			
-			expiredFeedItem = yield createDataObject('feedItem', { libraryID: feed.libraryID });
+			readExpiredFI = yield createDataObject('feedItem', { libraryID: feed.libraryID });
 			// Read 2 days ago
-			expiredFeedItem.isRead = true;
-			expiredFeedItem._feedItemReadTime = Zotero.Date.dateToSQL(
+			readExpiredFI.isRead = true;
+			readExpiredFI._feedItemReadTime = Zotero.Date.dateToSQL(
 					new Date(Date.now() - 2 * 24*60*60*1000), true);
-			yield expiredFeedItem.saveTx();
+			yield readExpiredFI.saveTx();
+
+			// Added 5 days ago
+			unreadExpiredFI = yield createDataObject('feedItem', { 
+				libraryID: feed.libraryID,
+				dateAdded: Zotero.Date.dateToSQL(new Date(Date.now() - 5 * 24*60*60*1000), true),
+				dateModified: Zotero.Date.dateToSQL(new Date(Date.now() - 5 * 24*60*60*1000), true)
+			});
+			yield unreadExpiredFI.saveTx();
 			
 			readStillInFeed = yield createDataObject('feedItem', { libraryID: feed.libraryID });
 			// Read 2 days ago
@@ -263,7 +274,7 @@ describe("Zotero.Feed", function() {
 			
 			assert.include(feedItemIDs, feedItem.id, "feed contains unread feed item");
 			assert.include(feedItemIDs, readFeedItem.id, "feed contains read feed item");
-			assert.include(feedItemIDs, expiredFeedItem.id, "feed contains expired feed item");
+			assert.include(feedItemIDs, readExpiredFI.id, "feed contains expired feed item");
 			assert.include(feedItemIDs, readStillInFeed.id, "feed contains expired but still in rss feed item");
 			
 			yield feed.clearExpiredItems(new Set([readStillInFeed.id]));
@@ -272,7 +283,8 @@ describe("Zotero.Feed", function() {
 		});
 	
 		it('should clear expired items', function() {
-			assert.notInclude(feedItemIDs, expiredFeedItem.id, "feed no longer contain expired feed item");	
+			assert.notInclude(feedItemIDs, readExpiredFI.id, "feed no longer contains expired read feed item");
+			assert.notInclude(feedItemIDs, unreadExpiredFI.id, "feed no longer contains expired feed item");	
 		});
 		
 		it('should not clear read items that have not expired yet', function() {
diff --git a/test/tests/feedsTest.js b/test/tests/feedsTest.js
index fcc1ceab3a..837fcd262f 100644
--- a/test/tests/feedsTest.js
+++ b/test/tests/feedsTest.js
@@ -56,7 +56,8 @@ describe("Zotero.Feeds", function () {
 					url,
 					name: Zotero.Utilities.randomString(),
 					refreshInterval: 5,
-					cleanupAfter: 3,
+					cleanupReadAfter: 3,
+					cleanupUnreadAfter: 30,
 				};
 				if (i == 0) {
 					existingFeedURL = url;