Closed Bug 1117967 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(feature-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S5 (6feb)
feature-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: benfrancis, Assigned: benfrancis)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 2 obsolete files)

46 bytes, text/x-github-pull-request
daleharvey
: review+
Details | Review
7.36 MB, video/mp4
Details
No description provided.
Whiteboard: [systemsfe]
feature-b2g: --- → 2.2+
Assignee: nobody → bfrancis
Target Milestone: --- → 2.2 S5 (6feb)
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.
Attached file WIP Patch (obsolete) —
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.
Attached file Patch (obsolete) —
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.
Attachment #8556055 - Attachment is obsolete: true
Attachment #8556612 - Flags: review?(dale)
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
Attachment #8556612 - Flags: review?(dale)
Comment on attachment 8556612 [details] [review]
Patch

Thanks Dale, have addressed review comments.
Attachment #8556612 - Flags: review?(dale)
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
Attachment #8556612 - Flags: review?(dale)
Attached file Patch
OK, not sure if this is any better.
Attachment #8556612 - Attachment is obsolete: true
Attachment #8558132 - Flags: review?(dale)
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.
OK, I've updated the patch.

I think we may have some broken tests though, waiting for TreeHerder.
Treeherder is green! Dale, I think this is ready for review again.
Flags: needinfo?(dale)
Comment on attachment 8558132 [details] [review]
Patch

Awesome, this is working well thanks
Flags: needinfo?(dale)
Attachment #8558132 - Flags: review?(dale) → review+
https://github.com/mozilla-b2g/gaia/commit/2b83a6d5d1185a438b5bbd287497ac2743b501db
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8558132 [details] [review]
Patch

If this sticks we should uplift to 2.2 as part of the search refactoring committed feature.
Attachment #8558132 - Flags: approval-gaia-v2.2?
Attachment #8558132 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
adding verifyme keyword to be verified on 2.2
Keywords: verifyme
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage?][MGSEI-Triage+]
(In reply to SandKing from comment #17)

> **You can see the same phenomenon with comment 12
Same with comment 15 not comment 12
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?][MGSEI-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Flags: needinfo?(pbylenga)
You need to log in before you can comment on or make changes to this bug.