Closed Bug 393870 Opened 13 years ago Closed 13 years ago

unable to sort by tags in the bookmark organizer

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: moco, Assigned: mano)

References

Details

Attachments

(1 file, 1 obsolete file)

unable to sort by tags in the bookmark organizer

currently, clicking on the tags column or using the View | Sort By Tags menu will do nothing.

see bug #387748 and bug #393868 for some background.
Flags: blocking-firefox3?
Go To BookMarks on toolbar and select  organize bookmarks....it will open popup window BookMarks Manager...
In that window go to View and select sort options...
and close it refresh the window .....
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Priority: -- → P5
I encountered this during beta 1 testing. Tag seems to be the only item in the list that you cannot sort, either by clicking on the tags column or View | Sort.
Go to Bookmarks-->Organize Bookmarks..it will show pop up window 
from that window select one bookmark and right click on that it will show the options and select the sort by name
Sorting in that column will be a semi-helpful thing at best, as there could be 1..N tags for any given entry.

Still, I think we should do the minimum expected here, which would be to list the tags in alphabetical order, and then do ascending/descending sorts.

So if I were to tag an entry as "foo, bar, zab", it would show up as

Page Title   http://page.url.here/page   bar, foo, zab
We already do that.
Assignee: nobody → mano
Target Milestone: Firefox 3 Mx → Firefox 3 M10
Well, then, all we need to do is have the "Tags" column use that as the basis for an alphabetic sort, such that:

 bar
 bar, foo
 bar, foo, zab 
 bar, zab
 bar, zab, zzz

and we're pretty much done!
Priority: P5 → P3
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Attached patch re-implement the tags column (obsolete) — Splinter Review
* Fix live-update of the tags column (this wasn't implemented at all)
* Cache it contents, we were calling getTagsForURI very often apparently.
* Make QUERY_UPDATE_COMPLEX_WITH_BOOKMARKS meaningful, *doh*.
* Allow sorting by tags.
Attachment #293460 - Flags: review?(sspitzer)
Attachment #293460 - Attachment description: re-implement tags column → re-implement the tags column
Attachment #293460 - Attachment is patch: true
Attachment #293460 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 293460 [details] [diff] [review]
re-implement the tags column

>Index: toolkit/components/places/public/nsINavHistoryService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v
>retrieving revision 1.72
>diff -u -p -8 -r1.72 nsINavHistoryService.idl
>--- toolkit/components/places/public/nsINavHistoryService.idl	20 Nov 2007 02:01:56 -0000	1.72
>+++ toolkit/components/places/public/nsINavHistoryService.idl	17 Dec 2007 03:06:03 -0000

>@@ -177,16 +177,22 @@ interface nsINavHistoryResultNode : nsIS
>    */
>   readonly attribute PRTime dateAdded;
> 
>   /**
>    * If the node is an item (bookmark, folder or a separator) this value is the 
>    * time that the item was last modified. For other nodes, this value is 0.
>    */
>   readonly attribute PRTime lastModified;
>+
>+  /**
>+   * For uri nodes, this is a sorted list of the tags for the uri represented
>+   * by this node. Otherwise this is an empty string.
>+   */
>+  readonly attribute AString tags;
> };

is this delimited somehow? please add to the comment.

>@@ -995,18 +1001,20 @@ interface nsINavHistoryQueryOptions : ns
>   const unsigned short SORT_BY_KEYWORD_ASCENDING = 9;
>   const unsigned short SORT_BY_KEYWORD_DESCENDING = 10;
>   const unsigned short SORT_BY_DATEADDED_ASCENDING = 11;
>   const unsigned short SORT_BY_DATEADDED_DESCENDING = 12;
>   const unsigned short SORT_BY_LASTMODIFIED_ASCENDING = 13;
>   const unsigned short SORT_BY_LASTMODIFIED_DESCENDING = 14;
>   const unsigned short SORT_BY_COUNT_ASCENDING = 15;
>   const unsigned short SORT_BY_COUNT_DESCENDING = 16;
>-  const unsigned short SORT_BY_ANNOTATION_ASCENDING = 17;
>-  const unsigned short SORT_BY_ANNOTATION_DESCENDING = 18;
>+  const unsigned short SORT_BY_TAGS_ASCENDING = 17;
>+  const unsigned short SORT_BY_TAGS_DESCENDING = 18;
>+  const unsigned short SORT_BY_ANNOTATION_ASCENDING = 19;
>+  const unsigned short SORT_BY_ANNOTATION_DESCENDING = 20;

is there a reason why you're changing the anno const values? please don't if it's not necessary.


>Index: toolkit/components/places/src/nsNavBookmarks.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v
>retrieving revision 1.136
>diff -u -p -8 -r1.136 nsNavBookmarks.cpp
>--- toolkit/components/places/src/nsNavBookmarks.cpp	11 Dec 2007 09:01:33 -0000	1.136
>+++ toolkit/components/places/src/nsNavBookmarks.cpp	17 Dec 2007 03:06:08 -0000
>@@ -918,28 +918,51 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
>   rv = transaction.Commit();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   AddBookmarkToHash(childID, 0);
> 
>   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>                       OnItemAdded(rowId, aFolder, index))
> 
>+  // If the bookmark has been added to a tag container, notify all
>+  // bookmark-folder result nodes which contain a bookmark for the new
>+  // bookmark's url
>+  PRInt64 grandParent;
>+  rv = GetFolderIdForItem(aFolder, &grandParent);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (grandParent == mTagRoot) {

iirc this is duplicated elsewhere. if so, should abstract this out into a method.

>+    // query for all bookmarks for that URI, notify for each
>+    nsTArray<PRInt64> bookmarks;
>+
>+    rv = GetBookmarkIdsForURITArray(aItem, &bookmarks);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (bookmarks.Length()) {
>+      for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
>+        ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                            OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
>+                                          PR_FALSE, EmptyCString()))
>+      }
>+    }
>+  }

any reason to not put the tag name as the value here, and make the name more descriptive of what's occurring? if it was "URIWasTagged" and had the tag values, observers could update w/o having to re-query the db.

also, "tags" should be a define somewhere. hm, would be good to consolidate all the possible OnItemChanged values in a set of defines somewhere.

>@@ -976,16 +1004,41 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = UpdateBookmarkHashOnRemove(placeId);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>                       OnItemRemoved(aItemId, folderId, childIndex))
> 
>+  if (itemType == TYPE_BOOKMARK) {
>+    // If the removed bookmark was a child of a tag container, notify all
>+    // bookmark-folder result nodes which contain a bookmark for the removed
>+    // bookmark's url.
>+    PRInt64 grandParent;
>+    rv = GetFolderIdForItem(folderId, &grandParent);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (grandParent == mTagRoot) {
>+      nsCOMPtr<nsIURI> uri;
>+      rv = NS_NewURI(getter_AddRefs(uri), spec);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      nsTArray<PRInt64> bookmarks;
>+
>+      rv = GetBookmarkIdsForURITArray(uri, &bookmarks);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      if (bookmarks.Length()) {
>+        for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
>+          ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>+                              OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
>+                                            PR_FALSE, EmptyCString()))
>+        }
>+      }

ditto from above comment when adding a bookmark

to here.
> 
> any reason to not put the tag name as the value here, and make the name more
> descriptive of what's occurring? if it was "URIWasTagged" and had the tag
> values, observers could update w/o having to re-query the db.
> 

I see what you're doing, disregard this comment.
Comment on attachment 293460 [details] [diff] [review]
re-implement the tags column

r=me w/ previous comments fixed
Attachment #293460 - Flags: review?(dietrich) → review+
> >@@ -995,18 +1001,20 @@ interface nsINavHistoryQueryOptions : ns
> >   const unsigned short SORT_BY_KEYWORD_ASCENDING = 9;
> >   const unsigned short SORT_BY_KEYWORD_DESCENDING = 10;
> >   const unsigned short SORT_BY_DATEADDED_ASCENDING = 11;
> >   const unsigned short SORT_BY_DATEADDED_DESCENDING = 12;
> >   const unsigned short SORT_BY_LASTMODIFIED_ASCENDING = 13;
> >   const unsigned short SORT_BY_LASTMODIFIED_DESCENDING = 14;
> >   const unsigned short SORT_BY_COUNT_ASCENDING = 15;
> >   const unsigned short SORT_BY_COUNT_DESCENDING = 16;
> >-  const unsigned short SORT_BY_ANNOTATION_ASCENDING = 17;
> >-  const unsigned short SORT_BY_ANNOTATION_DESCENDING = 18;
> >+  const unsigned short SORT_BY_TAGS_ASCENDING = 17;
> >+  const unsigned short SORT_BY_TAGS_DESCENDING = 18;
> >+  const unsigned short SORT_BY_ANNOTATION_ASCENDING = 19;
> >+  const unsigned short SORT_BY_ANNOTATION_DESCENDING = 20;
> 
> is there a reason why you're changing the anno const values? please don't if
> it's not necessary.

some places code relies on SORT_BY_ANNOTATION_DESCENDING being the max valid value for sortingMode.

> 
> 
> >Index: toolkit/components/places/src/nsNavBookmarks.cpp
> >===================================================================
> >RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v
> >retrieving revision 1.136
> >diff -u -p -8 -r1.136 nsNavBookmarks.cpp
> >--- toolkit/components/places/src/nsNavBookmarks.cpp	11 Dec 2007 09:01:33 -0000	1.136
> >+++ toolkit/components/places/src/nsNavBookmarks.cpp	17 Dec 2007 03:06:08 -0000
> >@@ -918,28 +918,51 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
> >   rv = transaction.Commit();
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   AddBookmarkToHash(childID, 0);
> > 
> >   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
> >                       OnItemAdded(rowId, aFolder, index))
> > 
> >+  // If the bookmark has been added to a tag container, notify all
> >+  // bookmark-folder result nodes which contain a bookmark for the new
> >+  // bookmark's url
> >+  PRInt64 grandParent;
> >+  rv = GetFolderIdForItem(aFolder, &grandParent);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  if (grandParent == mTagRoot) {
> 
> iirc this is duplicated elsewhere. if so, should abstract this out into a
> method.
> 

not in a re-usable manner, IIRC.
Comment on attachment 293460 [details] [diff] [review]
re-implement the tags column

r=sspitzer, but a few comments:

1)

+  rv = svc->GetTagsForURI(uri, &count, &tags);

we should error check rv here.

2)

+  if (count > 0) {
+    for (PRUint32 i=0; i < count; i++) {
+      mTags.Append(tags[i]);
+      if (i < count -1) { // separate with commas
+        mTags.Append(NS_LITERAL_STRING(", "));
+      }
+    }
+    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, tags);
+  }
+  aTags.Assign(mTags);

aren't we leaking tags when count is zero?  it will be an empty array, but still allocated.

3) can you write some tests?
Attachment #293460 - Flags: review?(sspitzer) → review+
Flags: in-testsuite?
> aren't we leaking tags when count is zero?  it will be an empty array, but
> still allocated.

as mano explained over irc, tags will be null when count is zero, so this code is correct as is.
Attached patch as checked inSplinter Review
I'll write some tests later.
mozilla/browser/components/places/content/editBookmarkOverlay.js 1.15
mozilla/browser/components/places/content/treeView.js 1.26
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.51
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.73
mozilla/toolkit/components/places/public/nsITaggingService.idl 1.9
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.137
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.217
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.124
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.50
mozilla/toolkit/components/places/src/nsTaggingService.js 1.7
mozilla/toolkit/components/places/tests/bookmarks/test_395593.js 1.2
mozilla/toolkit/components/places/tests/bookmarks/test_savedsearches.js 1.2
mozilla/toolkit/components/places/tests/unit/test_annotations.js 1.12
Attachment #293460 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
this checkin turned the tree orange, and locally, when I run the tests I am crashing:

2007-12-20 13:09:28 -0800

EXC_BAD_ACCESS (0x0001)
KERN_PROTECTION_FAILURE (0x0002) at 0x00000001

Thread 0 Crashed:
0    nsNavHistoryContainerResultNode::GetContainerOpen(int*) + 15 (nsNavHistoryResult.cpp:423)
1    nsNavHistoryFolderResultNode::GetContainerOpen(int*) + 24 (nsNavHistoryResult.h:757)
2    nsPlacesImportExportService::WriteContainerContents(nsINavHistoryResultNode*, nsACString const&, nsIOutputStream*) + 451 (nsPlacesImportExportService.cpp:2138)
3    nsPlacesImportExportService::ExportHTMLToFile(nsILocalFile*) + 3791 (nsPlacesImportExportService.cpp:2412)
4    NS_InvokeByIndex_P + 98 (xptcinvoke_unixish_x86.cpp:179)
5    XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 6226 (xpcwrappednative.cpp:2342)
6    XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*, long*) + 398 (xpcwrappednativejsops.cpp:1480)
7    js_Invoke + 3063 (jsinterp.c:1023)
8    js_Interpret + 67212 (jsinterp.c:3837)
9    js_Execute + 994 (jsinterp.c:1265)


js: /Users/sspitzer/Desktop/trunk/mozilla/tools/test-harness/xpcshell-simple/test_all.sh: line 129: 25721 Bus error               NATIVE_TOPSRCDIR="$native_topsrcdir" TOPSRCDIR="$topsrcdir" $xpcshell -s $headfiles -f $t $tailfiles 2>$t.log 1>&2
FAIL
../../../../_tests/xpcshell-simple/test_browser_places/unit/test_bookmarks_html.js.log:
>>>>>>>
*** test pending

<<<<<<<
../../../../_tests/xpcshell-simple/test_browser_places/unit/test_placesTxn.js: FAIL
../../../../_tests/xpcshell-simple/test_browser_places/unit/test_placesTxn.js.log:
>>>>>>>
*** test pending
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/sspitzer/Desktop/trunk/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp, line 931
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.insertBookmark]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///Users/sspitzer/Desktop/trunk/debug-build/dist/bin/components/nsPlacesTransactionsService.js :: PCIT_doTransaction :: line 338"  data: no]
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122704 Minefield/3.0b3pre. 
Status: RESOLVED → VERIFIED
Maybe someone should update the example at http://developer.mozilla.org/en/docs/Places:Tagging_Service - since the example for 'Getting all tags associated with a URL' does not work anymore after this checkin.
(In reply to comment #18)
> Maybe someone should update the example at
> http://developer.mozilla.org/en/docs/Places:Tagging_Service - since the example
> for 'Getting all tags associated with a URL' does not work anymore after this
> checkin.
> 

done
we have tests for library sorting now.
Flags: in-testsuite? → in-testsuite+
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.