1016241 - [Collections App] Populate collections with installed apps

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Amir Nissim (:amirn)]

1. for predefined collections
2. on first init
3. on app installs
4. on app updates (icon, locale)
5. on app un-installs

Comment 1

[Amir Nissim (:amirn)]

Attachment: WIP

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.

Comment 2

[Amir Nissim (:amirn)]

(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?

Comment 3

[Kevin Grandon :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.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8436938 [details] [review]
WIP

Overall feedback plus. I will take a deeper look if you mark me for R?. Thanks!

Comment 5

[Amir Nissim (:amirn)]

Attachment: 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.

Comment 6

[Kevin Grandon :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!

Comment 7

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8438537 [details] [review]
PR

Clearing r till PR is updated

Comment 8

[Amir Nissim (:amirn)]

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

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8438537 [details] [review]
PR

Forwarding review to James.

Comment 10

[Ran Ben Aharon [:ranbena] EverythingMe]

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

Comment 11

[Amir Nissim (:amirn)]

(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

Comment 12

[Kevin Grandon :kgrandon]

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 13

[Kevin Grandon :kgrandon]

Comment on attachment 8438537 [details] [review]
PR

Clearing the review on James until it's working fully.

Comment 14

[Kevin Grandon :kgrandon]

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.

Comment 15

[Kevin Grandon :kgrandon]

Sorry, meant to link to this line: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L306

Comment 16

[Amir Nissim (:amirn)]

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?

Comment 17

[Amir Nissim (:amirn)]

(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.

Comment 18

[Kevin Grandon :kgrandon]

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.

Comment 20

[Amir Nissim (:amirn)]

(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 21

[Amir Nissim (:amirn)]

landed in master: https://github.com/mozilla-b2g/gaia/commit/75ecfdd5dc3882a95cff6ff8f2bf4ce9db823d64

tbpl: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=0f76c48edacd3ece333be4e033d952bd030c7699

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8438537 [details] [review]
PR

This is needed for the vertical homescreen.

Comment 23

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/9a50c4b12f3c26891b34188befa3a05ca4777d48

Comment 24

[Johan Lorenzo [:jlorenzo]]

Verified with dependent bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1027005#c20

Comment 25

[sarsenyev]

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