377500 - Result is broken if two bookmarks items have the same URL but with different titles

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Asaf Romano (gone)]

Result is broken if two bookmarks items have the same URI but with different titles, at least if they're listed under the same container.

It seems like the title of the first bookmark in the container is used for all bookmark items with the same URL.

Comment 1

[Asaf Romano (gone)]

Attachment: WIP

So this works except it breaks searching over bookmarks even more.

Comment 2

[Asaf Romano (gone)]

Attachment: "Mano learns SQL v2"

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 261735 [details] [diff] [review]
"Mano learns SQL v2"

Code-wise, looks ok. However, when searching bookmarks, the search results show the moz_places entries for each bookmark. This is especially bad in the case of livemark entries, which are usually unvisited, so they don't have a proper title (eg: stm892ut.htm). The search results are then cluttered with entries of places that you haven't ever been, with useless titles, thereby making it much *harder* to find where you've been before, instead of easier :P

It seems like we shouldn't show bookmarked history entries in the unified search results... they don't provide much value if we're going to show the bookmark anyways, they just become more results for the user to sift through.
If we *do* want to show history entries of bookmarks, we should only show history entries that have been visited (and whose title differs from the bookmark), which would remove a lot of the clutter.

We might need to put the bookmarks query part of the UNION first, and then configure the history part of the query second, based on whether we're also querying bookmarks.

As it is, the bookmark search results are not usable, so removing the review request until we figure out a solution.

Comment 4

[Asaf Romano (gone)]

Attachment: v3

So other than the bug reported here:
 - The history sidebar no longer uses bookmark titles
 - Bookmarks in search queries show up as bookmarks, not as history items with their first-bookmark title
 - maxResults is honored for bookmark queries
 - Reduce size_of various places objects.
 - Disable unified search for now.
 - add queryType option, see idl.
 - clone maxResults option in nsNavHistoryQueryOptions::Clone

Comment 5

[Dietrich Ayala (:dietrich)]

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

thanks for doing this prtypes and footprint work!

the explicit query-type approach is good. but canceling review because lots of failures and fuzz when applying the patch. can you update it?

>+
>+  /**
>+   * The type of seatch to use when quering the DB; This attribute is only
>+   * honored by query nodes. It is silently ignored for simple folder queries.
>+   */
>+  attribute unsigned short queryType;

s/seatch/search/
s/quering/querying/

>+    }
>+    else {
>+      // XXX: implement meu
>+      return NS_ERROR_NOT_IMPLEMENTED;
>+    }

s/meu/me/

>@@ -3403,16 +3424,20 @@ nsNavHistory::RecursiveGroup(const nsCOM
>       rv = GroupByDay(aSource, aDest);
>       break;
>     case nsINavHistoryQueryOptions::GROUP_BY_HOST:
>       rv = GroupByHost(aSource, aDest, PR_FALSE);
>       break;
>     case nsINavHistoryQueryOptions::GROUP_BY_DOMAIN:
>       rv = GroupByHost(aSource, aDest, PR_TRUE);
>       break;
>+    case nsINavHistoryQueryOptions::GROUP_BY_FOLDER:
>+      // not yet supported (this code path is not reached for simlple bookmark
>+      // folder queries)
>+      return NS_ERROR_NOT_IMPLEMENTED;

s/simlple/simple/

> 
>   if (IsQueryURI(url)) {
>     // special case "place:" URIs: turn them into containers
>+    // XXX: should we set the bookmark identifier for this sort of nodes? It
>+    // would sure break few assumption on the frontend side
>     return QueryRowToResult(url, title, accessCount, time, favicon, aResult);

when would a place URI have a bookmark id? is there a use-case (yet) for bookmarking a place URI?

>@@ -1394,16 +1407,33 @@ SetOptionsKeyUint32(const nsCString& aVa
>     if (NS_FAILED(rv)) {
>       NS_WARNING("Error setting Int32 key value");
>     }
>   } else {
>     NS_WARNING("Invalid Int32 key value in query string.");
>   }
> }
> 
>+void // static
>+SetOptionsKeyUint16(const nsCString& aValue, nsINavHistoryQueryOptions* aOptions,
>+                    Uint16OptionsSetter setter)
>+{
>+  nsresult rv;
>+  PRUint16 value = NS_STATIC_CAST(PRUint16,
>+                                  aValue.ToInteger(NS_REINTERPRET_CAST(PRInt32*, &rv)));
>+  if (NS_SUCCEEDED(rv)) {
>+    rv = (aOptions->*setter)(value);
>+    if (NS_FAILED(rv)) {
>+      NS_WARNING("Error setting Int32 key value");
>+    }
>+  } else {
>+    NS_WARNING("Invalid Int32 key value in query string.");
>+  }
>+}
>+

need to update the warnings to "Int16"

>Index: browser/components/places/content/bookmarksPanel.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/bookmarksPanel.xul,v
>retrieving revision 1.5
>diff -u -p -8 -r1.5 bookmarksPanel.xul
>--- browser/components/places/content/bookmarksPanel.xul	10 Apr 2007 00:30:52 -0000	1.5
>+++ browser/components/places/content/bookmarksPanel.xul	22 Apr 2007 09:41:33 -0000
>@@ -65,17 +65,17 @@
>     <textbox id="search-box" flex="1"
>              type="timed" timeout="500"
>              oncommand="searchBookmarks(this.value);"/>
>   </hbox>
> 
>   <tree id="bookmarks-view" class="placesTree" type="places"
>         flex="1"
>         hidecolumnpicker="true"
>-        place="place:folder=2"
>+        place="place:folder=2&amp;queryType=1"
>         context="placesContext"
>         onkeypress="if (event.keyCode == 13) this.controller.openSelectedNodeWithEvent(event);"
>         onclick="SidebarUtils.handleClick(this, event);">
>     <treecols>
>       <treecol id="title" flex="1" primary="true" hideheader="true"/>
>     </treecols>
>     <treechildren id="bookmarks-view-children" view="bookmarks-view" flex="1"/>
>   </tree>

don't the other front-end queries need to be updated as well?

Comment 6

[Asaf Romano (gone)]

(In reply to comment #5)

> >   if (IsQueryURI(url)) {
> >     // special case "place:" URIs: turn them into containers
> >+    // XXX: should we set the bookmark identifier for this sort of nodes? It
> >+    // would sure break few assumption on the frontend side
> >     return QueryRowToResult(url, title, accessCount, time, favicon, aResult);
> 
> when would a place URI have a bookmark id? is there a use-case (yet) for
> bookmarking a place URI?
>

Saved searches, see "Google Sites" test in test_bookmarks.

> 
> >Index: browser/components/places/content/bookmarksPanel.xul
> >===================================================================
> don't the other front-end queries need to be updated as well?
> 

Hrm, only in the organizer. All other bookmark queries result in simple folder queries for which this attribute is ignored.

Comment 7

[Asaf Romano (gone)]

Attachment: v3.1

Comment 8

[Dietrich Ayala (:dietrich)]

re: the problem we discussed in IRC

this is the sql for bookmarks searches:

SELECT b.fk, h.url, b.title, h.rev_host, h.visit_count,
  (SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = b.fk),
  f.url, null, b.id
FROM moz_bookmarks b
  JOIN moz_places h ON b.fk = h.id
  LEFT OUTER JOIN moz_historyvisits v ON b.fk = v.place_id
  LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id
WHERE (EXISTS (SELECT b.fk FROM moz_bookmarks b WHERE b.type = 1 AND b.fk = h.id))
GROUP BY b.id

i looked at the query output, and it seems that folder types (b.type==2) are somehow being munged into the result set. the bad result record itself is a mashup of the bbc livemark and the "help" bookmark. this is because the |type| column is not being filtered properly, and the .fk column value for those 2 items is the same in my database. if i add "and b.type = 1" to the WHERE clause, outside the EXISTS sub-select, then the query works properly. i haven't yet looked further to see why the EXISTS bit isn't working.

Comment 9

[Asaf Romano (gone)]

Attachment: v3.2

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 262689 [details] [diff] [review]
v3.2

r=me, thanks

Comment 11

[Asaf Romano (gone)]

mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.56
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.81
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.118
mozilla/toolkit/components/places/src/nsNavHistory.h 1.75
mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp 1.28
mozilla/toolkit/components/places/src/nsNavHistoryQuery.h 1.15
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.87
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.33
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.5
mozilla/browser/components/places/content/bookmarksPanel.xul 1.6
mozilla/browser/components/places/content/places.xul 1.71

Comment 12

[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