Closed
Bug 393870
Opened 17 years ago
Closed 17 years ago
unable to sort by tags in the bookmark organizer
Categories
(Firefox :: Bookmarks & History, defect, P3)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta3
People
(Reporter: moco, Assigned: asaf)
References
Details
Attachments
(1 file, 1 obsolete file)
38.30 KB,
patch
|
Details | Diff | Splinter Review |
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 .....
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 M10
Updated•17 years ago
|
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Updated•17 years ago
|
Priority: -- → P5
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.
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
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
Assignee | ||
Comment 5•17 years ago
|
||
We already do that.
Assignee: nobody → mano
Target Milestone: Firefox 3 Mx → Firefox 3 M10
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!
Updated•17 years ago
|
Priority: P5 → P3
Target Milestone: Firefox 3 M10 → Firefox 3 M11
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.
Attachment #293460 -
Flags: review?(sspitzer)
Assignee | ||
Updated•17 years ago
|
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
Assignee | ||
Updated•17 years ago
|
Attachment #293460 -
Flags: review?(dietrich)
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
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.
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.
Comment 10•17 years ago
|
||
Comment on attachment 293460 [details] [diff] [review]
re-implement the tags column
r=me w/ previous comments fixed
Attachment #293460 -
Flags: review?(dietrich) → review+
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.
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?
Attachment #293460 -
Flags: review?(sspitzer) → review+
Reporter | ||
Updated•17 years ago
|
Flags: in-testsuite?
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.
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
Attachment #293460 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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]
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
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.
Status: RESOLVED → VERIFIED
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.
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
Comment 20•16 years ago
|
||
we have tests for library sorting now.
Flags: in-testsuite? → in-testsuite+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•