Closed Bug 1022985 Opened 6 years ago Closed 6 years ago

SmartCollection is not placed where you tap


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

Not set


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

2.0 S4 (20june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified


(Reporter: jlal, Assigned: kgrandon)



(Whiteboard: [systemsfe][2.0-flame-test-run-2])


(2 files, 1 obsolete file)

It is placed always at the bottom rather then on the placeholder.
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"
QA Whiteboard: [VH-FC-blocking+]
Assignee: nobody → jlal
blocking-b2g: --- → 2.0?
Marked as 2.0? blocker as it's a basic use case covered by specs.
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]
Flags: needinfo?(kgrandon)
Flags: needinfo?(amir)
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).
Flags: needinfo?(kgrandon)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S4 (20june)
Un-assigning for the moment my very WIP is here 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.
Assignee: jlal → nobody
Flags: needinfo?(amir)
Attached file Github pull request (obsolete) —
Feels janky, but it works.
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 tests now perhaps.
Attachment #8439672 - Flags: review?(jlal)
Comment on attachment 8439672 [details] [review]
Github pull request

Going to clean this one up more first.
Attachment #8439672 - Flags: review?(jlal)
QA Whiteboard: [VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking+]
blocking-b2g: 2.0? → 2.0+
This is feature work, so we should not block on it. We can mark feature-b2g 2.0 if we'd like though.
blocking-b2g: 2.0+ → ---
Fair enough - Candice - Can you add a feature-b2g flag here then?
Blocks: collection-app
No longer blocks: vertical-homescreen
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+] → [VH-FL-blocking+][VH-FC-blocking+]
Flags: needinfo?(cserran)
feature-b2g: --- → 2.0
Flags: needinfo?(cserran)
Depends on: 1026273
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+] → [VH-FL-blocking+][VH-FC-blocking+][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [systemsfe] → [systemsfe][2.0-flame-test-run-2]
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+][QAnalyst-Triage?] → [VH-FL-blocking+][VH-FC-blocking+][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Attached file Github pull request
Attachment #8439672 - Attachment is obsolete: true
Comment on attachment 8443037 [details] [review]
Github pull request

Hey dale - have time for a review of this?
Attachment #8443037 - Flags: review?(dale)
Assignee: nobody → kgrandon
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)
Attachment #8443037 - Flags: review?(dale) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8443037 [details] [review]
Github pull request

This is needed for the vertical homescreen.
Attachment #8443037 - Flags: approval-gaia-v2.0?(bbajaj)
Keywords: verifyme
Attachment #8443037 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Keywords: verifyme
Kevin, this pr seems break l10n for homescreen, can you check it?

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

Flags: needinfo?(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!
Flags: needinfo?(kgrandon)
no, it cannot constantly reproduce, and yes I can reproduce it on master. let's try to fix it on bug 1027708.
You need to log in before you can comment on or make changes to this bug.