Closed
Bug 393498
Opened 17 years ago
Closed 17 years ago
on cut, then undo of a bookmark, we lose the dateAdded and lastModified values
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta5
People
(Reporter: moco, Assigned: dietrich)
References
Details
(Keywords: dataloss)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
on cut, then undo of a bookmark, we lose the dateAdded and lastModified values
1) in the bm organizer dialog, select a bookmark with a dateAdded and lastModified values
2) cut
3) undo (or paste)
we lose the dateAdded and lastModified values.
note, at first, dateAdded is blank (until you switch to another folder and back)
it appears that dateAdded is now the "new" date added.
we should preserve these values on undo and paste.
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 M10
Assignee | ||
Updated•17 years ago
|
Whiteboard: [patch in bug 384370]
Assignee | ||
Updated•17 years ago
|
OS: Windows XP → All
Hardware: PC → All
Assignee | ||
Updated•17 years ago
|
Priority: -- → P3
Updated•17 years ago
|
Priority: P3 → P2
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dietrich
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Updated•17 years ago
|
Priority: P2 → P1
Assignee | ||
Updated•17 years ago
|
Whiteboard: [patch in bug 384370]
Comment 2•17 years ago
|
||
Still reproduces in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122704 Minefield/3.0b3pre.
Assignee | ||
Updated•17 years ago
|
Whiteboard: [patch in bug 384370]
Assignee | ||
Updated•17 years ago
|
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Updated•17 years ago
|
Priority: P1 → P2
Target Milestone: Firefox 3 beta4 → Firefox 3
Assignee | ||
Updated•17 years ago
|
Whiteboard: [patch in bug 384370]
Assignee | ||
Comment 3•17 years ago
|
||
this wasn't fixed by 384370, just copy and paste was.
this patch ensures added/modified times are retained upon removal and subsequent undo of an item from the front end. it also allows those values to reach the front end (lack of item id in the query caused RowToResultNode to not copy those values to the result node).
this solves the dataloss aspect of this bug, and part of the UI updating.
however, even after this patch, the UI will not be updated if only undo-ing a cut of a few items. since the transaction count is less than MIN_TRANSACTIONS_FOR_BATCH in nsPlacesTransactionsService, these changes are wrapped up in a single transaction when cut/undo-ing of a single bookmark for example. therefore there's no batching, and the front-end receives and processes OnItemAdded before the date changes occur. we don't send OnItemChanged for added/modified because mostly those values are changed as a result of other changes that will send a specific notification.
the workaround is to cause the UI to reload the container (by clicking another folder, and then returning, for example). you'll then see the restored dates.
one option might be to add aDoNotify to SetDateAdded/SetLastModified, however, this is pretty clunky and serves only this unique case. another might be to just get the date values directly from the bookmark service in treeView.js, but given the quantity of UI updating in the treeview (every mouse move over the cell, etc), that's likely going to cause performance issues.
i'll file a separate followup bug for resolving the UI updating issue.
Attachment #309850 -
Flags: review?(mano)
Comment 4•17 years ago
|
||
I don't think you should add that parameter to the API. I would rather make the _exposed_ versions of these methods always notify the front-end, and use some SetItem*Internally methods within the back-end.
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #309850 -
Attachment is obsolete: true
Attachment #309870 -
Flags: review?(mano)
Attachment #309850 -
Flags: review?(mano)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano]
Comment 6•17 years ago
|
||
Comment on attachment 309870 [details] [diff] [review]
fix, per comment #4, with test
you need to update
http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryResult.cpp#3391
I'm pretty sure the documentation for the observer interface has details about itemChanged's possible value types, please update it.
Attachment #309870 -
Flags: review?(mano) → review-
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano] → [needs new patch]
Assignee | ||
Comment 7•17 years ago
|
||
Attachment #309870 -
Attachment is obsolete: true
Attachment #309875 -
Flags: review?(mano)
Assignee | ||
Updated•17 years ago
|
Attachment #309875 -
Flags: review?(mano)
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #309875 -
Attachment is obsolete: true
Attachment #310040 -
Flags: review?(mano)
Comment 9•17 years ago
|
||
Comment on attachment 310040 [details] [diff] [review]
all comments addressed, added live update tests
reviewed over IRC.
Attachment #310040 -
Flags: review?(mano) → review-
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #310040 -
Attachment is obsolete: true
Attachment #310062 -
Flags: review?(mano)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs new patch] → [has patch][needs review mano]
Comment 11•17 years ago
|
||
Comment on attachment 310062 [details] [diff] [review]
don't use notification data
r=mano
Attachment #310062 -
Flags: review?(mano) → review+
Assignee | ||
Comment 12•17 years ago
|
||
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v <-- nsPlacesTransactionsService.js
new revision: 1.33; previous revision: 1.32
done
Checking in toolkit/components/places/public/nsINavBookmarksService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v <-- nsINavBookmarksService.idl
new revision: 1.54; previous revision: 1.53
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavBookmarks.cpp
new revision: 1.156; previous revision: 1.155
done
Checking in toolkit/components/places/src/nsNavBookmarks.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v <-- nsNavBookmarks.h
new revision: 1.53; previous revision: 1.52
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp
new revision: 1.273; previous revision: 1.272
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- nsNavHistoryResult.cpp
new revision: 1.137; previous revision: 1.136
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_393498.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_393498.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_393498.js,v <-- test_393498.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano]
Target Milestone: Firefox 3 → Firefox 3 beta5
Comment 13•17 years ago
|
||
In member function ‘virtual nsresult nsNavBookmarks::MoveItem(PRInt64, PRInt64, PRInt32)’:
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavBookmarks.cpp#1676
warning: unused variable ‘now’
PRTime now = PR_Now();
- rv = SetItemLastModified(oldParent, now);
+ rv = SetItemDateInternal(mDBSetItemLastModified, oldParent, PR_Now());
NS_ENSURE_SUCCESS(rv, rv);
- rv = SetItemLastModified(aNewParent, now);
+ rv = SetItemDateInternal(mDBSetItemLastModified, aNewParent, PR_Now());
Comment 14•17 years ago
|
||
Comment 15•17 years ago
|
||
verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032606
Firefox/3.0b5
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-GB; rv:1.9b5)
Gecko/2008032604 Firefox/3.0b5
Status: RESOLVED → VERIFIED
Comment 16•15 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
•