942175 - [Rocketbar] Add a search bar at the system level

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dirty.poc.searchbar.patch

The goal is to have a search bar in the system app that can forward search terms to a third party application that acts as a renderer for search results. The search application should be able to communicate back the search bar in order to provide suggestions, or inform the system about the user choice.

The attached patch is a very dirty poc that uses the inter-app-communication protocol in order to reuse that. The homescreen has a new search.html page that is used as the 'render' search app.

Something similar that use a separate app directly should be fine. Then a preference in order to enable/disable this feature needs to be added, pref'ed off by default.

The setting can just disable the click listener on the top-left corner of the screen that is actually used to display the search bar.

Kevin, please don't look too much into the detail of the code. The interesting part is the IAC communication and the use of a separate app to render result. I'm mostly asking f? to make sure that you don't miss this bug :)

Comment 1

[Kevin Grandon :kgrandon]

Comment on attachment 8336829 [details] [diff] [review]
dirty.poc.searchbar.patch

Review of attachment 8336829 [details] [diff] [review]:
-----------------------------------------------------------------

I like the concept, but we obviously need to get more people involved here :) I do think that landing something would be good, and we can easily iterate or tear it out if needed.

I think I'll take your concept and build on top of it a little bit this weekend if that's ok with you.

Comment 2

[Kevin Grandon :kgrandon]

Attachment: Pull request - Rocketbar and search app communication

Hi Vivien - Here's your patch with a little bit of polish. Marking you as f? so you are aware, and can take my changes if you want. The main changes are:

1 - Functionality is hidden behind a 'rocketbar.enabled' preference. Without it, the normal utility bar appears.

2 - Code moved out of utility_tray and into rocketbar.js.

3 - Results app now in a standalone app at apps/search.

I may continue to play with this over the weekend, and do some more polishing.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

<3 it.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Kevin, for the first version let's keep the search panel into the homescreen. That will make e.me transition simpler and will offer us more time to fix some of the multiple new issues spawned by this change.

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

Hi Vivien - Marking you as reviewer on this one. I've added another commit which moves the search/ code into the homescreen app, and launches that. 

I like this approach because:

1 - It enforces separation of logic which makes it easy to move code around in the future.

2 - It should have the same performance benefits of keeping things in the homescreen app.

Please let me know what you think!

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

A few nits but that should be good. Thanks :)

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

Hi Ben - 

We have a prototype here which shows a search page instead of the utility tray. This functionality is hidden behind a pref (and can be accessed from the settings app). There are currently no search "providers" besides contacts, but as a next step we can add E.me, open search, etc.

We currently copy the search app into the homescreen app for performance reasons and being able to load E.me fairly fast. In the future we may be able to have a purely standalone application.

We'd like to land this asap, but I definitely wanted your feedback before we do so. Please let me know if you have any concerns. Thanks!

Comment 8

[Ben Francis [:benfrancis]]

Cool prototype.

Let's discuss this in the Rocketbar sync on Thursday and have a proper discussion about architecture before landing anything.

Note that all the awesomebar use cases which need porting over to the Rocketbar are being tracked in the System::Browser Chrome component https://bugzilla.mozilla.org/buglist.cgi?product=Firefox%20OS&component=Gaia%3A%3ASystem%3A%3ABrowser%20Chrome&resolution=---&list_id=8767732

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

(In reply to Ben Francis [:benfrancis] from comment #8)
> Cool prototype.
> 
> Let's discuss this in the Rocketbar sync on Thursday and have a proper
> discussion about architecture before landing anything.

Sounds good - thanks for the information. Going to clear the feedback? flag until our path is more clear, but feel free to leave any feedback/concerns for this.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ben Francis [:benfrancis] from comment #8)
> Cool prototype.
> 
> Let's discuss this in the Rocketbar sync on Thursday and have a proper
> discussion about architecture before landing anything.

It has already been discussed into 2 meetings and I have not seen any arguments/features that makes sense against this architecture. Let's stop talking and start coding...

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #10)
> (In reply to Ben Francis [:benfrancis] from comment #8)
> > Cool prototype.
> > 
> > Let's discuss this in the Rocketbar sync on Thursday and have a proper
> > discussion about architecture before landing anything.
> 
> It has already been discussed into 2 meetings and I have not seen any
> arguments/features that makes sense against this architecture. Let's stop
> talking and start coding...

What meetings are you talking about? The UX and product meetings? I can't remember talking about the implementation in any of those meetings.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Gregor Wagner [:gwagner] from comment #11)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> 09/12/13) from comment #10)
> > (In reply to Ben Francis [:benfrancis] from comment #8)
> > > Cool prototype.
> > > 
> > > Let's discuss this in the Rocketbar sync on Thursday and have a proper
> > > discussion about architecture before landing anything.
> > 
> > It has already been discussed into 2 meetings and I have not seen any
> > arguments/features that makes sense against this architecture. Let's stop
> > talking and start coding...
> 
> What meetings are you talking about? The UX and product meetings? I can't
> remember talking about the implementation in any of those meetings.

Really ? We didn't heard the same thing then. I explained twice that the prototype I have is about having a separate app / page that does the rendering, with the code in Gaia, and that what this prototype is about, either we do B or C (since the difference they have is more about a philosophical difference that does not makes sense yet). There was A, as an option but my understanding was UX would like B/C, and A seems more a work around to me.

That's also what I told in the WW in SF to Ben when it was about implementing the proposition at that point, since there is no way we can run 2 separate apps with the resource constraints. Or just that having UI in general in the system app as always been very painful and is something to avoid,  in order to not leaks, or not prioritized this too much (network request from here, versus requests from content), or CPU resources...

Also at this level the architecture is pretty simple, a separate app, of which the internal architecture is out of the scope of this bug.

And in the worst case, we are not married with any solution, but iterating over a solution is probably the best way to see if it fits or not, and see which adjustment we need to do, or if we need something completely different.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I will not be surprised if what I tried to explained was crystal clear to me and impossible to get for others. I opened bug 946120 in order to fix that.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Gregor, I have a doubt. Are we speaking of the same thing or does the title of this bug is misleading and makes you feel this is the same as bug 942175 ?

Comment 15

[Kevin Grandon :kgrandon]

Vivien's architecture and proposal were clear - what was not clear to me, and is still not clear to me was the path forward. There are certainly some communication problems if Vivien, Ben, and myself are all on different pages.

I feel like we should have gathered clear actionable items from earlier meetings, and we would not be in this state.  I don't think English is to blame, but I do think we can structure our meetings and strive to have everyone on the same page in a more efficient manner. Because of this, I am unblocking on 946120.

Let's try to be constructive about this feature from here on out and not how we carried out meetings in the past if possible.

Comment 16

[Kevin Grandon :kgrandon]

I would rather not land this until everyone is on board with it, but: 

1 - I don't see harm in landing this early as it's disabled by default and behind a preference.

2 - It will not collide with other work, and it's trivial to integrate it into any other branch/search bar if necessary. I will sign up for this work.

3 - It's always best to land something, then iterate. Due to the split architecture of this, we can even iterate on multiple search bar implementations, and converge at a later date.

4 - It's a standalone app, so it's trivial to back this out.

If you have any concerns/disagreements about any of the above four points, please let me know.

Comment 17

[Ben Francis [:benfrancis]]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

I think the basic architecture of having the Rocketbar talk to a search app over the inter-app communication API is the right architecture to explore and the broad UX direction (the Rocketbar is like an expanded status bar and can be pulled down from the top) is the direction the UX team seems to be going in.

However, I don't think we should land this yet.

1. The mechanism for copying the search app into the homescreen app at build time is super hacky. Let's either make a standalone search app or make it part of the homescreen app, not both.
2. The problem with making search part of the homescreen app is that the homescreen app is meant to be user replaceable. If the user replaces the homescreen app, search stops working.
3. The main argument for making search part of the homescreen app seems to be to ease the migration of all the e.me code to the search app but we don't even know if that's going to happen yet, it's still under discussion.
4. It exposes a user-facing option in Settings to turn on broken UI which we shouldn't land in master, and definitely not before 1.3 branches.
5. Contacts search is not currently a priority for 1.4, we should concentrate on the Awesomebar use cases first. The opening a contact web activity doesn't seem to work for me anyway.
6. The latest I've heard from UX is that pulling down the Rocketbar should not focus it because the unfocused view may be used for the task manager.
7. There is a non-zero risk of this causing regressions in 1.3 and landing new features in 1.3 has to be approved by Fabrice at the moment.

Let's develop this feature in a feature branch until enough of it is implemented that it can stand up on its own (e.g. doesn't break notifications!)

There's nothing wrong with experimenting with multiple prototypes on feature branches before deciding on a final direction but please don't experiment in the master branch of a commercial product.

Comment 18

[Gregor Wagner [:gwagner]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #12)
> (In reply to Gregor Wagner [:gwagner] from comment #11)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until
> > 09/12/13) from comment #10)
> > > (In reply to Ben Francis [:benfrancis] from comment #8)
> > > > Cool prototype.
> > > > 
> > > > Let's discuss this in the Rocketbar sync on Thursday and have a proper
> > > > discussion about architecture before landing anything.
> > > 
> > > It has already been discussed into 2 meetings and I have not seen any
> > > arguments/features that makes sense against this architecture. Let's stop
> > > talking and start coding...
> > 
> > What meetings are you talking about? The UX and product meetings? I can't
> > remember talking about the implementation in any of those meetings.
> 
> Really ? We didn't heard the same thing then. I explained twice that the
> prototype I have is about having a separate app / page that does the
> rendering, with the code in Gaia, and that what this prototype is about,
> either we do B or C (since the difference they have is more about a
> philosophical difference that does not makes sense yet). There was A, as an
> option but my understanding was UX would like B/C, and A seems more a work
> around to me.

I remember the first sentence in the meeting being something like: Lets talk about the UX part
and leave the implementation discussions out here. So I guess thats where my confusion is coming from.
I wanted to know if there were other deeper technical meetings.

> 
> That's also what I told in the WW in SF to Ben when it was about
> implementing the proposition at that point, since there is no way we can run
> 2 separate apps with the resource constraints. Or just that having UI in
> general in the system app as always been very painful and is something to
> avoid,  in order to not leaks, or not prioritized this too much (network
> request from here, versus requests from content), or CPU resources...
> 
> Also at this level the architecture is pretty simple, a separate app, of
> which the internal architecture is out of the scope of this bug.


Sure, the search bar itself is not the problem and I was more interested about the internal
architecture. Whats the right bug for this?

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ben Francis [:benfrancis] from comment #17)
> Comment on attachment 8337131 [details] [review]
> Pull request - Rocketbar and search app communication
> 
> I think the basic architecture of having the Rocketbar talk to a search app
> over the inter-app communication API is the right architecture to explore
> and the broad UX direction (the Rocketbar is like an expanded status bar and
> can be pulled down from the top) is the direction the UX team seems to be
> going in.
> 
> However, I don't think we should land this yet.
> 
> 1. The mechanism for copying the search app into the homescreen app at build
> time is super hacky. Let's either make a standalone search app or make it
> part of the homescreen app, not both.

This is really not an issue, we can followup on that.

> 2. The problem with making search part of the homescreen app is that the
> homescreen app is meant to be user replaceable. If the user replaces the
> homescreen app, search stops working.

Sure. But until the homescreen is not replaceable that does not matter.

> 3. The main argument for making search part of the homescreen app seems to
> be to ease the migration of all the e.me code to the search app but we don't
> even know if that's going to happen yet, it's still under discussion.

The main argument is about prototyping with our partners. E.me is concerned about load time, so we want to start implementing a rendering UI in a homescreen page and have a way to turn on a separate app in order to profile.

> 4. It exposes a user-facing option in Settings to turn on broken UI which we
> shouldn't land in master, and definitely not before 1.3 branches.

It's trivial to do a patch to remove the setting in 1.3 once we branch if that's an issue. Then other people will be able to try it and play with it, and realize how broken it is right now and maybe propose some fixes.

> 5. Contacts search is not currently a priority for 1.4, we should
> concentrate on the Awesomebar use cases first. The opening a contact web
> activity doesn't seem to work for me anyway.

There is no issue about removing it. It was just used as a datasource to prototype since close to nothing has moved to datastore yet.

> 6. The latest I've heard from UX is that pulling down the Rocketbar should
> not focus it because the unfocused view may be used for the task manager.

The latest I heard is that pulling down the statusbar is not the access point to the rocket bar. Clicking on the statusbar is.
 
> 7. There is a non-zero risk of this causing regressions in 1.3 and landing
> new features in 1.3 has to be approved by Fabrice at the moment.
>

There is a non-zero risk for everything but this stuff does not change that much of the current logic. I think there is also a difference versus new feature that are used, and feature that are disabled by default.
 
> Let's develop this feature in a feature branch until enough of it is
> implemented that it can stand up on its own (e.g. doesn't break
> notifications!)
> 

The way it is done won't break notifications and I don't see why we should use a branch instead of master. Big landing is always a pain. 

> There's nothing wrong with experimenting with multiple prototypes on feature
> branches before deciding on a final direction but please don't experiment in
> the master branch of a commercial product.

Don't land a separate app until it is completely done? Seems completely off with the rest of the codebase ;)

Comment 20

[Ben Francis [:benfrancis]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #19)
> 
> > 2. The problem with making search part of the homescreen app is that the
> > homescreen app is meant to be user replaceable. If the user replaces the
> > homescreen app, search stops working.
> 
> Sure. But until the homescreen is not replaceable that does not matter.

OK, but we are just storing up problems for the future by making the system app and homescreen app even more tightly coupled.

> The main argument is about prototyping with our partners. E.me is concerned
> about load time, so we want to start implementing a rendering UI in a
> homescreen page and have a way to turn on a separate app in order to profile.

Prototyping sounds like a good idea. Landing a "proof of concept" prototype in the master branch sounds like a less good idea.

I was told to focus on awesomebar use cases and assume the e.me code is separate until a decision is made. It seems you're working from a different set of requirements.

> > 4. It exposes a user-facing option in Settings to turn on broken UI which we
> > shouldn't land in master, and definitely not before 1.3 branches.
> 
> It's trivial to do a patch to remove the setting in 1.3 once we branch if
> that's an issue.

Checking in known broken code then filing a bug to remove it in a branch sounds like a strange way of working, but OK.

> There is no issue about removing it. It was just used as a datasource to
> prototype since close to nothing has moved to datastore yet.

Again, fine for a prototype, but not in master if that code isn't intended to ship.

I agree that having no data to search is a bit of a pain! I'm trying to figure out how the browser's places database can be shared between system, homescreen and search apps after it is migrated. It's going to be tricky.

> > 6. The latest I've heard from UX is that pulling down the Rocketbar should
> > not focus it because the unfocused view may be used for the task manager.
> 
> The latest I heard is that pulling down the statusbar is not the access
> point to the rocket bar. Clicking on the statusbar is.

My understanding is that pulling down the status bar shows the rocketbar in an un-focussed state with the task manager underneath, then the user can tap the text box to focus the rocketbar to search. Tapping the status bar is a shortcut to open the rocketbar in a focussed state. I guess this is all still in flux though and the task manager currently lives elsewhere so this isn't too important.

> There is a non-zero risk for everything but this stuff does not change that
> much of the current logic. I think there is also a difference versus new
> feature that are used, and feature that are disabled by default.

As long as there's no user-facing way to turn it on I could agree. I assume it will still need sheriff approval to land though.

> > Let's develop this feature in a feature branch until enough of it is
> > implemented that it can stand up on its own (e.g. doesn't break
> > notifications!)
> > 
> 
> The way it is done won't break notifications and I don't see why we should
> use a branch instead of master. 

If you enable the option in settings, you can no longer access your notifications.

> Don't land a separate app until it is completely done? Seems completely off
> with the rest of the codebase ;)

I don't think we need to finish the whole app, just not leave the UI in a broken state.

FWIW I am really pleased to see some code written on this, we have to start somewhere! It would be great if we had clear requirements and a specification before we started coding but it seems that isn't going to happen until at least the second sprint of 1.4, so we can at least make sure we are all going in the same basic direction :)

If you can figure out a safe way to land this which won't result in broken UI on production devices then let's do that and use it as a starting point. In the absense of clear requirements it's difficult for me to argue against the design decisions. I will stash my current branch and try to build the awesomebar use cases on top of this instead.

Comment 21

[Kevin Grandon :kgrandon]

Attachment: Pull request to feature branch

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8337131 [details] [review]
Pull request - Rocketbar and search app communication

Obsoleting in favor of working with a feature branch for now.

Comment 23

[Kevin Grandon :kgrandon]

This has landed in the new 'rocketbar' feature branch here: https://github.com/mozilla-b2g/gaia/commit/a320cb0a515b28826ddb3c835a19863c00431524

The feature branch will be short-lived, and useful while we wait for 1.3 to branch.

Comment 24

[Ben Francis [:benfrancis]]

In the example search app implementation here, selecting a search suggestion fires a web activity to open the result (in this case a contact).

We could do it this way (and use a view URL web activity for web pages) but it requires having special knowledge to handle results from different sources.

What if every result was just a URL? Could we just call window.open on that URL to launch the result regardless of its source? 

One challenge would be knowing when to set the mozapp attribute of the iframe created by window.open, i.e. whether the result was from a web site or a web app. It's possible that we could store app manifest URLs in the Places database to record the fact that a particular place corresponds to a particular app. Or perhaps there is another way to determine whether a given URL belongs to an installed app? We may have to re-think the relationship between URLs and app windows anyway if we go forward with the "everything is a bookmark" content model.

Comment 25

[Kevin Grandon :kgrandon]

Resolving bugs which have landed in the rocketbar branch to clean up the dependency tree. All commits in the branch have been reviewed, and we can merge it into master at any point.