diff --git a/browser/components/places/content/editBookmarkOverlay.js b/browser/components/places/content/editBookmarkOverlay.js index 4509d84a8197..9070d2f95dd4 100644 --- a/browser/components/places/content/editBookmarkOverlay.js +++ b/browser/components/places/content/editBookmarkOverlay.js @@ -130,7 +130,7 @@ let gEditItemOverlay = { let newKeyword = aNewKeyword; if (newKeyword === undefined) { let itemId = this._paneInfo.itemId; - newKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId); + this._keyword = newKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId); } this._initTextField(this._keywordField, newKeyword); }), @@ -597,12 +597,16 @@ let gEditItemOverlay = { let itemId = this._paneInfo.itemId; let newKeyword = this._keywordField.value; if (!PlacesUIUtils.useAsyncTransactions) { - let txn = new PlacesEditBookmarkKeywordTransaction(itemId, newKeyword, this._paneInfo.postData); + let txn = new PlacesEditBookmarkKeywordTransaction(itemId, newKeyword, + this._paneInfo.postData, + this._keyword); PlacesUtils.transactionManager.doTransaction(txn); return; } let guid = this._paneInfo.itemGuid; - PlacesTransactions.EditKeyword({ guid, keyword: newKeyword }) + PlacesTransactions.EditKeyword({ guid, keyword: newKeyword, + postData: this._paneInfo.postData, + oldKeyword: this._keyword }) .transact().catch(Components.utils.reportError); }, diff --git a/toolkit/components/places/PlacesTransactions.jsm b/toolkit/components/places/PlacesTransactions.jsm index 0cb22bcdc6d6..546028372774 100644 --- a/toolkit/components/places/PlacesTransactions.jsm +++ b/toolkit/components/places/PlacesTransactions.jsm @@ -883,7 +883,7 @@ DefineTransaction.defineInputProps(["guid", "parentGuid", "newParentGuid"], DefineTransaction.guidValidate); DefineTransaction.defineInputProps(["title"], DefineTransaction.strOrNullValidate, null); -DefineTransaction.defineInputProps(["keyword", "postData", "tag", +DefineTransaction.defineInputProps(["keyword", "oldKeyword", "postData", "tag", "excludingAnnotation"], DefineTransaction.strValidate, ""); DefineTransaction.defineInputProps(["index", "newIndex"], @@ -1335,14 +1335,30 @@ PT.Annotate.prototype = { * * Required Input Properties: guid, keyword. */ -PT.EditKeyword = DefineTransaction(["guid", "keyword"]); +PT.EditKeyword = DefineTransaction(["guid", "keyword"], + ["postData", "oldKeyword"]); PT.EditKeyword.prototype = Object.seal({ - execute: function* (aGuid, aKeyword) { - let itemId = yield PlacesUtils.promiseItemId(aGuid), - oldKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId); - PlacesUtils.bookmarks.setKeywordForBookmark(itemId, aKeyword); - this.undo = () => { - PlacesUtils.bookmarks.setKeywordForBookmark(itemId, oldKeyword); + execute: function* (aGuid, aKeyword, aPostData, aOldKeyword) { + let url; + let oldEntry; + if (aOldKeyword) { + oldEntry = yield PlacesUtils.keywords.fetch(aOldKeyword); + url = oldEntry.url; + yield PlacesUtils.keywords.remove(aOldKeyword); + } + + if (!url) + url = (yield PlacesUtils.bookmarks.fetch(aGuid)).url; + yield PlacesUtils.keywords.insert({ + url: url, + keyword: aKeyword, + postData: aPostData || (oldEntry ? oldEntry.postData : "") + }); + + this.undo = function* () { + yield PlacesUtils.keywords.remove(aKeyword); + if (oldEntry) + yield PlacesUtils.keywords.insert(oldEntry); }; } }); diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index 7fccc1b3cf80..7e113f30403c 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -1864,7 +1864,8 @@ this.PlacesUtils = { }.bind(this); const QUERY_STR = - `WITH RECURSIVE + `/* do not warn (bug no): cannot use an index */ + WITH RECURSIVE descendants(fk, level, type, id, guid, parent, parentGuid, position, title, dateAdded, lastModified) AS ( SELECT b1.fk, 0, b1.type, b1.id, b1.guid, b1.parent, @@ -2349,29 +2350,49 @@ XPCOMUtils.defineLazyGetter(this, "gKeywordsCachePromise", () => }).catch(Cu.reportError); }, - onItemChanged(id, prop, isAnno, val, lastMod, itemType, parentId, guid) { - if (gIgnoreKeywordNotifications || - prop != "keyword") + onItemChanged(id, prop, isAnno, val, lastMod, itemType, parentId, guid, + parentGuid, oldVal) { + if (gIgnoreKeywordNotifications) + return; + if (prop == "keyword") { + this._onKeywordChanged(guid, val).catch(Cu.reportError); + } else if (prop == "uri") { + this._onUrlChanged(guid, val, oldVal).catch(Cu.reportError); + } + }, + + _onKeywordChanged: Task.async(function* (guid, keyword) { + let bookmark = yield PlacesUtils.bookmarks.fetch(guid); + // By this time the bookmark could be gone, there's nothing we can do. + if (!bookmark) return; - Task.spawn(function* () { - let bookmark = yield PlacesUtils.bookmarks.fetch(guid); - // By this time the bookmark could have gone, there's nothing we can do. - if (!bookmark) - return; - - if (val.length == 0) { - // We are removing a keyword. - let keywords = keywordsForHref(bookmark.url.href) - for (let keyword of keywords) { - cache.delete(keyword); - } - } else { - // We are adding a new keyword. - cache.set(val, { keyword: val, url: bookmark.url }); + if (keyword.length == 0) { + // We are removing a keyword. + let keywords = keywordsForHref(bookmark.url.href) + for (let kw of keywords) { + cache.delete(kw); } - }).catch(Cu.reportError); - } + } else { + // We are adding a new keyword. + cache.set(keyword, { keyword, url: bookmark.url }); + } + }), + + _onUrlChanged: Task.async(function* (guid, url, oldUrl) { + // Check if the old url is associated with keywords. + let entries = []; + yield PlacesUtils.keywords.fetch({ url: oldUrl }, e => entries.push(e)); + if (entries.length == 0) + return; + + // Move the keywords to the new url. + for (let entry of entries) { + yield PlacesUtils.keywords.remove(entry.keyword); + entry.url = new URL(url); + yield PlacesUtils.keywords.insert(entry); + } + }), }; PlacesUtils.bookmarks.addObserver(observer, false); @@ -3388,14 +3409,18 @@ PlacesSetPageAnnotationTransaction.prototype = { * new keyword for the bookmark * @param aNewPostData [optional] * new keyword's POST data, if available + * @param aOldKeyword [optional] + * old keyword of the bookmark * * @return nsITransaction object */ this.PlacesEditBookmarkKeywordTransaction = - function PlacesEditBookmarkKeywordTransaction(aItemId, aNewKeyword, aNewPostData) -{ + function PlacesEditBookmarkKeywordTransaction(aItemId, aNewKeyword, + aNewPostData, aOldKeyword) { this.item = new TransactionItemCache(); this.item.id = aItemId; + this.item.keyword = aOldKeyword; + this.item.href = (PlacesUtils.bookmarks.getBookmarkURI(aItemId)).spec; this.new = new TransactionItemCache(); this.new.keyword = aNewKeyword; this.new.postData = aNewPostData @@ -3406,22 +3431,51 @@ PlacesEditBookmarkKeywordTransaction.prototype = { doTransaction: function EBKTXN_doTransaction() { - // Store the current values. - this.item.keyword = PlacesUtils.bookmarks.getKeywordForBookmark(this.item.id); - if (this.item.keyword) - this.item.postData = PlacesUtils.getPostDataForBookmark(this.item.id); + let done = false; + Task.spawn(function* () { + if (this.item.keyword) { + let oldEntry = yield PlacesUtils.keywords.fetch(this.item.keyword); + this.item.postData = oldEntry.postData; + yield PlacesUtils.keywords.remove(this.item.keyword); + } - // Update the keyword. - PlacesUtils.bookmarks.setKeywordForBookmark(this.item.id, this.new.keyword); - if (this.new.keyword && this.new.postData) - PlacesUtils.setPostDataForBookmark(this.item.id, this.new.postData); + yield PlacesUtils.keywords.insert({ + url: this.item.href, + keyword: this.new.keyword, + postData: this.new.postData || this.item.postData + }); + }.bind(this)).catch(Cu.reportError) + .then(() => done = true); + // TODO: This hack is needed until we can use PlacesTransactions.jsm. + let thread = Services.tm.currentThread; + while (!done) { + thread.processNextEvent(true); + } }, undoTransaction: function EBKTXN_undoTransaction() { - PlacesUtils.bookmarks.setKeywordForBookmark(this.item.id, this.item.keyword); - if (this.item.postData) - PlacesUtils.setPostDataForBookmark(this.item.id, this.item.postData); + + let done = false; + Task.spawn(function* () { + if (this.new.keyword) { + yield PlacesUtils.keywords.remove(this.new.keyword); + } + + if (this.item.keyword) { + yield PlacesUtils.keywords.insert({ + url: this.item.href, + keyword: this.item.keyword, + postData: this.item.postData + }); + } + }.bind(this)).catch(Cu.reportError) + .then(() => done = true); + // TODO: This hack is needed until we can use PlacesTransactions.jsm. + let thread = Services.tm.currentThread; + while (!done) { + thread.processNextEvent(true); + } } }; diff --git a/toolkit/components/places/tests/unit/test_async_transactions.js b/toolkit/components/places/tests/unit/test_async_transactions.js index de59152de63e..8b58233c1652 100644 --- a/toolkit/components/places/tests/unit/test_async_transactions.js +++ b/toolkit/components/places/tests/unit/test_async_transactions.js @@ -1021,16 +1021,24 @@ add_task(function* test_edit_keyword() { bm_info.guid = yield PT.NewBookmark(bm_info).transact(); observer.reset(); - yield PT.EditKeyword({ guid: bm_info.guid, keyword: KEYWORD }).transact(); + yield PT.EditKeyword({ guid: bm_info.guid, keyword: KEYWORD, postData: "postData" }).transact(); ensureKeywordChange(KEYWORD); + let entry = yield PlacesUtils.keywords.fetch(KEYWORD); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData"); observer.reset(); yield PT.undo(); ensureKeywordChange(); + entry = yield PlacesUtils.keywords.fetch(KEYWORD); + Assert.equal(entry, null); observer.reset(); yield PT.redo(); ensureKeywordChange(KEYWORD); + entry = yield PlacesUtils.keywords.fetch(KEYWORD); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData"); // Cleanup observer.reset(); @@ -1043,6 +1051,68 @@ add_task(function* test_edit_keyword() { ensureUndoState(); }); +add_task(function* test_edit_specific_keyword() { + let bm_info = { parentGuid: rootGuid + , url: NetUtil.newURI("http://test.edit.keyword") }; + bm_info.guid = yield PT.NewBookmark(bm_info).transact(); + function ensureKeywordChange(aCurrentKeyword = "", aPreviousKeyword = "") { + ensureItemsChanged({ guid: bm_info.guid + , property: "keyword" + , newValue: aCurrentKeyword + }); + } + + yield PlacesUtils.keywords.insert({ keyword: "kw1", url: bm_info.url.spec, postData: "postData1" }); + yield PlacesUtils.keywords.insert({ keyword: "kw2", url: bm_info.url.spec, postData: "postData2" }); + bm_info.guid = yield PT.NewBookmark(bm_info).transact(); + + observer.reset(); + yield PT.EditKeyword({ guid: bm_info.guid, keyword: "keyword", oldKeyword: "kw2" }).transact(); + ensureKeywordChange("keyword", "kw2"); + let entry = yield PlacesUtils.keywords.fetch("kw1"); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData1"); + entry = yield PlacesUtils.keywords.fetch("keyword"); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData2"); + entry = yield PlacesUtils.keywords.fetch("kw2"); + Assert.equal(entry, null); + + observer.reset(); + yield PT.undo(); + ensureKeywordChange("kw2", "keyword"); + entry = yield PlacesUtils.keywords.fetch("kw1"); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData1"); + entry = yield PlacesUtils.keywords.fetch("kw2"); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData2"); + entry = yield PlacesUtils.keywords.fetch("keyword"); + Assert.equal(entry, null); + + observer.reset(); + yield PT.redo(); + ensureKeywordChange("keyword", "kw2"); + entry = yield PlacesUtils.keywords.fetch("kw1"); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData1"); + entry = yield PlacesUtils.keywords.fetch("keyword"); + Assert.equal(entry.url.href, bm_info.url.spec); + Assert.equal(entry.postData, "postData2"); + entry = yield PlacesUtils.keywords.fetch("kw2"); + Assert.equal(entry, null); + + // Cleanup + observer.reset(); + yield PT.undo(); + ensureKeywordChange("kw2"); + yield PT.undo(); + ensureItemsRemoved(bm_info); + + yield PT.clearTransactionsHistory(); + ensureUndoState(); +}); + add_task(function* test_tag_uri() { // This also tests passing uri specs. let bm_info_a = { url: "http://bookmarked.uri" diff --git a/toolkit/components/places/tests/unit/test_keywords.js b/toolkit/components/places/tests/unit/test_keywords.js index 55a726f12b7d..df7216f3a60d 100644 --- a/toolkit/components/places/tests/unit/test_keywords.js +++ b/toolkit/components/places/tests/unit/test_keywords.js @@ -513,6 +513,24 @@ add_task(function* test_oldKeywordsAPI() { check_no_orphans(); }); -function run_test() { - run_next_test(); -} +add_task(function* test_bookmarkURLChange() { + let fc1 = yield foreign_count("http://example1.com/"); + let fc2 = yield foreign_count("http://example2.com/"); + let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example1.com/", + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + yield PlacesUtils.keywords.insert({ keyword: "keyword", + url: "http://example1.com/" }); + + yield check_keyword(true, "http://example1.com/", "keyword"); + Assert.equal((yield foreign_count("http://example1.com/")), fc1 + 2); // +1 bookmark +1 keyword + + yield PlacesUtils.bookmarks.update({ guid: bookmark.guid, + url: "http://example2.com/"}); + yield promiseKeyword("keyword", "http://example2.com/"); + + yield check_keyword(false, "http://example1.com/", "keyword"); + yield check_keyword(true, "http://example2.com/", "keyword"); + Assert.equal((yield foreign_count("http://example1.com/")), fc1); // -1 bookmark -1 keyword + Assert.equal((yield foreign_count("http://example2.com/")), fc2 + 2); // +1 bookmark +1 keyword +}); diff --git a/toolkit/components/places/tests/unit/test_placesTxn.js b/toolkit/components/places/tests/unit/test_placesTxn.js index 63866dec6c7d..b19f5c170929 100644 --- a/toolkit/components/places/tests/unit/test_placesTxn.js +++ b/toolkit/components/places/tests/unit/test_placesTxn.js @@ -555,6 +555,40 @@ add_task(function* test_edit_keyword() { do_check_eq(PlacesUtils.getPostDataForBookmark(testBkmId), null); }); +add_task(function* test_edit_specific_keyword() { + const KEYWORD = "keyword-test_edit_keyword2"; + + let testURI = NetUtil.newURI("http://test_edit_keyword2.com"); + let testBkmId = bmsvc.insertBookmark(root, testURI, bmsvc.DEFAULT_INDEX, "Test edit keyword"); + // Add multiple keyword to this uri. + yield PlacesUtils.keywords.insert({ keyword: "kw1", url: testURI.spec, postData: "postData1" }); + yield PlacesUtils.keywords.insert({keyword: "kw2", url: testURI.spec, postData: "postData2" }); + + // Try to change only kw2. + let txn = new PlacesEditBookmarkKeywordTransaction(testBkmId, KEYWORD, "postData2", "kw2"); + + txn.doTransaction(); + do_check_eq(observer._itemChangedId, testBkmId); + do_check_eq(observer._itemChangedProperty, "keyword"); + do_check_eq(observer._itemChangedValue, KEYWORD); + let entry = yield PlacesUtils.keywords.fetch("kw1"); + Assert.equal(entry.url.href, testURI.spec); + Assert.equal(entry.postData, "postData1"); + yield promiseKeyword(KEYWORD, testURI.spec, "postData2"); + yield promiseKeyword("kw2", null); + + txn.undoTransaction(); + do_check_eq(observer._itemChangedId, testBkmId); + do_check_eq(observer._itemChangedProperty, "keyword"); + do_check_eq(observer._itemChangedValue, "kw2"); + do_check_eq(PlacesUtils.getPostDataForBookmark(testBkmId), "postData1"); + entry = yield PlacesUtils.keywords.fetch("kw1"); + Assert.equal(entry.url.href, testURI.spec); + Assert.equal(entry.postData, "postData1"); + yield promiseKeyword("kw2", testURI.spec, "postData2"); + yield promiseKeyword("keyword", null); +}); + add_task(function* test_LoadInSidebar_transaction() { let testURI = NetUtil.newURI("http://test_LoadInSidebar_transaction.com"); let testBkmId = bmsvc.insertBookmark(root, testURI, bmsvc.DEFAULT_INDEX, "Test LoadInSidebar transaction");