Closed Bug 1009358 Opened 7 years ago Closed 7 years ago

[User Story] Rocketbar: Ability to choose search provider used on Enter press


(Firefox OS Graveyard :: Gaia::Settings, defect, P3)



(feature-b2g:2.0, b2g-v2.0 verified)

2.0 S3 (6june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- verified


(Reporter: pdol, Assigned: benfrancis)



(Keywords: feature, Whiteboard: [ucid:System194, ft:systemsfe, 2.0])

User Story

As a user, I want to be able to change the search provider used upon Enter press in Rocketbar to allow me to pick a search engine that givers me the best results for my search use cases.

Acceptance Criteria:
1. I am able to choose the search provider to be used on Enter press from a list provided by the OEM/operator.
2. Upon choosing a new provider from the list in AC1, when Enter is pressed in Rocketbar after a string is in the search field, I am taken to search results from the selected provider. (other than when a url is entered)
3. Implementation matches UX spec.


(2 files)

No description provided.
User Story: (updated)
User Story: (updated)
It may make sense to pull the settings for this out of the existing Browser into Settings so that the user choice applies to both the current Browser and Rocketbar. (assuming that is possible)
feature-b2g: --- → 2.0
Un-assigning from myself until homescreen work is finished.
Assignee: kgrandon → nobody
Priority: -- → P3
Assignee: nobody → bfrancis
Target Milestone: --- → 2.0 S3 (6june)
Adding homescreen settings page which includes the search engine selector.
Ben - is it realistic that we will have this done in 2.0? I think going with a configured provider (instead of user-selected) is slightly less work for 2.0 and something we were originally planning on doing. I'm just wondering if we should move this into 2.1, but keep bug 1009353 in 2.0.
Flags: needinfo?(bfrancis)
Hi Kevin, we discussed this on Vidyo yesterday. Bug 1009353 is the first priority and this one is higher risk but would be nice to have for 2.0 as well. They're both in the backlog, let's see what we get done.
Flags: needinfo?(bfrancis)
Flags: in-moztrap?(jsmith)
Submitting this WIP patch for some feedback from Taipei in the hope that Evelyn or Arthur are still around to give some feedback before they go home.

I still need to write tests for this patch but wanted to check I'm going in the right direction so we can get it landed by feature landing.
Attachment #8434875 - Flags: feedback?(ehung)
Attachment #8434875 - Flags: feedback?(arthur.chen)
Comment on attachment 8434875 [details] [review]

Overall it looks good to me. However, I still have concerns on storing the provider list in the settings DB as the list is not user preference.
Attachment #8434875 - Flags: feedback?(arthur.chen) → feedback+
Thanks for the feedback Arthur, I have updated the patch based on your comments.

With regards to using DataStore instead of Settings API, we had a long discussion about this yesterday and I suggested the same thing (maintaining a DataStore of search providers), but in the end I was persuaded to use the settings API instead. The reasons are:
1) We populate the setting at build time based on customisations (see bug 1009353) which we can't do with DataStore AFAIK
2) DataStore has no mechanism to get a cursor over a list of entries and is intended to be used in conjunction with IndexedDB. Creating an IndexedDB database and a DataStore and keeping them synchronised just for this one thing seems like overkill. daleharvey argued against it.
I have filed bug 1020920 to allow DataStores to be populated at build time, like we can with settings, so that in future we don't have to use the Settings API like this.
Comment on attachment 8434875 [details] [review]

I've added tests, now ready for review. Hopefully we can get this landed tomorrow (Friday) in time for Feature Landing :)
Attachment #8434875 - Flags: feedback?(ehung) → review?(arthur.chen)
EJ, would you be able to review this so Ben could reach FL here?
Flags: needinfo?(ejchen)
Comment on attachment 8434875 [details] [review]

Thanks for the review Vivien! I have addressed your comments.
Attachment #8434875 - Flags: review?(arthur.chen) → review?(21)
Comment on attachment 8434875 [details] [review]

r+ with nits fixed and a followup bug to create a VerticalHomescreen.searchEngines object that contains itself.

It will make the code more isolated and easier to read.
Attachment #8434875 - Flags: review?(21) → review+
Ben just to check, this currently doesnt implement the icon settings right? thats something I am gonna need in
Also dont worry about implementing the UI for the icons in the search app at least, I can do that in, but will be blocked by the icon setting being populated
Note: Waiting for green Travis to land, there's what looks like a perma fail on vertical homescreen making the tree red :(
Dale, no this doesn't implement the settings entry point in the search app (in the form of an icon). This is intentional because that version of the spec is only supposed to be implemented if we support multiple search suggestion providers (bug 1021779).

We are storing the search.suggestionUrlTemplate setting, but it is not yet being used because we need a simple opensearch suggestions implementation in order to build the front end for this, and I didn't quite have time to do that before the end of the week. (We basically need a small opensearch library to sit alongside the EverythingMe one and use OpenSearch suggestions instead of the EverythingMe API if EverythingMe isn't set as the current search engine).

Currently suggestions are always provided by EverythingMe so it would be misleading to display the icon of another search engine next to those suggestions.
Oh and also yes, the setting in the Settings app currently only changes the search.urlTemplate setting, it doesn't set search.iconUrl or search.suggestionsUrlTemplate which is maybe what you were asking?
Closed: 7 years ago
Resolution: --- → FIXED
Thanks for the review, Vivien :)
Flags: needinfo?(ejchen)
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
Flags: in-moztrap?(jsmith) → in-moztrap?(nhirata.bugzilla)

Gaia      8df02268fcd7e80c5fab8c1ec099772e37f8659d
BuildID   20140627000201
Version   32.0a2 Jun 16 16:51:29 CST 2014
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.