1041295 - [Collection App] Icons are not updated after installing packaged apps from marketplace

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Amir Nissim (:amirn)]

STR:

0) reset your device
1) Go to Marketplace > Games
2) Install 'Cut the Rope'
3) Go to vertical home after installing this game
4) Observe the 'Games' icon

Expected:
'Games' icon should show 'cut the rope' as the middle app icon

Actual:
'Games' show an blank icon as the middle app icon


More information:

For packaged apps, the app icon is a local asset downloaded from the web.

The 'install' event fires too early:
https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/js/synchronize.js#L33

at this point the app hasn't finished downloading and retrieving the icon fails.


Suggested fix:
The event should be fired only when the app after download completes successfully.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Jacqueline, would UX block on that bug?

Comment 2

[Amir Nissim (:amirn)]

Attachment: WIP

this patch defers the 'install' event to after 'dowloadapplied'. 2 things are not working at the moment:

1. 'downloadapplied' is not fired for hosted apps, so the 'install' event isn't triggered as well. not sure what the right hook for hosted apps should be.

2. homescreen is not updated when the new icon is saved to the CollectionsDatabase and still shows the old icon

Comment 3

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (:amirn) from comment #2)
> Created attachment 8460232 [details] [review]
> WIP
> 
> this patch defers the 'install' event to after 'dowloadapplied'. 2 things
> are not working at the moment:
> 
> 1. 'downloadapplied' is not fired for hosted apps, so the 'install' event
> isn't triggered as well. not sure what the right hook for hosted apps should
> be.

To be honest, I'm not sure what to do here. I can't really think of anything better right now than messaging the collection app on each event.


> 2. homescreen is not updated when the new icon is saved to the
> CollectionsDatabase and still shows the old icon

This should totally work because we should get the datastore sync event. Cristian recently did some work to make homescreen launch faster by caching icons - I wonder if we need to manually bust the icon cache =/

Comment 4

[Amir Nissim (:amirn)]

(In reply to Kevin Grandon :kgrandon from comment #3)
> To be honest, I'm not sure what to do here. I can't really think of anything
> better right now than messaging the collection app on each event.

The problem with this approach is that 'install' fires for both packaged and hosted app and I don't know how to tell which type is it. For hosted we should handle it, but for packaged we should ignore it and wait for 'downloadapplied'. Who can give us some mozApps advice?


> This should totally work because we should get the datastore sync event.
> Cristian recently did some work to make homescreen launch faster by caching
> icons - I wonder if we need to manually bust the icon cache =/

This explains why I saw the updated icon after restarting the device. Crisitian, can you help?

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (:amirn) from comment #4)
> (In reply to Kevin Grandon :kgrandon from comment #3)
> The problem with this approach is that 'install' fires for both packaged and
> hosted app and I don't know how to tell which type is it. For hosted we
> should handle it, but for packaged we should ignore it and wait for
> 'downloadapplied'. Who can give us some mozApps advice?

I'll look into to see what else we can use. What's wrong with calling this twice for a packaged app? Will bad things happen? I know it's not ideal, but it may be an option while we continue to look for a solution. Also maybe check and see what we did in v1 that was different here?


> This explains why I saw the updated icon after restarting the device.
> Crisitian, can you help?

Can we simply delete the decoratedIconBlob property of icon.detail and call render? We should be able to do this in the 'update' datastore sync method.

Comment 6

[Jason Smith [:jsmith]]

I'm going mark this blocking- because I think having the collection icon not fully correct is a minor issue post installation of an app, as it does not cause any direct user impact (only visual impact). If somebody else disagrees, then feel free to flag down & indicate why.

Comment 7

[Cristian Rodriguez (:crdlc)]

To be honest I am lost here about what you want from me here. I've installed "Cut The Rope" from marketplace and it is displayed properly on the vertical homescreen. According to bug's title the collection icon is not updated. Is it because of cached icon? I can see how the flow does not go through cache code when the icon should be updated

https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/sources/collection.js#L121
https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/sources/collection.js#L156
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/collection.js#L71
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L578
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L385

Comment 8

[Cristian Rodriguez (:crdlc)]

Attachment: 1041295.patch

This patch calls to collection app when the download is applied but when we receive the update of the collection the icon is the same than the already displayed. See logs in the patch

Comment 9

[Amir Nissim (:amirn)]

Attachment: Pull Request

Thanks for your input Cristian.

It took a lot of digging around, but I finally found the problem:
on some cases, HomeIcons.init was not triggered after new app installs, and it's app index was out of sync.
I think this happened when the collection app was still in memory, and a new 'install' event didn't really trigger a fresh launch.

The attached patch fixes that by forcing index re-building on 'install' and 'uninstall'.

Comment 10

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8460874 [details] [review]
Pull Request

LGTM, just a minor comment on github, thanks a lot Amir

Comment 11

[Amir Nissim (:amirn)]

fixed and landed. thanks :)

master: https://github.com/mozilla-b2g/gaia/commit/44162e0

Comment 12

[Jacqueline Savory [:jsavory] UX]

I agree that this one is not a blocker, removing flag.