317847 - Implement "Save this Search"

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

When the user searches, a button should become available in the command bar to let them save the search into their Places list.

Comment 1

[Brett Wilson]

This is probably just a button on the query builder (bug 317831)

Comment 2

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

-> sullivan

Another requirement: there needs to be some additional bit set, e.g. as an annotation for the saved place URI that the saved place was user-generated, so that when the place is selected, an "Edit" button appears in the header, so the user can manipulate the saved place after the fact.

Comment 3

[Sylvain Pasche]

(In reply to comment #0)
> When the user searches, a button should become available in the command bar to
> let them save the search into their Places list.

Just curious: Where get the search saved exactly, and what is its name ? Maybe this could open a modified "Add Bookmark" popup, where the Location field would not contain an URL, but something more meaningful in this context.

Comment 4

[Annie Sullivan]

> Just curious: Where get the search saved exactly, and what is its name ? Maybe
> this could open a modified "Add Bookmark" popup, where the Location field would
> not contain an URL, but something more meaningful in this context.

The current UI design is described in the Places UI wiki: http://wiki.mozilla.org/Places:User_Interface#Saving_a_Search_Query

Comment 5

[Sylvain Pasche]

> The current UI design is described in the Places UI wiki:
> http://wiki.mozilla.org/Places:User_Interface#Saving_a_Search_Query

Thanks for the link (also I knew about it). However, the description does not explains precisely what happens once the save button is clicked. It says "the user saves the query", but does not tell on which folder it will be inserted, or what will be the bookmark label associated with it.

I was talking about it because I am faced with the same UI interaction with another project of mine. But maybe I get too much in the details for now.

Comment 6

[Annie Sullivan]

Attachment: First pass at saving searches

This adds basic save search functionality; the save dialog is implemented with a non-localized prompt until we get a more finalized UI.

Brett, I get asserts when I save searches, and when I drag them into folders.  Also, the view doesn't update correctly.

Comment 7

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

Looks good so far Annie... I'd suggest creating a menu item under File too for it, have it observe the command, and use the logic you have in saveSearch: trigger enabling/disable the command...

To do that I recommend pulling _setEnabled out of PlacesController and make it a global utility function, called setCommandEnabled(enabled); Make organizerCommandSet be a commandupdater, and in the onCommandUpdate function that you define in PlacesOrganizer, have it enable/disable using setCommandEnabled. 

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: wip

brought this patch up to date, and fixed a few things:

+ on searching, should always show the modifiers (save, etc)
+ when changing collections, should not clear search, instead search again in the chosen collection
+ can't delete queries from the front-end (item id is -1)
+ searches disappear after a reboot (^^)

still to do:
- no context menu showing
- after saving a search in the organizer, folder tree not showing it (not live updating container nodes when excludeItems?)
- search results should not expand queries (infinite recursion, eg: a saved search named "help" that searches for "help", which contains itself in the result)
- expandQueries expands items even when excludeItems=1
- test copy/paste, dnd

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: wip

fixes:

+ no context menu for queries
+ live update doesn't work for bookmarks trees if excludeItems=1 (item type in the bookmarks system is bookmark, but treated like a container in the result and view)
+ expandQueries expands items even when excludeItems=1

still to-do:

- saved-search results should not include themselves (infinite recursion, eg: a saved search for "help", which is named "help", and returns itself in the result). this is partially fixed. they're still showing up in the organizer content pane, but only recursing one level below the original.

- pasting a saved search results in a regular folder w/ the right name, but empty

- saved searches on the toolbar show the drop arrow, but are empty

- enable the "properties" context menu item and organizer toolbar button once mano's new popup lands

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: wip

Tests needed:
- that queries don't contain themselves in their results
- that queries don't contain queries in their results when excludeQueries is set
- that a query result node can have an itemId
- that query nodes are not expandable if excludeItems is set (zero children after opening the container)

TODO
- localizable save dialog
- pasting a saved search results in a regular folder w/ the right name, but empty
- enable editing of saved searches

Fixed
+ queries should filter out queries in results
+ saved searches on the toolbar show the drop arrow, but are empty
+ saved-search results should not include themselves (infinite recursion, eg: a saved search for "help", which is named "help", and returns itself in the result)

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: wip

adds tests

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

moved strings to properties file

there are still some open issues with this patch, but i don't think they block landing it so we can get testing and catch regressions before freeze.

known followup items:

- localizable save dialog
- pasting a saved search results in a regular folder w/ the correct name, but
empty
- enable editing of saved searches
- live-update not working for bookmark searches

Comment 13

[Asaf Romano (gone)]

Comment on attachment 278602 [details] [diff] [review]
fix v1


>Index: .xul
>===================================================================

>+        place="place:folder=2&queryType=1&expandQueries=1"

Rather than fixing every single query, please make it the default (what's the use case for expandQueries=false anyway?)


>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.96
>diff -u -8 -p -r1.96 places.js
>--- browser/components/places/content/places.js	22 Aug 2007 14:42:37 -0000	1.96
>+++ browser/components/places/content/places.js	28 Aug 2007 17:18:14 -0000
>@@ -187,20 +187,25 @@ var PlacesOrganizer = {
>       return;
>     var node = asQuery(this._places.selectedNode);
>     LOG("Node URI: " + node.uri);
>     var queries = node.getQueries({});
> 
>     // Items are only excluded on the left pane
>     var options = node.queryOptions.clone();
>     options.excludeItems = false;
>-    // Unset excludeQueries so incremental update is enabled for the content
>-    // pane.
>-    // XXXmano: remove that once we unset excludeQueries for the left pane.
>-    options.excludeQueries = false;
>+
>+    var count = {};
>+    queries[0].getFolders(count);
>+    if (count.value > 0) {

hrm, this seems wrong, you could have a complex query (i.e. saved search) with a scoping folder.

>+    var ios = Cc["@mozilla.org/network/io-service;1"].
>+                getService(Ci.nsIIOService);
>+    var placeURI = ios.newURI(placeSpec, null, null);

ScriptableIO-it


>     case "bookmarks":
>-      if (filterString != "")
>+      if (filterString != "") {

nit: if (filterString)

>         content.applyFilter(filterString, true);
>-      else
>+        PO.setHeaderText(PO.HEADER_TYPE_SEARCH, filterString);
>+      }
>+      else {
>         PlacesOrganizer.onPlaceSelected();
>+      }

please avoid braces single-line blocks.

>+    if (this.searchFilter.value != "")

nit: if (this.searchFilter.value)


>Index: browser/components/places/content/places.xul
>===================================================================
>     <command id="OrganizerCommand_export"
>              oncommand="PlacesOrganizer.exportBookmarks();"/>
>     <command id="OrganizerCommand_import"
>              oncommand="PlacesOrganizer.importBookmarks();"/>
>-    <command id="OrganizerCommand_search:save"/>
>+    <command id="OrganizerCommand_search:save"
>+      oncommand="PlacesOrganizer.saveSearch();"/>

fix indent.


>Index: browser/components/places/content/placesOverlay.xul
>===================================================================
>     <menuseparator id="placesContext_sortSeparator"/>
>     <menuitem id="placesContext_show:info"
>               command="placesCmd_show:info"
>               label="&cmd.properties.label;" 
>               accesskey="&cmd.properties.accesskey;"
>-              selection="bookmark|folder"
>+              selection="bookmark|folder|query"

hrm, is the current dialog useful for saved searches?


>Index: toolkit/components/places/src/nsNavHistory.cpp
>===================================================================

> nsresult
>-nsNavHistory::FilterResultSet(const nsCOMArray<nsNavHistoryResultNode>& aSet,
>+nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aParentNode,
>+                              const nsCOMArray<nsNavHistoryResultNode>& aSet,
>                               nsCOMArray<nsNavHistoryResultNode>* aFiltered,
>                               const nsString& aSearch)
> {

please annotate the changes here (on the bug).

>Index: toolkit/components/places/src/nsNavHistoryResult.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v
>retrieving revision 1.112
>diff -u -8 -p -r1.112 nsNavHistoryResult.cpp
>--- toolkit/components/places/src/nsNavHistoryResult.cpp	15 Aug 2007 06:14:26 -0000	1.112
>+++ toolkit/components/places/src/nsNavHistoryResult.cpp	28 Aug 2007 17:18:21 -0000
>@@ -1895,18 +1895,22 @@ nsNavHistoryQueryResultNode::nsNavHistor
> //    Whoever made us may want non-expanding queries. However, we always
> //    expand when we are the root node, or else asking for non-expanding
> //    queries would be useless.
> 
> PRBool
> nsNavHistoryQueryResultNode::CanExpand()
> {
>   nsNavHistoryQueryOptions* options = GetGeneratingOptions();
>-  if (options && options->ExpandQueries())
>-    return PR_TRUE;
>+  if (options) {
>+    if (options->ExcludeItems())
>+      return PR_FALSE;

might worth a comment.


>Index: toolkit/components/places/tests/bookmarks/test_savedsearches.js
>===================================================================

>+      // test that the container is closed
>+      var query = node.QueryInterface(Ci.nsINavHistoryQueryResultNode);

>+      do_check_eq(query.containerOpen, false);

why do you QI to nsINavHistoryQueryResultNode?

>+    }
>+    rootNode.containerOpen = false;

why?

>+      var query = node.QueryInterface(Ci.nsINavHistoryQueryResultNode);
>+      query.containerOpen = true;

ditto.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

> >-    // Unset excludeQueries so incremental update is enabled for the content
> >-    // pane.
> >-    // XXXmano: remove that once we unset excludeQueries for the left pane.
> >-    options.excludeQueries = false;
> >+
> >+    var count = {};
> >+    queries[0].getFolders(count);
> >+    if (count.value > 0) {
> 
> hrm, this seems wrong, you could have a complex query (i.e. saved search) with
> a scoping folder.

I removed it, and confirmed that live updating still works properly for the views.

> >Index: browser/components/places/content/placesOverlay.xul
> >===================================================================
> >     <menuseparator id="placesContext_sortSeparator"/>
> >     <menuitem id="placesContext_show:info"
> >               command="placesCmd_show:info"
> >               label="&cmd.properties.label;" 
> >               accesskey="&cmd.properties.accesskey;"
> >-              selection="bookmark|folder"
> >+              selection="bookmark|folder|query"
> 
> hrm, is the current dialog useful for saved searches?

no. removed for now.

> >Index: toolkit/components/places/src/nsNavHistory.cpp
> >===================================================================
> 
> > nsresult
> >-nsNavHistory::FilterResultSet(const nsCOMArray<nsNavHistoryResultNode>& aSet,
> >+nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aParentNode,
> >+                              const nsCOMArray<nsNavHistoryResultNode>& aSet,
> >                               nsCOMArray<nsNavHistoryResultNode>* aFiltered,
> >                               const nsString& aSearch)
> > {
> 
> please annotate the changes here (on the bug).

The parent node provides access to the query options, which are necessary for post facto filtering for options such as excludeQueries.

> >Index: toolkit/components/places/tests/bookmarks/test_savedsearches.js
> >===================================================================
> 
> >+      // test that the container is closed
> >+      var query = node.QueryInterface(Ci.nsINavHistoryQueryResultNode);
> 
> >+      do_check_eq(query.containerOpen, false);
> 
> why do you QI to nsINavHistoryQueryResultNode?

because containerOpen is not a property of nsINavHistoryResultNode.

> 
> >+    }
> >+    rootNode.containerOpen = false;
> 
> why?

so it will not receive live updates. but it doesn't buy much in this context, so removed.

Comment 15

[Asaf Romano (gone)]

-    // Unset excludeQueries so incremental update is enabled for the content
-    // pane.
-    // XXXmano: remove that once we unset excludeQueries for the left pane.
-    options.excludeQueries = false;

ew, this forces a full rebuild of the content pane whenever an item is changed.

Comment 16

[Asaf Romano (gone)]

> >+      // test that the container is closed
> >+      var query = node.QueryInterface(Ci.nsINavHistoryQueryResultNode);
> 
> >+      do_check_eq(query.containerOpen, false);
> 
> why do you QI to nsINavHistoryQueryResultNode?

then QI to the container interface

btw, I prefer:
node.QueryInterface(Ci.nsINavHistoryContainerResultNode);
node.containerOpen = true;

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> -    // Unset excludeQueries so incremental update is enabled for the content
> -    // pane.
> -    // XXXmano: remove that once we unset excludeQueries for the left pane.
> -    options.excludeQueries = false;
> 
> ew, this forces a full rebuild of the content pane whenever an item is changed.
> 

excludeQueries is no longer set on the left-pane. which means unsetting it here is no longer required, right?

i've confirmed that live-update is working properly in the content pane...

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

with comment #16 fixed.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 279018 [details] [diff] [review]
fix v3

OK, go ahead and land this.

>Index: browser/components/places/content/places.js
>===================================================================

>+  /**
>+   * Save the current search (or advanced query) to the places root.
>+   */

bookmarks root.

Comment 20

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.99; previous revision: 1.98
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.80; previous revision: 1.79
done
Checking in browser/components/places/content/placesOverlay.xul;
/cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v  <--  placesOverlay.xul
new revision: 1.17; previous revision: 1.16
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.159; previous revision: 1.158
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.95; previous revision: 1.94
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v  <--  nsNavHistoryQuery.cpp
new revision: 1.32; previous revision: 1.31
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.h,v  <--  nsNavHistoryQuery.h
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.35; previous revision: 1.34
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_savedsearches.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_savedsearches.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_savedsearches.js,v  <--  test_savedsearches.js
initial revision: 1.1
done

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: as checked in

Comment 22

[Steve England [:stevee]]

I believe this caused bug 394741

Comment 23

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091005 Minefield/3.0a8pre

Comment 24

[Al Billings [:abillings - ex-MoCo]]

We should add test cases for this and related functionality.

Comment 25

[Tracy Walker [:tracy]]

This feature was dramatically reduced in functionality. We have a testcase in litmus for basic search save.

Comment 26

[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