1022985 - SmartCollection is not placed where you tap

Status: VERIFIED FIXED

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

Description

[James Lal [:lightsofapollo]]

It is placed always at the bottom rather then on the placeholder.

Comment 1

[Johan Lorenzo [:jlorenzo]]

As per UX specs: "The smart collection that is added to the homepage will be placed in the group that the user initiated the long press in"

Comment 2

[Johan Lorenzo [:jlorenzo]]

Marked as 2.0? blocker as it's a basic use case covered by specs.

Comment 3

[James Lal [:lightsofapollo]]

OK- This turned out to be slightly more complicated... I have the part where you tap and I can figure how roughly where we should place the collection (great easy) but after triggering the activity it gets ugly (not hard but ugly). Looking at the code now I see two options:


  - pass the desired position through to the collections app (and propagate it through into the datastore)
  - set some flag internally in the homescreen to wait for the collection to be added (then add it into the right spot) [might have race conditions?]

Neither option is difficult but both are somewhat ugly... Anyone have a better idea? [I will continue to think while I tackle a different bug]

Comment 4

[Kevin Grandon :kgrandon]

I don't think this should go into datastore. My preference would be to set some flag internally, until we return from the creation.

You could either tack something onto the window, or stick it onto app.itemStore.collectionSource (which is the context you should get the datastore update event in).

Comment 5

[James Lal [:lightsofapollo]]

Un-assigning for the moment my very WIP is here https://github.com/mozilla-b2g/gaia/pull/20387 to bring it the rest of the way the following needs to be done:

  - keep track of a pending insertion (by emitting an event and listening for it in the collection source)
  - buffer all additions to the collection source
  - return the ids of the newly added collections (there might be more then one) from the activity
  - when the ids are returned move items from the buffer which match the ids returned by the activity to  the homescreen and the saved position


I will likely pick up this work again right after but to correctly test/ensure we don't have races is taking longer then I anticipated and I am going to work on easier to fix but require more input from the UX team.

Comment 6

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Feels janky, but it works.

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8439672 [details] [review]
Github pull request

James - I hijacked your patch. What do you think of this? Fairly simple, but seems to work.

Going to work on trying to write some E.me tests now perhaps.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8439672 [details] [review]
Github pull request

Going to clean this one up more first.

Comment 9

[Kevin Grandon :kgrandon]

This is feature work, so we should not block on it. We can mark feature-b2g 2.0 if we'd like though.

Comment 10

[Jason Smith [:jsmith]]

Fair enough - Candice - Can you add a feature-b2g flag here then?

Comment 11

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 12

[Kevin Grandon :kgrandon]

Comment on attachment 8443037 [details] [review]
Github pull request

Hey dale - have time for a review of this?

Comment 13

[Dale Harvey (:daleharvey)]

Comment on attachment 8443037 [details] [review]
Github pull request

This works good for me, we should really have integration tests for ths, but going to work on that, can land with a green run (seeing app_search test failures at the moment)

Comment 14

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/b0f74767bfb7e62c9cafca4b12b593b4912591ed

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8443037 [details] [review]
Github pull request

This is needed for the vertical homescreen.

Comment 16

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/65c9a7655840ccdee1fb8ffb6cb16638082742e8

Comment 17

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: 2014-06-23 16:24:40 的螢幕擷圖.png

Kevin, this pr seems break l10n for homescreen, can you check it?

STR:
1. download Italian language pack[1], unzip it to <L10N_DIR>/it
2. add {"it": "Italian"} to shared/resources/languages.json
3. LOCALE_BASEDIR=<L10N_DIR> make

[1] http://hg.mozilla.org/gaia-l10n/it/archive/tip.tar.gz

Comment 18

[Kevin Grandon :kgrandon]

Hey Yuren - are you constantly able to reproduce? Do you see it on master at all? I wonder if this may be another manifestation of bug 1027708, which we hope to fix soon. Thanks!

Comment 19

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

no, it cannot constantly reproduce, and yes I can reproduce it on master. let's try to fix it on bug 1027708.

Comment 20

[Johan Lorenzo [:jlorenzo]]

https://moztrap.mozilla.org/manage/case/13200/