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

RESOLVED FIXED in Firefox 3 alpha4

Status

()

P2
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Trunk
Firefox 3 alpha4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Duplicating (pasting/dropping) a bookmark item does not copy over its annotations.
Priority: -- → P2
Target Milestone: Firefox 3 alpha3 → Firefox 3 alpha4
same goes for moving a folder.
Created attachment 260081 [details] [diff] [review]
patch
Attachment #260080 - Attachment is obsolete: true
Attachment #260081 - Flags: review?(dietrich)
Attachment #260080 - Flags: review?(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/
Attachment #260081 - Flags: review?(dietrich) → review+
Comment on attachment 260081 [details] [diff] [review]
patch

taking back review. adding live-bookmark causes an error and an overly large dialog (screenshot forthcoming).
Attachment #260081 - Flags: review+ → review-
Created attachment 260148 [details]
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 on attachment 260081 [details] [diff] [review]
patch

sorry, problem is not related to this patch, my mistake.
Attachment #260081 - Flags: review- → review+
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
Created attachment 260171 [details] [diff] [review]
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
Attachment #260169 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Created attachment 260191 [details] [diff] [review]
followup

sorry, my earlier review comment was not clear: the comment and the function parameter both needed to be updated.
Attachment #260191 - Flags: review?(mano)
Comment on attachment 260191 [details] [diff] [review]
followup

*oops*
Attachment #260191 - Flags: review?(mano) → review+
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
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.