393464 - Bookmarks manager doesn't search in tags field

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Darren Kalck [:D-Kalck]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082305 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082305 Minefield/3.0a8pre

I added a page to my bookmarks and tagged it, when I go to Bookmarks manager I see the tags in the column, but if I use 'Search in Bookmarks' of the download manager and type one of the tag I used for the page, it doesn't find it.

Reproducible: Always

Steps to Reproduce:
1.Tag a page with for example "computer"
2.Type "computer" in the search of the Bookmarks manager

Actual Results:  
The page does not appear in the results

Expected Results:  
The page should appear in the results

Comment 1

[(not reading, please use [e-mail] instead)]

see also bug #393584

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: v1

ugh, there must be a better way to get those tags from nsIVariant array.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 279877 [details] [diff] [review]
v1

needs to be exact tag match, as in location bar.

Comment 5

[(not reading, please use [e-mail] instead)]

> needs to be exact tag match, as in location bar.

I think it should be a case insensitive exact match.

Comment 6

[(not reading, please use [e-mail] instead)]

some comments

1)  as we discussed in irc, move the call to GetTagsForURI() until as late as possible

2)  consider changing GetTagsForURI() to be URIHasTag()

that method would take a nsIURI and your search term (the string) and return a bool.

then in URIHasTag(), you'd get the nsIVariant (the array of tags).  for each tag, see if it exactly matches your search term (but do a case insensitive compare).  this would prevent creating the string array and copying the strings.

note, as we discussed over irc, because the tagging service returns the tags as sorted, you could do a binary search instead of a linear one.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: v2

fixes comments, broadens tests

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 279942 [details] [diff] [review]
v2

1)
 
+// nsITaggingService
+#define TAGGING_SERVICE_CID "@mozilla.org/browser/tagging-service;1"
+

I'd recommend moving that #define to the tagging service idl, like this:


%{C++



#define TAGGING_SERVICE_CID "@mozilla.org/browser/tagging-service;1"

%}

(See lxr for other examples of where we do this, like toolkit\components\startup\public\nsIUserInfo.idl)

2) binary search, nice!  

a) can add a comment about how you can use this because tags are sorted?

b) overkill: for your linear search again since tags are sorted, if position > 0, you can stop searching.

c) hmm, concern:  tags are sorted, but are they sorted by the tagging service in a case insensitive way?  (They may not be, in which case we need to fix that.)

d) hmm, the binary search searches the tags on a given uri.  while the user may have lots of tags, do we expect them to have tons of tags on a single uri? 

If not, I still like URIHasTag(), but maybe I've lead you down the wrong path, and we should just do a linear complete search, and not even worry about doing a case insensitive sort in the tagging service.

3)

+ * Contributor(s):
+ *  Seth Spitzer <[e-mail]>

you, not me.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v3

comments fixed

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 279950 [details] [diff] [review]
v3

r=sspitzer, nice.

sorry about misleading you down the binary search pre-optimization path.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 279950 [details] [diff] [review]
v3

requesting approval: this patch enables searching over tags in the bookmarks sidebar, organizer, and smart folders (saved searches). risk is fairly low. it's got accompanying tests, and the rest of the places test suite would have caught regressions.

Comment 12

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/public/nsITaggingService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsITaggingService.idl,v  <--  nsITaggingService.idl
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.167; previous revision: 1.166
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.96; previous revision: 1.95
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_395101.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_395101.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_395101.js,v  <--  test_395101.js
initial revision: 1.1
done

Comment 13

[Marcia Knous [:marcia]]

I tested this using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007090704 Minefield/3.0a8pre. I get two entries when I try to tag a page. See attached screenshot for a sample of what happens when I try to tag any page and then search for it in organize bookmarks.

Comment 14

[Marcia Knous [:marcia]]

Attachment: Screenshot of issue

This is what I get when I tag the espn.go.com home page with "sports" and then search for "sports" in organize bookmarks. This happens when I tagged other sites as well - I always seem to get two entries that come up. Not sure whether this is expected.

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> Created an attachment (id=280075) [details]
> Screenshot of issue
> 
> This is what I get when I tag the espn.go.com home page with "sports" and then
> search for "sports" in organize bookmarks. This happens when I tagged other
> sites as well - I always seem to get two entries that come up. Not sure whether
> this is expected.
> 

no, not expected. looking into it.

Comment 16

[Marcia Knous [:marcia]]

Just by way of information I get the same behavior with both a new profile and an existing one (used only for Minefield).

Comment 17

[bomfog]

This has been thus since tags hit the trunk. I saw it when I searched bookmarks for "/" to see the infamous invisible "all bookmarks". You get one "no title" entry per tag, plus the titled original. The untitleds show up in the bookmarks table, as I recall, as children of the tags, one row per child.

I just assumed it was due to the lack of UI at the time, and they'd be hidden from view once tags were functional, so I didn't file a bug.

Comment 18

[Dietrich Ayala (:dietrich)]

reverting until we fix search-in-folders so that the tag entries themselves won't show up in results.

Checking in toolkit/components/places/public/nsITaggingService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsITaggingService.idl,v  <--  nsITaggingService.idl
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.169; previous revision: 1.168
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.99; previous revision: 1.98
done
Checking in toolkit/components/places/tests/bookmarks/test_395101.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_395101.js,v  <--  test_395101.js
new revision: 1.2; previous revision: 1.1
done

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: patch used to revert and disable the tests

Comment 20

[Asaf Romano (gone)]

Re-landed so I can land search-in-folders support, and then fix the issues raised here (in follow-ups).

mozilla/toolkit/components/places/public/nsITaggingService.idl 1.6
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.170
mozilla/toolkit/components/places/src/nsNavHistory.h 1.100
mozilla/toolkit/components/places/tests/bookmarks/test_395101.js 1.3

Comment 21

[(not reading, please use [e-mail] instead)]

> Re-landed so I can land search-in-folders support, and then fix the issues
> raised here (in follow-ups)

mano / dietrich:  besides marcia's comment #14 (logged as bug #397453), what
other follow ups where there?

Comment 22

[Dietrich Ayala (:dietrich)]

(In reply to comment #21)
> > Re-landed so I can land search-in-folders support, and then fix the issues
> > raised here (in follow-ups)
> 
> mano / dietrich:  besides marcia's comment #14 (logged as bug #397453), what
> other follow ups where there?
> 

comment #14 is bug 397165, so bug 397453 seems like a dupe.

Comment 23

[(not reading, please use [e-mail] instead)]

> comment #14 is bug 397165, so bug 397453 seems like a dupe.

you're right, thanks dietrich.  my tree was stale.

Comment 24

[Paul Craciunoiu [:paulc]]

Verified on
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008060309 Firefox/3.0

Comment 25

[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