Closed
Bug 317847
Opened 19 years ago
Closed 17 years ago
Implement "Save this Search"
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 alpha8
People
(Reporter: bugs, Assigned: dietrich)
References
Details
(Whiteboard: [places-ui])
Attachments
(1 file, 8 obsolete files)
43.06 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
This is probably just a button on the query builder (bug 317831)
Depends on: 317831
Updated•19 years ago
|
Priority: -- → P2
Reporter | ||
Comment 2•19 years ago
|
||
-> 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
Reporter | ||
Updated•19 years ago
|
Target Milestone: --- → Firefox 2 alpha2
Comment 3•18 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•18 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•18 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.
Reporter | ||
Updated•18 years ago
|
Priority: P2 → P3
Updated•18 years ago
|
Whiteboard: swag: 2 days
Comment 6•18 years ago
|
||
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.
Reporter | ||
Comment 7•18 years ago
|
||
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.
Updated•17 years ago
|
Assignee: annie.sullivan → nobody
Severity: normal → enhancement
Priority: P3 → --
Whiteboard: swag: 2 days
Target Milestone: Firefox 2 alpha2 → ---
Assignee | ||
Updated•17 years ago
|
Severity: enhancement → normal
Target Milestone: --- → Firefox 3 M8
Assignee | ||
Updated•17 years ago
|
Whiteboard: [places-ui]
Assignee | ||
Comment 8•17 years ago
|
||
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 | ||
Comment 9•17 years ago
|
||
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•17 years ago
|
||
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 | ||
Comment 12•17 years ago
|
||
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 13•17 years ago
|
||
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•17 years ago
|
||
> >- // 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)
Comment 15•17 years ago
|
||
- // 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•17 years ago
|
||
> >+ // 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•17 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•17 years ago
|
||
with comment #16 fixed.
Attachment #278919 -
Attachment is obsolete: true
Attachment #279018 -
Flags: review?(mano)
Attachment #278919 -
Flags: review?(mano)
Comment 19•17 years ago
|
||
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•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•17 years ago
|
||
Attachment #279018 -
Attachment is obsolete: true
Comment 22•17 years ago
|
||
I believe this caused bug 394741
Comment 23•17 years ago
|
||
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091005 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Comment 24•17 years ago
|
||
We should add test cases for this and related functionality.
Flags: in-litmus?
Comment 25•16 years ago
|
||
This feature was dramatically reduced in functionality. We have a testcase in litmus for basic search save.
Flags: in-litmus? → in-litmus+
Comment 26•15 years ago
|
||
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.
Description
•