Closed Bug 1016241 Opened 8 years ago Closed 8 years ago

[Collections App] Populate collections with installed apps

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v2.0 verified, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.0 S4 (20june)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: amirn, Assigned: amirn)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

PR
46 bytes, text/x-github-pull-request
ranbena
: review+
Details | Review
1. for predefined collections
2. on first init
3. on app installs
4. on app updates (icon, locale)
5. on app un-installs
blocking-b2g: --- → backlog
feature-b2g: --- → 2.0
Whiteboard: [systemsfe]
Assignee: nobody → amirn
Target Milestone: --- → 2.0 S3 (6june)
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Attached file WIP (obsolete) —
I'd love to get some feedback here (still WIP).

Specifically, when pinning installed bookmarks/apps to collections - what should we pin?
I would prefer saving only the bookmarkURL/manifestURL and fetch any other data when we need to render it (when viewing a collection).

Note that when we implemented pinning web results there was no choice but to save all the data because it isn't stored anywhere else.
Attachment #8436938 - Flags: feedback?(ran)
Attachment #8436938 - Flags: feedback?(kgrandon)
(In reply to Amir Nissim (Everything.me) from comment #1)
> I would prefer saving only the bookmarkURL/manifestURL and fetch any other
> data when we need to render it (when viewing a collection).

I think we can do this either by IAC, or by creating a new database where we would store only the apps displayed on the homescreen.

I prefer the latter, we would have bookmarks_store, collections_store and apps_store. This will allow the collections app to utilize all the processing that is done by the homescreen (processing entry points, hidden roles, locales, icons etc.)

Kevin, what do you think?
Flags: needinfo?(kgrandon)
(In reply to Amir Nissim (Everything.me) from comment #2)> 
> apps_store. This will allow the collections app to utilize all the
> processing that is done by the homescreen (processing entry points, hidden
> roles, locales, icons etc.)

It's interesting and something I thought about before. Entry points are super painful, and we are looking at removing them in the future. Locales are also painful. Datastore may be a way to resolve some of these painpoints, or potentially using a small lightweight library in each application.

I will look at the patch shortly, but I think my preference for now would be to store the minimum, and fetch the data necessary. In the homescreen we only store the detail object for each grid item in indexedDB, and that's enough to get the data that we need. We have some small libraries in the homescreen which links the two up (verticalhome/js/sources/*). Perhaps we can move these into the component, or into shared.
Flags: needinfo?(kgrandon)
Comment on attachment 8436938 [details] [review]
WIP

Overall feedback plus. I will take a deeper look if you mark me for R?. Thanks!
Attachment #8436938 - Flags: feedback?(kgrandon) → feedback+
Attached file PR
steps to test:

0. reset device
1. add music collection
2. open music collection - triggers the population task (aka native_info)
3. no pinned apps shown yet cause the collection was rendered before the task completed
4. close the music collection and open it again (task does not run again)
5. 3 pinned apps appear in the music folder :)

known bug:
when pinning a webresult to the collection, the previously pinned apps are not shown until the collection is closed and opened again

note:
I had to pass a homeIcons object to every |collection.render| call since it now renders homescreen icons. I don't like it, but can't think of any other way of doing this without a homescreen_database.

please take a look at the PR and let me know what you think.

Thanks.
Attachment #8436938 - Attachment is obsolete: true
Attachment #8436938 - Flags: feedback?(ran)
Attachment #8438537 - Flags: review?(ran)
Attachment #8438537 - Flags: review?(kevingrandon)
Attachment #8438537 - Flags: review?(kevingrandon) → review?(kgrandon)
Comment on attachment 8438537 [details] [review]
PR

Overall I think this is looking pretty good. I would like to see another version before R+. Also the first time I installed I noticed some apps appear in a smart collection (with some bugs), but cool! But then I couldn't get it to replicate again. Nice work!
Attachment #8438537 - Flags: review?(kgrandon)
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+]
Comment on attachment 8438537 [details] [review]
PR

Clearing r till PR is updated
Attachment #8438537 - Flags: review?(ran)
Comment on attachment 8438537 [details] [review]
PR

updated PR.

see comment #5 on how to test this.

(In reply to Amir Nissim (Everything.me) from comment #5)
> known bug:
> when pinning a webresult to the collection, the previously pinned homeIcons are
> not shown until the collection is closed and opened again

Kevin, I would love some help with this. I have no idea what's going.

I know the PR is not perfect but it is already quite big so I think we can land it and file followups for missing functionality and bugfixes.

Thanks
Attachment #8438537 - Flags: review?(ran)
Attachment #8438537 - Flags: review?(kgrandon)
Comment on attachment 8438537 [details] [review]
PR

Forwarding review to James.
Attachment #8438537 - Flags: review?(kgrandon) → review?(jlal)
Amir, a bug I found:
1. Add Utilities to homescreen. (or any SM that includes mozApps)
2. Click on it.

Expected: displays mozApps.
Actual: Doesn't but will the next time it's opened.

This may be the same as the comment #8 bug
(In reply to Ran Ben Aharon (Everything.me) from comment #10)
> Amir, a bug I found:
> 1. Add Utilities to homescreen. (or any SM that includes mozApps)
> 2. Click on it.
> 
> Expected: displays mozApps.
> Actual: Doesn't but will the next time it's opened.
> 
> This may be the same as the comment #8 bug

That's not a bug, it's a feature :) see comment 5
I am fairly sure that this is because we simply call setup() without waiting for any pinning. I would recommend either re-rendering after we've done the setup or change the initialization to something like:

NativeInfo.setup().then(function() {
  HandleView(activity)
});
Comment on attachment 8438537 [details] [review]
PR

Clearing the review on James until it's working fully.
Attachment #8438537 - Flags: review?(jlal)
Ok, I think I found the problem. I think this is due to the fact that even if we call clear, existing grid items hold onto their element references, even though they have been removed from the dom: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js

A quick fix for this from the collection app would be to create a new GaiaGrid.Mozapp() object, but I will also investigate if there is an easy fix from the gaia grid.
Depends on: 1026759
This should fix 'mozapps only visible on 2nd open' by running the population task upon creation of new collections:
https://github.com/EverythingMe/gaia/commit/d0aab2926eae2bd56114ff4958053bf7c1ae51f0

sadly it does not work due to [JavaScript Error: "DataCloneError: The object could not be cloned."]

anyone has an idea what this is about?
(In reply to Amir Nissim (Everything.me) from comment #16)
> This should fix 'mozapps only visible on 2nd open' by running the population
> task upon creation of new collections:
> https://github.com/EverythingMe/gaia/commit/
> d0aab2926eae2bd56114ff4958053bf7c1ae51f0

deferring to bug 1027003.
I think this patch is already too big.
Blocks: 1027003
Blocks: 1027005
Attachment #8438537 - Flags: review?(ran) → review+
It does feel a bit off to land this patch without apps being displayed on the first view. I do see we have a patch in the other bug that could address this though, so if we land that other one quickly maybe it's not too bad.
Duplicate of this bug: 1027003
(In reply to Kevin Grandon :kgrandon from comment #18)
> It does feel a bit off to land this patch without apps being displayed on
> the first view. I do see we have a patch in the other bug that could address
> this though, so if we land that other one quickly maybe it's not too bad.

squashed PR for both bugs and closed 1027003 as duplicate of this one.
waiting for Travis/TBPL..
Comment on attachment 8438537 [details] [review]
PR

This is needed for the vertical homescreen.
Attachment #8438537 - Flags: approval-gaia-v2.0?(bbajaj)
Flags: needinfo?(jlorenzo)
Keywords: verifyme
Attachment #8438537 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Verified with dependent bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1027005#c20
Flags: needinfo?(jlorenzo)
Flags: in-moztrap?(jlorenzo)
Verified the bug is fixed on 2.2, 2.1 and 2.0
Collection is populated with install apps 

Device: Flame 2.2 Master KK
BuildID: 20141028040202
Gaia: 6a7fb482a03c5083ef79b41e7b0dfab27527cd04
Gecko: a255a234946e
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
BuildID: 20141028001203
Gaia: a0174f7166745256aaca1cb3aa9f894033fbffa6
Gecko: 43bda3541f6b
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0
BuildID: 20141028000202
Gaia: 5e532a84e762b1bb6231756182cf1465671a061e
Gecko: 124f0bed1700
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+] → [VH-FL-blocking+][VH-FC-blocking+][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
Flags: needinfo?(ktucker)
blocking-b2g: backlog → ---
Flags: in-moztrap?(jlorenzo)
You need to log in before you can comment on or make changes to this bug.