Closed Bug 1082675 Opened 6 years ago Closed 6 years ago

(App-grouping) Enable app-grouping by default

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-master verified)

VERIFIED FIXED
2.1 S9 (21Nov)
Tracking Status
b2g-master --- verified

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
crdlc
: review+
kgrandon
: review+
kcaldwell
: ui-review+
hnguyen
: ui-review+
Details | Review
We're currently maintaining two home-screen experiences; with and without app-grouping. Assuming we want app-grouping in 2.2, it would be sensible to enable this by default and perhaps even remove the option to disable it sooner rather than later.

The design is still being iterated on, but at this point, the changes aren't particularly radical.

Opening this bug to start discussion and track the issues that are stopping us from enabling it.
Thanks Chris. I think we've given the team a good amount of time to review and give feedback on this. 

The UX team has requested to evaluate a version with Hung's updates (no group names, left alignment etc). Flagging Rob and Peter/Wilfred here to weigh in on whether we can move grouping to default or if we'd like to wait until the new version has been tried out.
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(rmacdonald)
Flags: needinfo?(pdolanjski)
How long would it take to do a review of the version with the updates? if its just few days lets do that review and then make it default.
Flags: needinfo?(wmathanaraj)
Depends on: 1086514
(In reply to Wilfred Mathanaraj [:WDM] from comment #2)
> How long would it take to do a review of the version with the updates? if
> its just few days lets do that review and then make it default.

I agree.  If we're going to change it very soon, let's just wait until the change.  If, however, that'll take a few weeks, let's just enable it.
Flags: needinfo?(pdolanjski)
Actually, I think we should block this on the animations patch landing, otherwise all the transitions are super-janky and I'd rather not give a bad first impression...
Depends on: 927349
Blocks: app-grouping
Looks like we are close to a go-ahead to enable by default from the UX side. Chris, do you still think it's worth blocking on bug 927349? It seems up in the air how long it will take to get that one fixed. 

Normally I would agree with waiting until the animations patch but at this point I'm inclined to just enable it so we can get more people playing around with the feature. If we expect the patch to take longer than a week or so I say enable by default, and we'll write up an email to dev-gaia explaining the jank. Let's meet early next week for the final call.
+1 for turning this on now. I would like to see us fix bug 1091007 first before enabling this by default as that's currently a bit janky. I'm currently swamped with blockers, but I can try to grab it next week if you don't get to it Chris.
Yes, I suppose blocking on bug 927349 might potentially delay this quite a bit, so perhaps we shouldn't. Let's go ahead (I'll fix bug 1091007 tomorrow) without it, but I think your suggestion of posting about the jank is a good idea.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
This removes the setting and enables app-grouping by default.

I'll flag this for ui-review as soon as I know the right person to flag :)
Attachment #8519980 - Flags: review?(kgrandon)
Attachment #8519980 - Flags: review?(crdlc)
Comment on attachment 8519980 [details] [review]
Enable app-grouping permanently

Falling for ui-review, to make sure that we're ready to do this.
Attachment #8519980 - Flags: ui-review?(kcaldwell)
Comment on attachment 8519980 [details] [review]
Enable app-grouping permanently

This is awesome! The code looks good, but there's a few failing marionette tests that I'd like to see addressed before doing the review cycle. Since we're deprecating the setting it looks like we'll need to fix-up the tests to take that into account.

I looked at one failing test, and it looked like it was long-pressing on a divider to trigger the long-press dialog. I'd imagine that this is one of the trickier situations to solve. It may be easy if we somehow create a group with only a single app result, then you could long press on an area with no apps. Anyway, take a look if you can and re-flag me when needed. Thanks!
Attachment #8519980 - Flags: review?(kgrandon)
Attachment #8519980 - Flags: review?(crdlc)
Fixed tests locally, waiting on green try before flagging.
Attachment #8519980 - Flags: ui-review?(hnguyen)
There's one more failing python test, but it's exposed an actual bug... If it's small, I'll fix it as part of this.
Comment on attachment 8519980 [details] [review]
Enable app-grouping permanently

Took a look at the latest master and aside from some minor potential improvements, I think we should move forward with making this default.
Attachment #8519980 - Flags: ui-review?(hnguyen) → ui-review+
Attachment #8519980 - Flags: ui-review?(kcaldwell) → ui-review+
Blocks: 1096538
Comment on attachment 8519980 [details] [review]
Enable app-grouping permanently

I think this will be green now... https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=81afd17b5871
Attachment #8519980 - Flags: review?(kgrandon)
Attachment #8519980 - Flags: review?(crdlc)
Comment on attachment 8519980 [details] [review]
Enable app-grouping permanently

Very clean and excellent job, thanks
Attachment #8519980 - Flags: review?(crdlc) → review+
Comment on attachment 8519980 [details] [review]
Enable app-grouping permanently

Looks good to me. Let's do this!
Attachment #8519980 - Flags: review?(kgrandon) → review+
Merged! https://github.com/mozilla-b2g/gaia/commit/d0bc392a83fad3dd15d8f614581fcc4f36d4adbc

There was a failure in f1 to do with notifications, but I'm going to assume that's something else (this doesn't touch anything near that...)

Fingers crossed this sticks...
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Chris Lord [:cwiiis] from comment #17)
> Merged!

\o/
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S9 (21Nov)
Flags: needinfo?(rmacdonald)
This issue is verified fixed on Flame Master.

Result: App-grouping is enabled by default. 
 
Device: Flame Master (319mb, full flash)
Build ID: 20150209010211
Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec
Gecko: 3436787a82d0
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.