373504 - Duplicating (pasting/dropping) a bookmark item does not copy over its annotations

Status: RESOLVED FIXED

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

Description

[Asaf Romano (gone)]

Duplicating (pasting/dropping) a bookmark item does not copy over its annotations.

Comment 1

[Asaf Romano (gone)]

same goes for moving a folder.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 260081 [details] [diff] [review]
patch

just nits, r=me

>   /**
>-   * Get a create-item transaction for the item added in the dialog
>+   * [New Item Mode] Get the insertion point for the item given
>+   * dialog state and opening arguments.
>    */
>-  _getCreateItemTransaction: function() {
>-    NS_ASSERT(this._action != ACTION_EDIT,
>-              "_getCreateItemTransaction called when editing an item");
>-
>+  _getInsertionPointDetails: function BPP__getInsertionPointDetails() {
>     var containerId, indexInContainer = -1;
>     if (isElementVisible(this._folderMenuList))
>       containerId = this._getFolderIdFromMenuList();
>     else {
>       containerId = this._defaultInsertionPoint.folderId;
>       indexInContainer = this._defaultInsertionPoint.index;
>     }
> 
>-    if (this._itemType == BOOKMARK_ITEM) {
>-      var uri = PlacesUtils._uri(this._element("editURLBar").value);
>-      NS_ASSERT(uri, "cannot create an item without a uri");
>-      return new
>-        PlacesCreateItemTransaction(uri, containerId, indexInContainer);
>-    }
>-    else if (this._itemType == LIVEMARK_CONTAINER) {
>-      var feedURIString = this._element("feedLocationTextfield").value;
>-      var feedURI = PlacesUtils._uri(feedURIString);
>+    return [containerId, indexInContainer];
>+  },

nit: can you document this return type in the method's description above?

>+
>+  _getLoadInSidebarAnnotation:
>+  function BPP__getLoadInSidebarAnnotation(aLoadInSidebar) {

nit: can you add jsdoc descriptions for this and the other new methods?

>+  _getCreateNewLivemarkTransaction:
>+  function BPP__getCreateNewLivemarkTransaction() {

nit: for code-navigability, can you move this up by the other getCreateNew*Transaction methods?

> /**
>- * Create a new Folder
>+ * Transaction for creating a new folder item.
>+ *
>+ * @param aName
>+ *        the name of the new folder
>+ * @param aContainer
>+ *        the identifier of the folder in which the new folder should be
>+ *        added.
>+ * @param [optional] aIndex
>+ *        the index of the item in aContainer, pass -1 or nothing to create
>+ *        the item at the end of aContainer.
>+ * @param [optional] aAnnotations
>+ *        the annotations to set for the new folder.
>+ * @param [optional] aChildItemsTransaction
>+ *        array of transactions for items to be created under the new folder.
>  */

can you pluralize aChildItemsTransaction as you did on the property, and on PlacesCreateItemTransaction?

>   doTransaction: function PIST_doTransaction() {
>   undoTransaction: function PIST_undoTransaction() {

nit, and not related to your patch, but since you're there: these should be PCST now.

> /**
>- * Create a new live bookmark
>+ * Transaction for creating a new live-bookmark item.
>+ *
>+ * @see nsILivemarksService::createLivemark for documentation regrading the
>+ * first three arguments.

s/regrading/regarding/

> 
>   /**
>    * Opens the bookmark properties panel for a given bookmark idnetifier.

since you're cleaning, s/idnetifier/identifier/

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 260081 [details] [diff] [review]
patch

taking back review. adding live-bookmark causes an error and an overly large dialog (screenshot forthcoming).

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: screenshot - big dialog

and the error that accompanied it:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderTitle]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/bookmarkProperties.js :: BPP__appendFolderItemToMenuList :: line 374"  data: no]

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 260081 [details] [diff] [review]
patch

sorry, problem is not related to this patch, my mistake.

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 260169 [details] [diff] [review]
patch

>Index: browser/components/places/content/bookmarkProperties.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v
>retrieving revision 1.40
>diff -u -p -8 -r1.40 bookmarkProperties.js
>--- browser/components/places/content/bookmarkProperties.js	28 Mar 2007 20:32:51 -0000	1.40
>+++ browser/components/places/content/bookmarkProperties.js	30 Mar 2007 20:15:43 -0000
>@@ -42,33 +42,33 @@
>  * as window.arguments[0]. The object must have the following fields set:
>  *   @ action (String). Possible values:
>  *     - "add" - for adding a new item.
>  *       @ type (String). Possible values:
>  *         - "bookmark"
>  *           @ loadBookmarkInSidebar - optional, the default state for the
>  *             "Load this bookmark in the sidebar" field.
>  *         - "folder"
>- *         - "folder with items"
>- *           @ URIList (Array of nsIURI objects)- list of uris to be bookmarked
>- *             under the new folder.
>+ *            @ URIList (Array of nsIURI objects) - optional, list of uris to
>+ *              be bookmarked under the new folder.

nit: extra space of indentation?

>+  /**
>+   * Returns an object which could then be used to set/unset the
>+   * description annotation for an item (any type).
>+   *
>+   * @parma aDescription
>+   *        The description of the item.

nit: s/parma/param/

>+   * @parma aLoadInSidebar

ditto

Comment 10

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/browser/components/places/content/bookmarkProperties.js 1.41
mozilla/browser/components/places/content/controller.js 1.140
mozilla/browser/components/places/content/utils.js 1.28

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: followup

sorry, my earlier review comment was not clear: the comment and the function parameter both needed to be updated.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 260191 [details] [diff] [review]
followup

*oops*

Comment 13

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.141; previous revision: 1.140
done

Comment 14

[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