Closed
Bug 405938
Opened 15 years ago
Closed 15 years ago
problems when import/exporting of saved searches
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3
People
(Reporter: moco, Assigned: dietrich)
References
Details
(Keywords: dataloss, Whiteboard: [has patch][has reviews])
Attachments
(1 file, 1 obsolete file)
18.72 KB,
patch
|
asaf
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
problems when import/exporting of saved searches (including the "places" folder in the personal toolbar) here's the problem: you might have a bookmark to place:folder=<x>&.. on, export, we do not write out folder item ids, but we do write out place: uris (for saved searches) that include folder items. what we could do is write out item ids on export. on import, we won't use them for item ids (we need to create new ids as we create new folders), but we can keep a map of exported ids to new ids, and then fix up place queries. so, item ids are internally consistent, within a bookmarks.html file (or .json).
Flags: blocking-firefox3?
Reporter | ||
Comment 1•15 years ago
|
||
this is pretty critical, and should be noticable on backup/restore, for example.
Priority: -- → P1
Updated•15 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P1 → P2
Target Milestone: --- → Firefox 3 M11
Assignee | ||
Updated•15 years ago
|
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Assignee | ||
Comment 2•15 years ago
|
||
folder ids need to internally consistent in both the json and html import/export.
Assignee: nobody → dietrich
Updated•15 years ago
|
Target Milestone: Firefox 3 beta4 → Firefox 3
Assignee | ||
Comment 3•15 years ago
|
||
bug 399264 will safeguard our default searches, queries, etc. However, it doesn't safeguard user-created searches of user-created folders.
Depends on: 399264
Assignee | ||
Updated•15 years ago
|
Whiteboard: [swag: 2d]
Assignee | ||
Updated•15 years ago
|
Summary: problems when import/exporting of saved searches (including the "places" folder in the personal toolbar) → problems when import/exporting of saved searches
Updated•15 years ago
|
Whiteboard: [swag: 2d] → [swag: 2d][need status update]
Assignee | ||
Comment 4•15 years ago
|
||
- keep a map of old->new folder ids when restoring - resolve the restored place URIs after restore is done this is just for backup/restore. i'll file a separate bug HTML import/export.
Attachment #315842 -
Flags: review?(mano)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [swag: 2d][need status update] → [has patch][needs review mano]
Comment 5•15 years ago
|
||
Comment on attachment 315842 [details] [diff] [review] fix v1 >Index: toolkit/components/places/src/nsNavHistoryResult.cpp >=================================================================== >@@ -3304,25 +3304,27 @@ nsNavHistoryFolderResultNode::ReindexRan > node->mBookmarkIndex <= aEndIndex) > node->mBookmarkIndex += aDelta; > } > } > > > // nsNavHistoryFolderResultNode::FindChildById > // >-// Searches this folder for a node with the given URI. Returns null if not >+// Searches this folder for a node with the given id. Returns null if not > // found. Does not addref the node! > > nsNavHistoryResultNode* > nsNavHistoryFolderResultNode::FindChildById(PRInt64 aItemId, > PRUint32* aNodeIndex) > { > for (PRInt32 i = 0; i < mChildren.Count(); i ++) { >- if (mChildren[i]->mItemId == aItemId) { >+ if (mChildren[i]->mItemId == aItemId || >+ (mChildren[i]->IsFolder() && >+ mChildren[i]->GetAsFolder()->mQueryItemId == aItemId)) { I don't understand this change. It looks to me like it may break live update of items under folder shortcuts. Why is this part of this patch anyay? >Index: toolkit/components/places/src/utils.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v >retrieving revision 1.16 >diff -u -8 -p -r1.16 utils.js >--- toolkit/components/places/src/utils.js 15 Apr 2008 17:17:35 -0000 1.16 >+++ toolkit/components/places/src/utils.js 15 Apr 2008 21:41:04 -0000 >@@ -1037,18 +1037,22 @@ var PlacesUtils = { > > // ensure tag folder gets processed last > nodes[0].children.sort(function sortRoots(aNode, bNode) { > return (aNode.root && aNode.root == "tagsFolder") ? 1 : > (bNode.root && bNode.root == "tagsFolder") ? -1 : 0; > }); > > var batch = { >+ _utils: PlacesUtils, I'm pretty sure that could be |_utils: this| (or self, or whatever is right in the context _outside_ of batch). >+ >+ // fixup imported place: uris that contain folders >+ if (searchIds.length && folderIdMap.length) { >+ searchIds.forEach(function(aId) { I don't think you need the scoping if block. >+ var oldURI = this._utils.bookmarks.getBookmarkURI(aId); >+ var uri = this._utils.fixupQuery(this._utils.bookmarks.getBookmarkURI(aId), >+ folderIdMap); >+ this._utils.bookmarks.changeBookmarkURI(aId, uri); we should do that only if the uri has been changed (use equal()) > /** > * Takes a JSON-serialized node and inserts it into the db. > * > * @param aData > * The unwrapped data blob of dropped or pasted data. > * @param aContainer > * The container the data was dropped or pasted into > * @param aIndex > * The index within the container the item was dropped or pasted at >+ * @returns An array containing of maps of old folder ids to new folder ids, mega nit: s/An/an/ >+ }, >+ >+ /** >+ * Replace imported folder ids with their local counterparts in a place: URI. replaces >+ * >+ * @param aURI >+ * A place: URI with folder ids. >+ * @param aFolderIdMap >+ * An array mapping old folder id to new folder ids. >+ * @returns URI >+ * Returns the fixed up URI if all matched. If some matched, it returns nit: this should be just @returns he fixed up URI... >+ * the URI with only the matching folders included. If none matched it >+ * returns the input URI unchanged. >+ */ >+ fixupQuery: function PU_fixupQuery(aQueryURI, aFolderIdMap) { make that _fixupQuert >+ var queries = {}; >+ var options = {}; >+ this.history.queryStringToQueries(aQueryURI.spec, queries, {}, options); >+ var query = queries.value[0]; what about multiple queries? > self.bookmarks.getItemType(aJSNode.id) == self.bookmarks.TYPE_BOOKMARK) { > aJSNode.type = self.TYPE_X_MOZ_PLACE; > aJSNode.uri = aPlacesNode.uri; >- aJSNode.concreteId = PlacesUtils.getConcreteItemId(aPlacesNode); Make sure we don't assume it's there elsewhere. >Index: toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js >=================================================================== >+ * >+ * The Initial Developer of the Original Code is Mozilla Corp. >+ * Portions created by the Initial Developer are Copyright (C) 2008 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Dietrich Ayala <dietrich@mozilla.com> (Original Author) :)
Attachment #315842 -
Flags: review?(mano) → review-
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #315842 -
Attachment is obsolete: true
Attachment #316950 -
Flags: review?(mano)
Assignee | ||
Comment 7•15 years ago
|
||
all comments fixed
Comment 8•15 years ago
|
||
Comment on attachment 316950 [details] [diff] [review] fix v2 r=mano
Attachment #316950 -
Flags: review?(mano) → review+
Updated•15 years ago
|
Whiteboard: [has patch][needs review mano] → [has patch][has reviews]
Assignee | ||
Comment 9•15 years ago
|
||
Comment on attachment 316950 [details] [diff] [review] fix v2 drivers: this fixes backup and restore to preserve saved-searches of user-created folders. has tests.
Attachment #316950 -
Flags: approval1.9?
Comment 10•15 years ago
|
||
Comment on attachment 316950 [details] [diff] [review] fix v2 a=mconnor on behalf of 1.9 drivers
Attachment #316950 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 11•15 years ago
|
||
Checking in toolkit/components/places/src/utils.js; /cvsroot/mozilla/toolkit/components/places/src/utils.js,v <-- utils.js new revision: 1.19; previous revision: 1.18 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js,v done Checking in toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js; /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js,v <-- test_405938_restore_queries.js initial revision: 1.1 done
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite+
Comment 12•15 years ago
|
||
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre. I verified by creating several saved searches, backing up all bookmark data, removing all bookmark data, and then restoring.
Status: RESOLVED → VERIFIED
Comment 13•13 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•