Closed Bug 1054234 Opened 10 years ago Closed 10 years ago

[Flatfish][Rocket bar] In search page, typing 'p' text will appear Phone app.

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: eva.chen.fx, Assigned: chens)

References

Details

(Whiteboard: [Flatfish][TCP])

Attachments

(4 files)

Attached image Bug_Phone App.jpg
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36

Steps to reproduce:

1. Update a Flatfish to BuildID: 20140815012352
2. Open Settings app.
3. Tap the Rocket bar.
   (Rocket bar: The left side of the Rocket bar displays the title of the current app.)
4. Typing 'p' text into the 'Search or enter address' fields.
-----------------------------------------------
Environment Info:
BuildID: 20140815012352
Gaia: e57ea93196e997336934d6e47323755c5bdd80e9
Gecko:1e2086d3a6f473b2437f25a4a9226b6c2d6c2605
B2G: 2.1.0.0-prerelease
Platform version : 34.0a1
Repro frequency: 100%
See attached: Bug_Phone App.jpg


Actual results:

<ISSUE> In search page, typing 'p' text will appear Phone app. The Flatfish doesn't support telephony, so that is incorrect.
Blocks: flatfish
Whiteboard: [Flatfish][TCP]
Attached file WIP
Github pull request: https://github.com/mozilla-b2g/gaia/pull/22969
Assignee: nobody → shchen
Attachment #8474354 - Flags: feedback?(kgrandon)
Hi Kevin, 

I found in the original homescreen we will check each app's launch path to determine to process this app or not, do we have to handle this in the vertical one? 

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L1540-L1549
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L1162
Flags: needinfo?(kgrandon)
Comment on attachment 8474354 [details] [review]
WIP

Wow, this setting is a giant hack. We really need to get rid of entry_points so we can get rid of it.

If you're going to do it, don't do it in every search. Instead do it outside of the autocomplete flow: https://github.com/shamenchens/gaia/blob/Bug1054234-SearchBlacklistApp/apps/search/js/providers/local_apps.js#L21
Attachment #8474354 - Flags: feedback?(kgrandon)
(In reply to Sherman Chen [:chens] from comment #2)
> Hi Kevin, 
> 
> I found in the original homescreen we will check each app's launch path to
> determine to process this app or not, do we have to handle this in the
> vertical one? 

Perhaps you could add this logic here: https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/sources/application.js#L199
Flags: needinfo?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #3)
> If you're going to do it, don't do it in every search. Instead do it outside
> of the autocomplete flow:
> https://github.com/shamenchens/gaia/blob/Bug1054234-SearchBlacklistApp/apps/
> search/js/providers/local_apps.js#L21

I found it's not sufficient to do it outside the autocomplete flow, within the find function it will still looking for entry_points for each app: https://github.com/shamenchens/gaia/blob/Bug1054234-SearchBlacklistApp/apps/search/js/providers/local_apps.js#L94-L99

Looks like it also handle inside find function or to update the key to store the app, I will provide another WIP patch later.
Flags: needinfo?(kgrandon)
Ok, my recommendation is that we should build appListing externally to the find() function. Are we in any kind of rush here? I assume that 2.1 on tablet is only for developers at this point?
Flags: needinfo?(kgrandon)
Ah, I got your point, will try to separate appListing later.
And you are right, currently 2.1 on tablet is for developer only so it's no rush here.
I'm just curious about what things can be done on tablet :)
Attached file WIP 2
Hi Kevin, WIP updated to separate appListing and add unit test.
Could you take a look and give some feeback? thanks.

Github pull request: https://github.com/mozilla-b2g/gaia/pull/23030
Attachment #8475041 - Flags: feedback?(kgrandon)
Comment on attachment 8475041 [details] [review]
WIP 2

Not happy about having to hit settings again, but this approach should be fine. Thanks.
Attachment #8475041 - Flags: feedback?(kgrandon) → feedback+
Comment on attachment 8475041 [details] [review]
WIP 2

Pull request updated, could you review this one?
Attachment #8475041 - Flags: review?(kgrandon)
Comment on attachment 8475041 [details] [review]
WIP 2

Left a few comments on github. Let me know if you have any questions.
Attachment #8475041 - Flags: review?(kgrandon)
Attached file WIP 3
Hi Kevin, 

Patch updated and comments are addressed, could you take a look and give feedback? thanks.

Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23130
Attachment #8476473 - Flags: feedback?(kgrandon)
Comment on attachment 8476473 [details] [review]
WIP 3

Ok, this is much better, and should be quite performant - thanks! I didn't see anything wrong with this and it seemed to work, so I'm fine leaving an R+ here.

I do want to be clear that this is a terrible hack and needs to go away. I've filed bug 1056874 so we can track this.
Attachment #8476473 - Flags: feedback?(kgrandon) → review+
Merged to master:
https://github.com/mozilla-b2g/gaia/commit/02e61f6054717699f773448907707f5246f863f6
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Verified okay and close the bug.
Enviromental Variables:
----------------------------------------------------
Device  : Flatfish 2.1 Master
B2G     : 2.1.0.0-prerelease
Gaia    : b5aed82b1d8750a53848479b6400902d920d1475
          Merge: 553f796 7a06c70
          Author: Cristian Rodriguez <crdlc@tid.es>
          Date:   Thu Aug 28 08:21:36 2014 +0200
Gecko   : 64c4bef1c1234d2fdd60974ed30d1034ec570c34
BuildID : 20140821013807
Version : 34.0a1
Status: RESOLVED → VERIFIED
Depends on: 1114493
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: