359332 - allowing for per-grouping sorts

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

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

allowing for per-grouping sorts

while places allows for multiple groupings (see bug #358784, for example), we do not allow for different sorts per-group.

what I'd like to do is allow for GROUP_BY_DAY to be sorted by SORT_BY_DATE_DESCENDING followed by GROUP_BY_HOST sorted by SORT_BY_TITLE_ASCENDING.

for a serialized query, that would be something like:

place:&beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&type=1&group=0&sort=4&group=1&sort=1

This would be the uri we'd used for the history sidebar, "View | By Date and Site"

To accomplish this, in addition to fixing the code that serializes / deserializes the queries, we'd need to fix nsNavHistoryContainerResultNode::RecursiveSort() to not simply pass through the sort comparator, but to allow the container to have a sort type that would override the parent node's sort type.

the real goal is to avoid hard coding the assumption of "when sorting by title ascending, sort date containers by date descending", because that is what we want for the history sidebar.

removing that assumption will make it easier for extenstion authors to write a "better" history sidebar.

Comment 1

[Ondrej Brablc]

Bug 385245 has removed grouping. Sorting for "By Date" and "By Date and Site" on the first level is now hardcoded (always by date descending). The sort order on the second level is hardcoded too in PlacesSQLQueryBuilder::SelectAsDay(). I'm not sure whether this degree of customization is really needed. I suggest waiting for requests from extension authors.

Comment 2

[Ronny Perinke]

(In reply to comment #1)
> Bug 385245 has removed grouping. Sorting for "By Date" and "By Date and Site"
> on the first level is now hardcoded (always by date descending). The sort order
> on the second level is hardcoded too in PlacesSQLQueryBuilder::SelectAsDay().
> I'm not sure whether this degree of customization is really needed. I suggest
> waiting for requests from extension authors.
> 
*requesting*

My extension "Bookmarks Menu Button" does not only add a bookmarks button, but also a history button, that includes the history in a submenu. It is normaly sorted "by date and site" (1st level day, 2nd site). It was also in possible the current testing version for Firefox 3.0 to sort by date (1st), host (2nd) and site (3rd).


here is an example (the full test-package can be found in bug 405296 including instructions how to install)

  <menubar id="sample-menubar">
    <menu id="sample-menu" label="History (by Day, Domain &amp; Host)">
      <menupopup type="places" place="place:group=0&amp;group=2&amp;group=1&amp;sort=1"/>
    </menu>
    <menu id="sample-menu" label="History (by Day &amp; Host)">
      <menupopup type="places" place="place:group=0&amp;group=1&amp;sort=1"/>
    </menu>
    <menu id="sample-menu" label="History (by Day)">
      <menupopup type="places" place="place:group=0&amp;sort=1"/>
    </menu>
    <menu id="sample-menu" label="History (no grouping, limited to 100)">
      <menupopup type="places" place="place:sort=1&amp;maxResults=100"/>
    </menu>
  </menubar>


Please undo the removal.

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)

> 
> Please undo the removal.
> 

Hi Ronny,

This fix is not going to be rolled back.

The problem with the old grouping approach is that while it provided more customization, performance *blows*. The groupings are done post-query, so we had to pull most, if not all, of the user's history out of the db before grouping.

The new solution is less flexible, but smarter and faster. I'd suggest filing a new bug to get that custom grouping added. Or better, lets implement an API for registering custom groupers at runtime, which would provide extensions the most flexibility, and remove grouping specialization from the backend altogether.

Also, you can emulate the previous functionality from js: Query for a flat list of all history, and build the groupings manually. Sadly, that's basically what the old backend was doing. I'd be glad to help you out if you want to pursue this.

Comment 4

[Ondrej Brablc]

The old way of grouping was not efficient enough, we should look forward how to make it work for you now. I would suggest something like:

place:type=T1&sort=S2&passthru=(type=T2;sort=S2;passthru=(type=T3;sort=S3))

If you look at http://mxr.mozilla.org/firefox/source/toolkit/components/places/src/nsNavHistory.cpp#2928 you see how the day queries are constructed. The type and sort of the second level are already parameters of the query. Instead of hardcoding this we would append to the query the value of passthru option (obviously needs some s/;/&/). This way we could remove RESULTS_AS_DATE_SITE_QUERY.

Additionally we could have something like beginTime=inherit in the passthru value what would get replaced too. May be we could achieve grouping by site and date this way too. 

Comment 5

[Ronny Perinke]

(In reply to comment #3)
> [...] Or better, lets implement an API for
> registering custom groupers at runtime, which would provide extensions the most
> flexibility, and remove grouping specialization from the backend altogether.
> 
sounds like a good compromise :)

> Also, you can emulate the previous functionality from js: Query for a flat list
> of all history, and build the groupings manually. Sadly, that's basically what
> the old backend was doing. I'd be glad to help you out if you want to pursue
> this.
> 

And I'd highly appreciate your help, Dietrich. An example how to do that would be nice. :D
I guess this is the only way to achieve this for my extension, until there is a
better way ...

Comment 6

[Ondrej Brablc]

With bug 420003 we have found that "Smart Bookmarks -> Recent Tags -> TagX" does not show included bookmarks in their natural order, it inherits from the parent query. This bug has the potential to fix it.

Comment 7

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

This on its own doesn't block, but if this ends up being the only way to fix the blocker in 420003, we'll obviously take that patch.

Comment 8

[Ondrej Brablc]

Please take a look at bug 421118. I suggest to mark this bug as invalid since we do not have groups anymore.

Comment 9

[Dietrich Ayala (:dietrich)]

-> invalid in favor of bug 421118

Comment 10

[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