(App-grouping) Enable app-grouping by default

VERIFIED FIXED in 2.1 S9 (21Nov)

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

unspecified
2.1 S9 (21Nov)
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-master verified)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
crdlc
: review+
kgrandon
: review+
kcaldwell
: ui-review+
hnguyen
: ui-review+
Details | Review
Assignee

Description

5 years ago
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)
Assignee

Updated

5 years ago
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)
Assignee

Comment 4

5 years ago
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
Assignee

Updated

5 years ago
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.
Assignee

Comment 7

5 years ago
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

Updated

5 years ago
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Assignee

Comment 8

5 years ago
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)
Assignee

Comment 9

5 years ago
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)
Assignee

Comment 11

5 years ago
Fixed tests locally, waiting on green try before flagging.

Updated

5 years ago
Attachment #8519980 - Flags: ui-review?(hnguyen)
Assignee

Comment 12

5 years ago
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 13

5 years ago
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+

Updated

5 years ago
Attachment #8519980 - Flags: ui-review?(kcaldwell) → ui-review+
Assignee

Updated

5 years ago
Blocks: 1096538
Assignee

Comment 14

5 years ago
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+
Assignee

Comment 17

5 years ago
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: 5 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.