Implement "Save this Search"

VERIFIED FIXED in Firefox 3 alpha8

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Assigned: dietrich)

Tracking

Trunk
Firefox 3 alpha8
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [places-ui])

Attachments

(1 attachment, 8 obsolete attachments)

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

12 years ago
This is probably just a button on the query builder (bug 317831)
Depends on: 317831

Updated

12 years ago
Priority: -- → P2
-> 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.
Assignee: brettw → annie.sullivan
Target Milestone: --- → Firefox 2 alpha2

Comment 3

12 years ago
(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

12 years ago
> 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

12 years ago
> 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.
Priority: P2 → P3

Updated

12 years ago
Whiteboard: swag: 2 days

Comment 6

11 years ago
Created attachment 218348 [details] [diff] [review]
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.
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. 
Assignee: annie.sullivan → nobody
Severity: normal → enhancement
Priority: P3 → --
Whiteboard: swag: 2 days
Target Milestone: Firefox 2 alpha2 → ---
(Assignee)

Updated

10 years ago
Severity: enhancement → normal
Target Milestone: --- → Firefox 3 M8
(Assignee)

Updated

10 years ago
Whiteboard: [places-ui]
(Assignee)

Comment 8

10 years ago
Created attachment 276213 [details] [diff] [review]
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
Assignee: nobody → dietrich
Attachment #218348 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 9

10 years ago
Created attachment 276563 [details] [diff] [review]
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
Attachment #276213 - Attachment is obsolete: true
(Assignee)

Comment 10

10 years ago
Created attachment 277126 [details] [diff] [review]
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)
Attachment #276563 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Blocks: 387734
(Assignee)

Updated

10 years ago
Blocks: 387996
(Assignee)

Updated

10 years ago
Blocks: 393873
(Assignee)

Comment 11

10 years ago
Created attachment 278554 [details] [diff] [review]
wip

adds tests
Attachment #277126 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
Created attachment 278602 [details] [diff] [review]
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
Attachment #278554 - Attachment is obsolete: true
Attachment #278602 - Flags: review?(mano)
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.
Attachment #278602 - Flags: review?(mano) → review-
(Assignee)

Comment 14

10 years ago
Created attachment 278919 [details] [diff] [review]
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.
Attachment #278602 - Attachment is obsolete: true
Attachment #278919 - Flags: review?(mano)
-    // 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.
> >+      // 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;
(Assignee)

Comment 17

10 years ago
(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...
(Assignee)

Comment 18

10 years ago
Created attachment 279018 [details] [diff] [review]
fix v3

with comment #16 fixed.
Attachment #278919 - Attachment is obsolete: true
Attachment #279018 - Flags: review?(mano)
Attachment #278919 - Flags: review?(mano)
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.
Attachment #279018 - Flags: review?(mano) → review+
(Assignee)

Comment 20

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

10 years ago
Created attachment 279274 [details] [diff] [review]
as checked in
Attachment #279018 - Attachment is obsolete: true

Updated

10 years ago
Depends on: 394741
I believe this caused bug 394741
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091005 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
We should add test cases for this and related functionality.
Flags: in-litmus?
This feature was dramatically reduced in functionality. We have a testcase in litmus for basic search save.
Flags: in-litmus? → in-litmus+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.