Bug 1067435 (app-grouping)

[meta] Home-screen grouping

RESOLVED FIXED in 2.2 S4 (23jan)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

unspecified
2.2 S4 (23jan)
All
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(feature-b2g:2.2+)

Details

(Whiteboard: [systemsfe] [2.2-feature-qa?])

User Story

This meta bug describes the app grouping feature.

App grouping enables users to organize applications on their homescreen in an intuitive manner.

Attachments

(2 attachments, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
kgrandon
: review+
crdlc
: review+
kgrandon
: feedback+
crdlc
: feedback+
Details | Review
1.58 MB, application/pdf
Details
(Assignee)

Description

5 years ago
We would like a more comprehensive grouping capability in the home-screen than the current dividers allow. Namely, we want:

- Expanding/collapsing groups
- Reordering groups
- Creating groups without having to drag icons to the end of the container
- Naming/renaming groups
Shall we turn this into a [meta] bug and block this bug with user stories and engineering bugs?
Alias: app-grouping
Summary: Home-screen grouping → [meta] Home-screen grouping
Whiteboard: [systemsfe]
(Assignee)

Comment 2

5 years ago
Posted file Implementation
This is an initial implementation, to spec (but not complete), of the new homescreen grouping.

The one major missing feature is group naming/renaming, and there's a serious bug where occasionally after reinstalling the home-screen (and I assume possibly in other situations where the homescreen saves/restarts) where the first group can get reattached to the last group and lose an icon. There are also some styling issues with dividers when not enabling grouping that I'll fix soon.

Submitting for feedback so we can fix up the last few issues/polish and get it landed (perhaps behind a setting, perhaps just as is).
Attachment #8489425 - Flags: feedback?(kgrandon)
Attachment #8489425 - Flags: feedback?(crdlc)
(Assignee)

Comment 3

5 years ago
(In reply to Kevin Grandon :kgrandon from comment #1)
> Shall we turn this into a [meta] bug and block this bug with user stories
> and engineering bugs?

Definitely - though I'll just post the bulk patch on here for now. I imagine after landing, there will be several smaller/larger bugs to fix :)
Comment on attachment 8489425 [details] [review]
Implementation

I gave some comments on github. LGTM as the seed for the new feature although I would like you to implement tests for all these new features before landing.
Attachment #8489425 - Flags: feedback?(crdlc) → feedback+
Depends on: 1067772
(Assignee)

Comment 5

5 years ago
Adding some platform dependencies. While bug 927349 doesn't strictly block this, a lot of the animations won't play readily without it (and there's already code in there to work around this bug that'd be nice to remove).
Depends on: 1066713, 927349
User Story: (updated)
Comment on attachment 8489425 [details] [review]
Implementation

I will take a more in-depth look when we have the R? request. Happy with the direction though :)
Attachment #8489425 - Flags: feedback?(kgrandon) → feedback+
(Assignee)

Comment 7

5 years ago
Comment on attachment 8489425 [details] [review]
Implementation

I've taken the feedback into account, fixed some tests (though a few integration tests still need fixing) and added new unit tests to test all new grouping features. Also fixed quite a few bugs with regards to rearranging app icons/groups (one or two pre-existing, I think).

I think this is ready for review :) (I also still think it'd be good to add integration tests for the new features, but in the interest of getting this merged sooner, I'd quite like to do this as follow-up and rely on the unit testing for now)
Attachment #8489425 - Flags: review?(kgrandon)
Attachment #8489425 - Flags: review?(crdlc)
Just making an early note that this should be rebased against master, and we should get the integration tests passing. Linters are failing also - but I'm not sure if that's because of the patch, or the recent Linter breakage.
Hey guys, just FYI there may be some reasons to wait with putting this in master - let's discuss in the meeting tomorrow morning.
Depends on: 1069693
Depends on: 1069696
Depends on: 1069697
Depends on: 1069698
Depends on: 1069699
Depends on: 1069701
Depends on: 1069703
Depends on: 1069705
No longer depends on: 1067772
Comment on attachment 8489425 [details] [review]
Implementation

Added some comments on github. I couldn't check this in my device because of rebasing but I guess that this pr does not take care of four columns layout. BTW excellent job as always! Once rebased, comments addressed or answered you can ask again for a review. Thanks a lot
Attachment #8489425 - Flags: review?(crdlc)
(Assignee)

Comment 11

5 years ago
Comment on attachment 8489425 [details] [review]
Implementation

I believe this addresses most of the review comments (but I may need some reminding), fixes some (all? Don't know...) tests and 4-column mode - I've also added a hacky switch for it in the developer menu, as requested by UX and discussed with kgrandon.
Attachment #8489425 - Flags: review?(crdlc)
Comment on attachment 8489425 [details] [review]
Implementation

Good job as always. It seems to be working fine!! I guess this an excellent code to land in master. Obviously we can add several integration tests but in a follow-up thought. I left some comments on Github.

I am a bit concerned about adding SettingsListener. Could we move it to own datastore of preferences? Maybe the home screen view in settings could show this option if the developer menu is enabled.
Attachment #8489425 - Flags: review?(crdlc) → review+
(Assignee)

Updated

5 years ago
Depends on: 1071029
Comment on attachment 8489425 [details] [review]
Implementation

This looks good for now, I've added a few things on github that should probably be addressed before landing. Here are the main issues I've noticed that I think should be fixed by turning this on by default, but I'm fine landing this and iterating on it. We should get some bugs filed for these follow-up issues:

* You can't drop an app on a collapsed group, not sure if this is as spec'd.

* You can create an infinite number of empty groups when you are dragging. It's not terrible as they disappear when you drop, but I feel we shouldn't have more than one empty group at any time.

* There can sometimes be an empty space after the last group.

* When you get to a certain number of icons that are not collapsed we will always miss the transition which seems kind of weird. It is quite nice when you have it though. I suppose we can keep it for now, but we should definitely do some profiling and see if the gfx team can help us here.
Attachment #8489425 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 14

5 years ago
(In reply to Kevin Grandon :kgrandon from comment #13)
> Comment on attachment 8489425 [details] [review]
> Implementation
> 
> This looks good for now, I've added a few things on github that should
> probably be addressed before landing. Here are the main issues I've noticed
> that I think should be fixed by turning this on by default, but I'm fine
> landing this and iterating on it. We should get some bugs filed for these
> follow-up issues:
> 
> * You can't drop an app on a collapsed group, not sure if this is as spec'd.

I don't think this is as spec'd, and easily fixed - but I may address this in follow-up. Similarly, you should be able to move expanded groups (the long-press causes them to collapse), but I'd rather address that in follow-up.

> * You can create an infinite number of empty groups when you are dragging.
> It's not terrible as they disappear when you drop, but I feel we shouldn't
> have more than one empty group at any time.

Damnit, I thought I'd fixed this, must've regressed it at some point - I'll fix this.

> * There can sometimes be an empty space after the last group.

So before there would be an empty space in edit mode and not in non-edit mode, but this caused jarring transitions if you were at the bottom of the list when you exited edit mode... Again, I think I'd rather address this in follow-up.

> * When you get to a certain number of icons that are not collapsed we will
> always miss the transition which seems kind of weird. It is quite nice when
> you have it though. I suppose we can keep it for now, but we should
> definitely do some profiling and see if the gfx team can help us here.

This is basically bug 927349 - I tried working around it a few ways, but couldn't get anything satisfactory that wasn't pretty convoluted and I know that that bug's being worked on (it's in the dependencies list of this bug).
Thanks Chris. I'm fine with fixing the top three items in a follow-up, I just wanted to note them here.
I think that we can land this and fix problems in follow-ups. Then we can land more patches once done it. What do you think? Thanks guys
(Assignee)

Comment 17

5 years ago
(In reply to Cristian Rodriguez (:crdlc) from comment #16)
> I think that we can land this and fix problems in follow-ups. Then we can
> land more patches once done it. What do you think? Thanks guys

I think so, but then I may be biased :) I think once bug 1071029 hits a Nightly and I've made fix-ups for the bits Kevin has pointed out, tests should pass again and it'll be ready.
(Assignee)

Comment 18

5 years ago
Merged! https://github.com/mozilla-b2g/gaia/commit/997fe515e4417301d9e0c0704c7e3beb19179db5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 1091007
Hi guys, sorry if this is a bad bug manners but should we re-open this bug since it's the big old metabug and we still have open bugs that this one is depending on? 

In particular I'm trying to make sure that the platform bugs (bug 927349 and bug 1066713 are clearly marked as blocking grouping.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 20

5 years ago
I agree with Maria, I think it's more useful to leave this open until we at least have it on by default.
Status: RESOLVED → REOPENED
Depends on: 1082675
Resolution: FIXED → ---
Sounds good, let's keep it open until all of the user stories are closed.
Flags: needinfo?(kgrandon)
(Assignee)

Updated

5 years ago
Depends on: 1078766
(Assignee)

Updated

5 years ago
Depends on: 1092352
(Assignee)

Updated

5 years ago
Depends on: 1092327
Depends on: 1088353
(Assignee)

Updated

5 years ago
Depends on: 1100357
feature-b2g: --- → 2.2+

Comment 22

5 years ago
Posted file App Grouping UX spec [v1.6] (obsolete) —
Depends on: 1102119
(Assignee)

Updated

5 years ago
Depends on: 1102220
(Assignee)

Updated

5 years ago
Depends on: 1104081
(Assignee)

Updated

5 years ago
No longer depends on: 1100357
Flags: in-moztrap?(mozillamarcia.knous)
Whiteboard: [systemsfe] → [systemsfe] [2.2-feature-qa+]
Whiteboard: [systemsfe] [2.2-feature-qa+] → [systemsfe] [2.2-feature-qa?]
(Assignee)

Updated

4 years ago
Depends on: 1109068
(Assignee)

Updated

4 years ago
Depends on: 1105850
(Assignee)

Updated

4 years ago
Depends on: 1087656
(Assignee)

Updated

4 years ago
Depends on: 1093270
(Assignee)

Updated

4 years ago
Depends on: 1077169
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 23

4 years ago
Is there meant to be a question for me here?
Flags: needinfo?(chrislord.net) → needinfo?(hcheng)
Sorry, I forgot to input my question. I wanted to ask is this meta bug included in 2.2 scope, but I have see the 2.2 feature list and it is included. Thanks.
Flags: needinfo?(hcheng)
Depends on: 1107325
Based on the information I got, the target milestone for this bug could be 2.2 Sprint 4. Please feel free to change it if I am wrong. Thank you very much.
Target Milestone: --- → 2.2 S4 (23jan)
Depends on: 1109948
(Assignee)

Updated

4 years ago
Depends on: 1116082
(Assignee)

Updated

4 years ago
Depends on: 1118311
(Assignee)

Updated

4 years ago
No longer depends on: 1077169
(Assignee)

Updated

4 years ago
No longer depends on: 1104081
(Assignee)

Comment 27

4 years ago
I believe we can call this done now - all dependent bugs are fixed. We can track enhancements in individual bugs and open an enhancement meta-bug if we feel it's necessary, but I think we're finished here - this is good enough to ship :)
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Depends on: 1115512
(Assignee)

Updated

4 years ago
No longer depends on: 1119650
Hi Peter,

This feature is not part of the 2.2 committed list [1]. Could you please contact Kevin Hu and Thomas Ho if you want to keep this User Story in b2g v2.2?

[1] https://docs.google.com/a/mozilla.com/spreadsheets/d/1l_HJcd3xcWgvM-VsDOzPUQPR3kPP36gs84lGXxC-TIY/edit#gid=981008657 (first tab of spreadsheet)
Flags: needinfo?(pdolanjski)
Hermes, agreed, it should just ride the trains and ship if/when ready as judged by UX.
Flags: needinfo?(pdolanjski)
Depends on: 1120718
Depends on: 1120737
(Assignee)

Updated

4 years ago
Depends on: 1120882
Depends on: 1121680
(Assignee)

Updated

4 years ago
No longer depends on: 1121865
(Assignee)

Updated

4 years ago
No longer depends on: 1121680

Comment 30

4 years ago
Posted file App Grouping UX spec [v1.7] (obsolete) —
v1.7 reflects the following updates: 
Updated User Stories, Open Issues, updated details of collapsed groups (changed 7 to 6 icons), removed drag handle for collapsed groups. Added group highlight in edit mode for moving groups and moving apps into groups. Action Menu Behavior added. App Name details added. Adding Smart Collections flow added. Added note about small actionable icons in details. Removed ‘tap on empty space’ to exit Edit mode. Added dragging icons into collapsed groups flow.
Attachment #8525403 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1122078
(Assignee)

Updated

4 years ago
Depends on: 1106544

Comment 31

4 years ago
App Grouping UX Spec - v1.8 

Updates:
Updated expand/collapsed toggle functionality (removed tapping on blank space to
expand collapsed group). Updated Open Issues.
Attachment #8551997 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1131039
Depends on: 1132728
Test run to sign off v2.2 Feature Landing

***Test Summary***
23 test cases ran, 23 passed

***Test Run record***
https://moztrap.mozilla.org/results/cases/?&pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=3089&filter-run=6441

***Build Info***
Build ID               20150215002504
Gaia Revision          ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gaia Date              2015-02-13 05:27:43
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/62c80c92b39e
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150215.040754
Firmware Date          Sun Feb 15 04:08:04 EST 2015
Bootloader             L1TC000118D0
You need to log in before you can comment on or make changes to this bug.