Closed Bug 402799 Opened 12 years ago Closed 12 years ago

Saved searches that match on tags show duplicates

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: bomfog, Assigned: mak)

References

Details

(Keywords: polish, ue)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a9pre) Gecko/2007110613 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a9pre) Gecko/2007110613 Minefield/3.0a9pre

When I save a Places search that matches on a tag, it includes a duplicate untitled entry along with the target entry. There is one dupe for each tag on the target entry, whether the other tags match the search or not.

Reproducible: Always

Steps to Reproduce:
1.Unzip a recent trunk hourly
2.Start it with a new profile
3.Click the URL bar star twice to open the dialog for the current page
4.Enter XXX in the Tags: field, click done
5.From the Bookmarks menu, click Organize Bookmarks...
6.Enter XXX in the search field, then click the Save button when the advanced search UI opens. Accept the default query name (New Query)
7.Select New Query on the left side of the organizer, and notice the result in the main pane.

Actual Results:  
One entry named 404: File Not Found, and a second entry named (no title) with the same URL and tag.

Expected Results:  
Only display the 404: File Not Found entry, ala the "smart folders" in the Places toolbar folder.

bug 397165 and bug 401450 are related, I think, but they're fixed and the search-saving isn't.
Version: unspecified → Trunk
confirmed.

as you point out, we must not be limiting our search to the proper folders

for those other bugs, we limit to PlacesUtils.bookmarksRootId, PlacesUtils.unfiledRootId (and thereby exclude the tag root)

playing around with the advanced search editor, it looks like there are similar issues when changing the search target between selected folder, all bookmarks, bookmarks menu, bookmarks toolbar, and history.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Server 2003 → All
Hardware: PC → All
Is there a good spot to start for info on deciphering ctrl+shift+e parameters 'n stuff?
> Is there a good spot to start for info on deciphering ctrl+shift+e parameters
> 'n stuff?

note, mano recently removed the debug panel.
note, this bug is still an issue.

1) for a saved search, we show duplicates (because we are not excluding the tag root)

2) for the duplicates, while the title is correct in the list pane (thanks to, I think, mano's recent change to always do "COALESCE(a.title, h.title)" (see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavBookmarks.cpp#156), the title is blank in the details pane.  (that is bug #400590)

it seems like the problem here is we are not saving off the folder options when we save the search.
Another case of “(no title)”:

Open the Library (Places Organizer).
Right‐click a tag under “Tags” in the left pane.
Choose “Sort by Name”.
…after a possible short hang, the entries in the right pane have their titles replaced with “(no title)” until a refresh.

See also bug 412682.
This bug was intended to be about the dupes more than the "(no title)", which seems to be fixed anyway. The dupes have the correct titles now.

Summary corrected.
Summary: Saved searches that match on tags show "(no title)" duplicates → Saved searches that match on tags show duplicates
There are actually two distinct types of duplicate result - (1) where the search term matches on title/uri as well as tag, and (2) where a bookmark has more than one tag, it gets returned once for each tag even if only one of the tags matches.
This makes the feature less usable. Nominating for blocking to get at least wanted.
Flags: blocking-firefox3?
Marco: is there an easy fix for this? I don't think it blocks, but it's really, really desired cleanup.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Keywords: polish, ue
we could filter out those when searching on every queryType=bookmarks, and let tag contents be returned only from RESULTS_AS_TAG_CONTENTS. Will take a look if this appear to be complex at this stage
actually after Bug 429889 if you follow the steps the query is correct. That is because we are setting folder values for the search, so results are returned only from th 3 main roots, and tag contents are excluded.
Old saved searches are instead still showing the issue
Attached patch patch (obsolete) — Splinter Review
- fixed old queries and generic bookmark related queries so that tag contents are excluded.
- fixed potential endian bugs that could nullify/change the filtering on PPC machines.
- fixed test_multi_word_tags.js test, it was expecting tagged bookmarks without bookmarking uris, so it was counting unbookmarked tag contents.
- fixed a glitch in test_419731.js, we were selecting all tags contents instead of 1 tag contents
- fixed test_resolveNullBookmarksTitle.js since we actually don't return tag contents in normal bookmark queries, it will now check that we return history title if the tagged bookmark does not have one (using RESULTS_AS_TAG_CONTENTS)
- added a new test for this specific bug, will check that we don't return tag contents in a normal bookmarks query

This will make impossible asking for a tag folder content directly without specifying RESULTS_AS_TAG_CONTENTS and imo this is good since they should be really available only in the backend for direct queries.

Notice that we have already replaced left pane queries so we should not see any problem with old tag containers.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #317505 - Flags: review?(dietrich)
Whiteboard: [has patch][needs review dietrich]
Flags: in-testsuite?
I only tried a minimal check, but the latest tryserver build seems to WFM. Cool!

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008042411 Minefield/3.0pre - Build ID: 2008042411
mak77: regarding the "in-testsuite" flag, I think I have a test for this ready to go in bug 425842. My test there seems to encounter the same problem that is reported here, but it uses RESULTS_AS_URI and not RESULTS_AS_TAG_CONTAINERS.  I'm happy to amend that test to also test using tag containers, but first I'll give it a whirl with your patch on this bug to see if the original problem is fixed. 

Please let me know if I'm wrong and these are two unrelated issues.
(In reply to comment #14)
Ok answered my own question.  These are the same issue.  Your patch fixes the issue reported in bug 425842.  I'll mark 425842 as a dupe of this one.  Do you want me to append an explicit test for RESULTS_AS_TAG_CONTAINERS for this bug too, or will the test on that bug be sufficient?

I'm going to mark 425842 as a dupe of this bug.

Duplicate of this bug: 425842
sorry clint i don't understand well the question about tests, my test here (included in the patch) should be enough to cover the problem, still if you want to test RESULTS_AS_TAG_CONTENTS in your generic advanced search tests it would be still good.
Comment on attachment 317505 [details] [diff] [review]
patch


>       else {
>+        nsNavHistory* history = nsNavHistory::GetHistoryService();
>+        NS_ENSURE_STATE(history);
>+

could move this out of the if/else now, since used in both cases.

>         mQueryString = NS_LITERAL_CSTRING(
>           "SELECT b.fk, h.url, COALESCE(b.title, h.title), h.rev_host, "
>             "h.visit_count,"
>             SQL_STR_FRAGMENT_MAX_VISIT_DATE( "b.fk" )
>             ", f.url, null, b.id, b.dateAdded, b.lastModified "
>           "FROM moz_bookmarks b "
>                "JOIN moz_places h ON b.fk = h.id AND b.type = 1 "
>-               "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
>+               "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
>+               "WHERE NOT EXISTS "
>+                "(SELECT id FROM moz_bookmarks WHERE id = b.parent AND parent = ") +
>+                nsPrintfCString("%lld", history->GetTagsFolder()) +
>+                NS_LITERAL_CSTRING(") {ADDITIONAL_CONDITIONS}");

nit: fix indent

>Index: toolkit/components/places/tests/unit/test_402799.js
>===================================================================
>RCS file: toolkit/components/places/tests/unit/test_402799.js
>diff -N toolkit/components/places/tests/unit/test_402799.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/components/places/tests/unit/test_402799.js	24 Apr 2008 10:49:06 -0000

>+  // add some tags
>+  tagssvc.tagURI(uri1, ["foo"]);
>+  tagssvc.tagURI(uri1, ["bar"]);
>+  tagssvc.tagURI(uri1, ["foobar"]);
>+  tagssvc.tagURI(uri1, ["foo bar"]);
>+

why all the separate calls to tagURI?

>Index: toolkit/components/places/tests/unit/test_419731.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_419731.js,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 test_419731.js
>--- toolkit/components/places/tests/unit/test_419731.js	11 Apr 2008 16:22:02 -0000	1.1
>+++ toolkit/components/places/tests/unit/test_419731.js	24 Apr 2008 10:49:06 -0000
>@@ -78,30 +78,31 @@ function run_test() {
> 
>   // get tag folder id
>   var options = histsvc.getNewQueryOptions();
>   var query = histsvc.getNewQuery();
>   query.setFolders([bmsvc.tagsFolder], 1);
>   var result = histsvc.executeQuery(query, options);
>   var tagRoot = result.root;
>   tagRoot.containerOpen = true;
>-  var tag1node = tagRoot.getChild(0)
>+  var tagNode = tagRoot.getChild(0)
>                         .QueryInterface(Ci.nsINavHistoryContainerResultNode);

nit: line it up

r=me otherwise
Attachment #317505 - Flags: review?(dietrich) → review+
(In reply to comment #18)
> (From update of attachment 317505 [details] [diff] [review])
> 
> >       else {
> >+        nsNavHistory* history = nsNavHistory::GetHistoryService();
> >+        NS_ENSURE_STATE(history);
> >+
> 
> could move this out of the if/else now, since used in both cases.


See http://support.microsoft.com/kb/86479 this gives an error on compile since we are in the same scope of the switch clause and the compiler thinks that it could remain uninitialized.
So the trick is assigning to the var after its definition, so i'm doing:

      // Don't initialize on var creation, that would give an error on compile
      // because we are in the same scope of the switch clause and the var could
      // not be initialized. Do an assignment rather than an initialization.
      nsNavHistory* history;
      history = nsNavHistory::GetHistoryService();
      NS_ENSURE_STATE(history);

and this compiles and works fine.
Attached patch patchSplinter Review
bringing on Dietrich's review.

Asking approval, this makes the search in the Library working correctly with tagged items.
This is also needed (as a blocker) for PPC machines due to an endian bug on tag folder id assignment.

unit test included.
Attachment #317505 - Attachment is obsolete: true
Attachment #318345 - Flags: review+
Attachment #318345 - Flags: approval1.9?
Comment on attachment 318345 [details] [diff] [review]
patch

a=beltzner, yay, tests!
Attachment #318345 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [has patch][needs review dietrich] → [has patch][has reviews]
mozilla/toolkit/components/places/src/nsNavHistory.cpp 	1.299
mozilla/toolkit/components/places/tests/unit/test_402799.js 	1.1
mozilla/toolkit/components/places/tests/unit/test_419731.js 	1.2
mozilla/toolkit/components/places/tests/unit/test_multi_word_tags.js 	1.3
mozilla/toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js 	1.6 
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → Firefox 3
Verified with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008043004 Minefield/3.0pre
and 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008043007 Minefield/3.0pre
Status: RESOLVED → VERIFIED
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.