402799 - Saved searches that match on tags show duplicates

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[bomfog]

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.

Comment 1

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

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.

Comment 2

[bomfog]

Is there a good spot to start for info on deciphering ctrl+shift+e parameters 'n stuff?

Comment 3

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

> Is there a good spot to start for info on deciphering ctrl+shift+e parameters
> 'n stuff?

note, mano recently removed the debug panel.

Comment 4

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

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.

Comment 5

[[:Aleksej]]

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.

Comment 6

[bomfog]

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.

Comment 7

[Matthew Babbs]

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.

Comment 8

[[:Aleksej]]

This makes the feature less usable. Nominating for blocking to get at least wanted.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Marco: is there an easy fix for this? I don't think it blocks, but it's really, really desired cleanup.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

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

Comment 13

[bomfog]

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

Comment 14

[cmtalbert]

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.

Comment 15

[cmtalbert]

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

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 18

[Dietrich Ayala (:dietrich)]

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

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

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.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 318345 [details] [diff] [review]
patch

a=beltzner, yay, tests!

Comment 22

[:Gavin Sharp [email: [e-mail]]]

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 

Comment 23

[Tracy Walker [:tracy]]

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

Comment 24

[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