(not reading, please use seth@sspitzer.org instead) Reporter
	Description • 17 years ago

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.

	santhosh
	Comment 1 • 17 years ago

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 .....

	Marcia Knous [:marcia]
	Comment 2 • 17 years ago

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.

	santhosh
	Comment 3 • 17 years ago

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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 4 • 17 years ago

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

	Asaf Romano (gone) Assignee
	Comment 5 • 17 years ago

We already do that.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 6 • 17 years ago

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!

	Asaf Romano (gone) Assignee
	Comment 7 • 17 years ago

* 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.

	Dietrich Ayala (:dietrich)
	Comment 8 • 17 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 9 • 17 years ago

> 
> 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.

	Dietrich Ayala (:dietrich)
	Comment 10 • 17 years ago

Comment on attachment 293460 [details] [diff] [review]
re-implement the tags column

r=me w/ previous comments fixed

	Asaf Romano (gone) Assignee
	Comment 11 • 17 years ago

> >@@ -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.

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 12 • 17 years ago

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?

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 13 • 17 years ago

> 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.

	Asaf Romano (gone) Assignee
	Comment 14 • 17 years ago

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

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 15 • 17 years ago

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]

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 16 • 17 years ago

mano checked in a follow up fix to test_placesTxn.js and I no longer crash and we pass the tests.

for his checkin, see: 

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/components/places/tests/unit&command=DIFF_FRAMESET&file=test_placesTxn.js&rev1=1.11&rev2=1.12&root=/cvsroot

	Al Billings [:abillings - ex-MoCo]
	Comment 17 • 17 years ago

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

	Martin Stubenschrott
	Comment 18 • 17 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 19 • 17 years ago

(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

	Marco Bonardo [:mak]
	Comment 20 • 16 years ago

we have tests for library sorting now.

	Gervase Markham [:gerv]
	Comment 21 • 15 years ago

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