1067435 - (app-grouping) [meta] Home-screen grouping

Status: RESOLVED FIXED

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

Description

[Chris Lord [:cwiiis]]

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

Comment 1

[Kevin Grandon :kgrandon]

Shall we turn this into a [meta] bug and block this bug with user stories and engineering bugs?

Comment 2

[Chris Lord [:cwiiis]]

Attachment: 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).

Comment 3

[Chris Lord [:cwiiis]]

(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 4

[Cristian Rodriguez (:crdlc)]

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.

Comment 5

[Chris Lord [:cwiiis]]

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).

Comment 6

[Kevin Grandon :kgrandon]

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 :)

Comment 7

[Chris Lord [:cwiiis]]

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)

Comment 8

[Kevin Grandon :kgrandon]

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.

Comment 9

[Maria Sandberg [:mushi]]

Hey guys, just FYI there may be some reasons to wait with putting this in master - let's discuss in the meeting tomorrow morning.

Comment 10

[Cristian Rodriguez (:crdlc)]

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

Comment 11

[Chris Lord [:cwiiis]]

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.

Comment 12

[Cristian Rodriguez (: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.

Comment 13

[Kevin Grandon :kgrandon]

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.

Comment 14

[Chris Lord [:cwiiis]]

(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).

Comment 15

[Kevin Grandon :kgrandon]

Thanks Chris. I'm fine with fixing the top three items in a follow-up, I just wanted to note them here.

Comment 16

[Cristian Rodriguez (:crdlc)]

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

Comment 17

[Chris Lord [:cwiiis]]

(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.

Comment 18

[Chris Lord [:cwiiis]]

Merged! https://github.com/mozilla-b2g/gaia/commit/997fe515e4417301d9e0c0704c7e3beb19179db5

Comment 19

[Maria Sandberg [:mushi]]

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.

Comment 20

[Chris Lord [:cwiiis]]

I agree with Maria, I think it's more useful to leave this open until we at least have it on by default.

Comment 21

[Kevin Grandon :kgrandon]

Sounds good, let's keep it open until all of the user stories are closed.

Comment 22

[katieC]

Attachment: App Grouping UX spec [v1.6]

Comment 23

[Chris Lord [:cwiiis]]

Is there meant to be a question for me here?

Comment 24

[Hermes Cheng (inactive after July 27, 2015)]

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.

Comment 25

[Kai-Chih Hu [:khu]]

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.

Comment 26

[Hermes Cheng (inactive after July 27, 2015)]

Test cases on MozTrap
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=3089

Comment 27

[Chris Lord [:cwiiis]]

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 :)

Comment 28

[Hermes Cheng (inactive after July 27, 2015)]

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)

Comment 29

[Peter Dolanjski [:pdol]]

Hermes, agreed, it should just ride the trains and ship if/when ready as judged by UX.

Comment 30

[katieC]

Attachment: App Grouping UX spec [v1.7]

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.

Comment 31

[katieC]

Attachment: App Grouping UX spec [v1.8]

App Grouping UX Spec - v1.8 

Updates:
Updated expand/collapsed toggle functionality (removed tapping on blank space to
expand collapsed group). Updated Open Issues.

Comment 32

[Hermes Cheng (inactive after July 27, 2015)]

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