378799 - GROUP_BY_FOLDER only works in simple bookmark-folder queries

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

GROUP_BY_FOLDER only works in simple bookmark-folder queries (place:folder=number), for complex queries (i.e. nsNavHistoryQueryResultNode), GROUP_BY_FOLDER breaks the result.

Comment 1

[Dietrich Ayala (:dietrich)]

Is the expected result a flat level of folders containing items that match the search parameters? Or like a regular bookmarks tree, but with the only visible folders being those with items that match the search parameters (or with descendant folders that contain items that match the search parameters)?

The latter option seems to be less confusing (eg: a flat result could result in multiple folders w/ the same name), however neither seems as useful as a regular folder-less bookmarks search. What are some use-cases for this? What are the scenarios in which we, or extension developers, would want to do this?

Also, this seems like a nice-to-have for A6, not a release blocker.

Comment 2

[Dietrich Ayala (:dietrich)]

re-targeting for beta 1. please comment if you have specific use-cases that require this functionality.

Comment 3

[Dietrich Ayala (:dietrich)]

without a use-case, i question whether this is blocking-worthy. anyone?

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: version 1 - flat list of folders

this implements a flat grouping by folder. for example, given a hierarchy like:

folder 1
  bookmark 1
  folder 2
    bookmark 2

the top-level GBF result would be:

folder 1
folder 2

Comment 5

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

fwiw, see https://bugzilla.mozilla.org/attachment.cgi?id=282667 (and my wip version of GroupByTag())

I think you'll want to create a unqiue urn to pass into the first param to the nsNavHistoryContainerResultNode ctor, if we plan to show these results in a tree (so that we can persist the open state)

I think you'll want to pass in the folder id for the second param to CreatePlacesPersistURN().  (my patch for tags changes that param from "age in days" to just a PRInt64 value to be used to make the urn unique, which you'll need in case folder 1 and folder 2 have the same title.

Comment 6

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

> without a use-case, i question whether this is blocking-worthy. anyone?

I'm investigating if I can use group by folder (instead of group by tag, which I've implemented) for the tag related menus for bug #387996.

I'll try out your patch, and if that works, that would be better and makes this a blocker.

Comment 7

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

dietrich / mano:  your group by folders will work for what I need it for the tag specific queries for bug #387996.

I did need to make a change to nsNavHistory::RowToResult() to get the title of tagged bookmarks, and I made the CreatePlacesPersistURN() change I mentioned in comment #5).

I'll attach an updated version of your patch that I've got in my local tree.

Comment 8

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

a modified version of dietrich original patch landed along with the fix for bug #387996.

Thanks again, Dietrich!

Comment 9

[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