320655 - History view changes should change sorting

Status: RESOLVED FIXED

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

Description

[Brett Wilson]

When you toggle between "group by domain" and "group by visit" the default sorting mode should change. Domain should be displayed alphabetically (it's weird to see this sorted by last visit date). Visits should be sorted by date by default.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

comments to follow

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

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

This patch implements view sorting/grouping. 

>Index: browser/components/places/content/commands.inc
>+      <command id="placesCmd_groupby:feed" 
...

This just attaches the handler functions to the group-by-feed/group-by-post commands. 


>Index: browser/components/places/content/controller.js
>-   * The current groupable Places view.
>+   * The current groupable Places view. This is tracked independently of the 
...

Add better documentation about the reason for this property. 

>+  loadNodeIntoView: function PC_loadQueries(view, node, groupings, sortingMode) {

Generic function for loading a query-result-node into a particular view, grouped and sorted a certain way. 

>-  setGroupingMode: function PC_setGroupingOptions(groupings) {
>+  setGroupingMode: function PC_setGroupingMode(groupings, sortingMode) {

Allow specific grouping modes to have specific sorting options, e.g. sort-by-title for domain groupings, sort-by-date for flat lists. 

>   groupBySite: function PC_groupBySite() {
>-    this.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_DOMAIN]);
>+    var groupings = [Ci.nsINavHistoryQueryOptions.GROUP_BY_DOMAIN];

Update all the call sites to pass sort info too. 

>+  groupByAnnotation: 
>+  function PC_groupByAnnotation(annotation, groupings, sortingMode) {

Supply a method for loading a view with results matching an annotation.

>Index: browser/components/places/content/places.js
>-  placeSelected: function PP_placeSelected(event) {
>-    var node = this._places.selectedNode;
>-    if (!node || this._places.suppressSelection)
>+  onPlaceSelected: function PP_onPlaceSelected(event) {
>+    var node = asQuery(this._places.selectedNode);
>+    if (!node)

Change function name, clean this code up a lot and use generic function. 

>+  groupByFeed: function PP_groupByFeed() {

Implement group by feed/post handlers. 


>   onContentChanged: function PP_onContentChanged() {
>     var panelID = "commands_history";
>-    var filterButtonID = "filterList_history";

Remove some dead code, make sure that the right commands for grouping feed entries are available.

>Index: browser/components/places/content/places.xul
>-              onselect="PlacesPage.placeSelected(event);"
>+              onselect="PlacesPage.onPlaceSelected(event);"

Change function name. 

>           <hbox class="commands" id="commands_feed" flex="1">
>-            <button id="subscribe" command="placesCmd_new:folder"/>
>-            <separator/>

Remove unnecessary button. 

>             <button id="groupOption0" class="commandGroupButton first" 
>-                    view="placeContent" command="placesCmd_groupby:site"/>
>+                    view="placeContent" command="placesCmd_groupby:feed"/>

Use the right command names.

Comment 3

[Annie Sullivan]

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


>+  loadNodeIntoView: function PC_loadQueries(view, node, groupings, sortingMode) {
>+    ASSERT(view, "Must have a view to load node contents into!");
>+    var node = asQuery(node);
>+    var queries = node.getQueries({ });
>+    var newQueries = [];
>+    for (var i = 0; i < queries.length; ++i) {
>+      var query = queries[i].clone();
>+      newQueries.push(query);
>+    }
>+    var newOptions = node.queryOptions.clone();
>+    
>+    // Update the grouping mode only after persisting, so that the URI is not 
>+    // changed. 
>+    newOptions.setGroupingMode(groupings, groupings.length);
>+    
>+    // Set the sort order of the results
>+    newOptions.sortingMode = sortingMode;
>+    
>+    // Reload the view 
>+    view.load(newQueries, newOptions);
>+  },

Note that the menu view has no load() method because it might only be a result node, and not a result.  I'm not sure whether it's a better solution to add an ASSERT() on view.load with a comment, or implement the menu load method.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

I adjusted the comment to specify that the view must be a tree view. A "load" method is generally only useful for views whose contents can change based on selection changes in another view. This is not so for menupopups, whose content is generated when they are shown. 

I want to clearly define the view interfaces somewhere. This will reduce the confusion. 

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Fixed branch and trunk. 

Comment 6

[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