393498 - on cut, then undo of a bookmark, we lose the dateAdded and lastModified values

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Still reproduces in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122704 Minefield/3.0b3pre.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: fix

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.

Comment 4

[Asaf Romano (gone)]

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.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: fix, per comment #4, with test

Comment 6

[Asaf Romano (gone)]

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.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: add idl documentation of the new values

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: all comments addressed, added live update tests

Comment 9

[Asaf Romano (gone)]

Comment on attachment 310040 [details] [diff] [review]
all comments addressed, added live update tests

reviewed over IRC.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: don't use notification data

Comment 11

[Asaf Romano (gone)]

Comment on attachment 310062 [details] [diff] [review]
don't use notification data

r=mano

Comment 12

[Dietrich Ayala (:dietrich)]

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

Comment 13

[Tuukka Tolvanen (sp3000)]

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

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> warning: unused variable ‘now’

bug 423677

Comment 15

[Tracy Walker [:tracy]]

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

Comment 16

[Gervase Markham [:gerv]]

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