1117967 - Show a single line of combined local apps, bookmarks and Marketplace results

Status: VERIFIED FIXED

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

Comment 1

[Ben Francis [:benfrancis]]

I can restrict the number of results to four using a hack on GridProvider, but this wouldn't create the scrollable UI the spec asks for.

Spoke to Kevin and it looks like we need to stop using <gaia-grid> here and create something custom instead.

Comment 2

[Ben Francis [:benfrancis]]

Attachment: WIP Patch

Here's a work in progress patch which simply limits the grid to 4 items.

This doesn't achieve the horizontal scrolling behaviour required in the spec, but I think that's going to require some serious refactoring of GaiaGrid which we might want to consider doing in a follow-up.

I think implementing the scrolling UI will require either a) adding a horizontal scrolling mode to <gaia-grid> or b) creating a <gaia-icon> component to reuse in both <gaia-grid> and a new IconProvider, in order to reuse all the code in the various GridItems.

Limiting the grid to 4 items also doesn't meet the spec with regards to landscape mode showing more icons, but that's already limited to 4 wide so at least this isn't a regression.

Comment 3

[Ben Francis [:benfrancis]]

Attachment: Patch

OK, I think this is ready for review.

There are some trade-offs in this patch. The apps results area has a fixed height to reduce flicker but means there's quite a gap before the next results section when icons only have one line labels. Also, the apps results area is hidden when there are no results, but this happens per grid provider which means that if the first provider returns no results it will get hidden but shown again if subsequent providers return results. Because all the providers return asynchronously there's no easy way around this without waiting to show results until all providers have returned.

The horizontal scrolling feature will happen in follow-up bug 1127369, but this patch at least reduces the results down to one line so that we can start to see the new results formatting taking shape.

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 8556612 [details] [review]
Patch

Few nits I would like to see addressed, this looks good and agree with the approach to limit the gaia-grid to 4 and then work on horizontal scrolling

Comment 5

[Ben Francis [:benfrancis]]

Comment on attachment 8556612 [details] [review]
Patch

Thanks Dale, have addressed review comments.

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8556612 [details] [review]
Patch

Yeh putting any of this logic in grid_provider which is a base class for each of the providers is messy and possibly even wrong (if grid.add were to be synchronous), I cant think of what the problem adding it to search.js would be but I can help out with it. search.js handles deduplication and is the singleton that all the providers run through so its the right place to handle that logic

That aside the rest looks good, thanks

Comment 7

[Ben Francis [:benfrancis]]

Attachment: Patch

OK, not sure if this is any better.

Comment 8

[Dale Harvey (:daleharvey)]

Ben, how about 

    collect: function(provider, results) {

      if (provider.remote) {
        this.loadingElement.classList.remove('loading');
      }

      if (provider.dedupes) { 
        results = this.dedupe.reduce(results, provider.dedupeStrategy);
      }

      if (results.length + shownResults > MAX_RESULTS) { 
        var spaces = MAX_RESULTS - shownResults;
        if (spaces < 1) { 
          return; // abort too?
        }
        results.splice(spaces, (results.length - spaces));
      }

      shownResults += results.length;
      provider.render(results);
    },

Remembering to clear shownResults in `clear`, 

It keeps the dedupe logic together with the count logic and doesnt depend on any implementation details of the grid so we can not render to gaia-grid and it doesnt need to change.

Comment 9

[Ben Francis [:benfrancis]]

OK, I've updated the patch.

I think we may have some broken tests though, waiting for TreeHerder.

Comment 10

[Ben Francis [:benfrancis]]

Treeherder is green! Dale, I think this is ready for review again.

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8558132 [details] [review]
Patch

Awesome, this is working well thanks

Comment 12

[Ben Francis [:benfrancis]]

https://github.com/mozilla-b2g/gaia/commit/2b83a6d5d1185a438b5bbd287497ac2743b501db

Comment 13

[Ben Francis [:benfrancis]]

Comment on attachment 8558132 [details] [review]
Patch

If this sticks we should uplift to 2.2 as part of the search refactoring committed feature.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/935f9c99ef55c1f57e54a4bbfaabd2efc89feb94

Comment 15

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue is verified fixed on Flame 3.0

If the user conducts a search in the search bar they will only see 4 apps total of any combination from combined local apps, bookmarks and Marketplace results. Below the apps is a search list based on what search provider is currently being used.

Environmental Variables:
Device: Flame 3.0 (319mb)(Kitkat)(Full Flash)
Build ID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment 16

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

adding verifyme keyword to be verified on 2.2

Comment 17

[SandKing]

Attachment: Verified video: verify_1117967.mp4

This issue has been verified successfully on latest Flame2.2
STR:
1. Launch Serch bar.
2. Input some keywords.
**You can see the same phenomenon with comment 12
Verify video:" verify_1117967.mp4".

Flame 2.2 build:
Build ID               20150210002516
Gaia Revision          b30c8e4303595a0fcb5b640d673cf8503b954701
Gaia Date              2015-02-10 04:09:47
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3e9fa4e70a1b
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150210.041059
Firmware Date          Tue Feb 10 04:11:10 EST 2015
Bootloader             L1TC000118D0

Comment 18

[SandKing]

(In reply to SandKing from comment #17)

> **You can see the same phenomenon with comment 12
Same with comment 15 not comment 12