1032829 - Smart Collection stop beeing added once the collection app has returned |false| (no user choice)

Status: VERIFIED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Step to reproduce:
 1. Long press on any empty part of the homescreen
 2. Choose 'Add Smart Collection'
 3. Once the list is displayd hit 'OK' without having choose any

 Repeat 1. and 2.

 4. Choose a collection this time

Actual result:
 - Nothing happens

Expected result:
 - The smart collection is added to the homescreen

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sounds definitively a blocker.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1032829.patch

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8448745 [details] [diff] [review]
bug1032829.patch

Review of attachment 8448745 [details] [diff] [review]:
-----------------------------------------------------------------

We need to call processPending, or update the state. Defaulting to an array is the easiest thing to do here. R+ assuming you do that.

::: apps/verticalhome/js/sources/collection.js
@@ +138,5 @@
>            this.inCreateActivity = false;
> +
> +          var ids = e.detail.ids;
> +          if (e.detail.ids === false) {
> +            break;

I am fairly positive that we want to call this.processPending() here. This should safeguard us against the super rare case where other sources can potentially create collections at the same time. I think we can potentially get into a strange case if we do not do this.

The problem is that we could continue to buffer collections into pendingCollections, and insertPosition is still defined. If calling processPending() breaks things, then we should fix that, or simply perform the necessary reset steps here. We could also potentially just default e.detail.ids to an array.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1032829.patch

Kevin, is it what you are looking for ?

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8448792 [details] [diff] [review]
bug1032829.patch

Review of attachment 8448792 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't test it, but assuming this works yes, I think this is more safe. Thanks!

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/6ca092e7cc1de651d1d652dafbd89414c38501fe

Comment 7

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/3d492ab8d93b1afe61e930da905639b23ad3dde0

Comment 8

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame v2.1&2.0.
See attachment: verified_1.png,verified_2.png and verified_3.png.
Reproduce rate: 0/5

STR:
1. Long press on any empty part of the homescreen
2. Choose 'Add Smart Collection'.
**See attch:verified_0.png and verified_1.png.
3. Once the list is displayd hit 'OK' without having choose any.
**There is no new collection added to homescreen
4. Repeat Step 1&2.
5. Choose a or more collections.
**The smart collection is added to the homescreen.See attch:verified_2.png and verified_3.png.

Flame 2.0 build:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ff1100ba2ab8
Build-ID        20141204000228
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.040425
FW-Date         Thu Dec  4 04:04:36 EST 2014
Bootloader      L1TC00011880

Flame 2.1 build:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.034958
FW-Date         Thu Dec  4 03:50:09 EST 2014
Bootloader      L1TC00011880

Comment 9

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1.MP4

Comment 10

[Shally Li（leave from Mozilla）]

Attachment: verified_0.png

Comment 11

[Shally Li（leave from Mozilla）]

Attachment: verified_1.png

Comment 12

[Shally Li（leave from Mozilla）]

Attachment: verified_2.png

Comment 13

[Shally Li（leave from Mozilla）]

Attachment: verified_3.png