320261 - Add bookmark separators

Status: RESOLVED FIXED

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

Description

[Brett Wilson]

Also, be sure to fix nsBookmarksHTML.cpp to set them on import.

Comment 1

[Brian Ryner (not reading)]

Note to self: we decided to encode separators as rows in the bookmarks table where folder_child and item_child are both null (but the index is set).

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

A couple of notes:
* Separators are represented as rows in the bookmark table with folder_child and item_child both NULL.
* Separators are exposed in the API using just the parent and position.  There is no other data associated with a separator.

Comment 3

[Brett Wilson]

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

>+NS_IMETHODIMP
>+nsNavBookmarks::RemoveChildAt(PRInt64 aParent, PRInt32 aIndex)
>+{

This function is a little weird because you take care to not send remove separator notifications until you commit, but call remove item and remove folder subroutines (which will send their own notifications) from inside the transaction. It would probably be better if we could remove all notifications from transactions when possible.

>@@ -818,8 +965,17 @@ nsNavBookmarks::RemoveFolderChildren(PRI
>+  // Remove separators. Sort the array so we can enumerate it backwards
>+  // and not bother updating indices due to item removal.
>+  separatorChildren.Sort();

I don't understand this, won't it already be sorted because you enumerated items in order by position?

>+NS_IMETHODIMP
>+nsNavHistoryQueryResultNode::OnSeparatorAdded(PRInt64 aParent, PRInt32 aIndex)
>+{
>+  if (mLiveUpdate == QUERYUPDATE_COMPLEX_WITH_BOOKMARKS)
>+    return Refresh();
>+  return NS_OK;
>+}
>+NS_IMETHODIMP
>+nsNavHistoryQueryResultNode::OnSeparatorRemoved(PRInt64 aParent,
>+                                                PRInt32 aIndex)
>+{
>+  if (mLiveUpdate == QUERYUPDATE_COMPLEX_WITH_BOOKMARKS)
>+    return Refresh();
>+  return NS_OK;
>+}

I don't think you need to do anything in these functions other than return OK. A query with bookmarks won't ever contain separators since that is a query over history items that are bookmarked.


> // nsNavHistoryFolderResultNode ************************************************
> //
>@@ -2958,6 +2972,46 @@ nsNavHistoryFolderResultNode::OnFolderCh
>   return NS_OK;
> }
> 
>+// nsNavHistoryFolderResultNode::OnSeparatorAdded (nsINavBookmarkObserver)
>+
>+NS_IMETHODIMP
>+nsNavHistoryFolderResultNode::OnSeparatorAdded(PRInt64 aParent, PRInt32 aIndex)
>+{
>+  NS_ASSERTION(aParent == mFolderId, "Got wrong bookmark update");
>+  if (! StartIncrementalUpdate())
>+    return NS_OK;
>+
>+  nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>+  NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>+
>+  nsNavHistoryResultNode* node = new nsNavHistorySeparatorResultNode();
>+  NS_ENSURE_TRUE(node, NS_ERROR_OUT_OF_MEMORY);
>+
>+  return InsertChildAt(node, aIndex);
>+}

This doesn't handle sorting. You will need to check to see if the sorting is NONE (= natural order for bookmarks) and then you can do this. If sorting is something else, you probably don't need to do anything.

I think sorting bookmark folders should remove separators. I would make nsNavHistoryContainerResultNode::RecursiveSort virtual and override it for folder containers to first find all separators and remove them and then call through to the base implementation. Can you also remove the reference in the current implementation to being static? I just noticed that and it's no longer true.


>+// nsNavHistoryFolderResultNode::OnSeparatorRemoved (nsINavBookmarkObserver)
>+
>+NS_IMETHODIMP
>+nsNavHistoryFolderResultNode::OnSeparatorRemoved(PRInt64 aParent,
>+                                                 PRInt32 aIndex)
>+{
>+  NS_ASSERTION(aParent == mFolderId, "Got wrong bookmark update");
>+  if (! StartIncrementalUpdate())
>+    return NS_OK;
>+
>+  // XXX is this index always correct?
>+  return RemoveChildAt(aIndex);
>+}

No. Check sorting first as above. Then I think you should also check that the index is in bounds and the item below it is a separator node. If not, assertnotreached and return (so even if this happens in release, it won't do anything).

>+// nsNavHistoryResult::OnSeparatorAdded (nsINavBookmarkObserver)
>+
>+NS_IMETHODIMP
>+nsNavHistoryResult::OnSeparatorAdded(PRInt64 aParent, PRInt32 aIndex)
>+{
>+  ENUMERATE_BOOKMARK_OBSERVERS_FOR_FOLDER(aParent,
>+      OnSeparatorAdded(aParent, aIndex));
>+  ENUMERATE_HISTORY_OBSERVERS(OnSeparatorAdded(aParent, aIndex));
>+  return NS_OK;
>+}
>+
>+
>+// nsNavHistoryResult::OnSeparatorRemoved (nsINavBookmarkObserver)
>+
>+NS_IMETHODIMP
>+nsNavHistoryResult::OnSeparatorRemoved(PRInt64 aParent, PRInt32 aIndex)
>+{
>+  ENUMERATE_BOOKMARK_OBSERVERS_FOR_FOLDER(aParent,
>+      OnSeparatorRemoved(aParent, aIndex));
>+  ENUMERATE_HISTORY_OBSERVERS(OnSeparatorRemoved(aParent, aIndex));
>+  return NS_OK;
>+}

You don't need to enumerate history observers in this case since non-folder nodes can't have separators. You should probably put a note in to that effect.

Comment 4

[Ben Goodger (CC only: Do not assign bugs to this!)]

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

>+  undoTransaction: function PIST_undoTransaction() {
>+    LOG("UNCreate separator from: " + this._container + "," + this._index);
>+    this._bms.removeChildAt(this._index);
>+  },

This method (removeChildAt) is defined as taking two parameters. Is this a bug? Does undo/redo of an insert work?

Can you attach the patch that you're going to check in though?

Comment 5

[Brian Ryner (not reading)]

Attachment: revised patch

This is the patch to check in.  I fixed Brett and Ben's comments, as well as a couple of additional bugs in RemoveChildAt (binding parameter 0 twice instead of binding parameter 1; adjusting indices before the actual remove resulting in two items being removed and a hole being left).

Comment 6

[Brian Ryner (not reading)]

checked in (trunk and branch)

Comment 7

[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