1174727 - Add new homescreen prototype to main Gaia repository

Status: RESOLVED FIXED

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

Description

[Chris Lord [:cwiiis]]

I've been working on a new homescreen for Gaia, which you can find here: https://gitlab.com/Cwiiis/homescreen-ng

At some point, it would be good to distribute this with Gaia as an alternative homescreen that can be switched to (it's not ready to replace verticalhome yet, and that isn't a given either).

I'm currently unsure of the best way to do this - it uses two web components that it links to with submodules. There is currently no test coverage for any of this code, and development is still going at a rapid enough pace that being part of main Gaia would be a considerable impediment.

I also like that this works as a reasonable example of a third-party system application at the moment, something we have very few of.

We could import this as a submodule perhaps, and periodically, manually update it? Suggestions welcome.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

I know that could sound overkilling but what about using the customization scripts:

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide

So we could define alternative gaia builds, if I remember correctly there is even a UI to perform this:
https://github.com/mozilla-b2g/preload-app-toolkit

Comment 2

[Chris Lord [:cwiiis]]

I think we've decided to go with Bower to handle external dependencies - we'll try to get this done this week if possible.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 8643760 [details] [review]
[gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

Not flagging this for review yet, would like to make sure at least unit tests are running correctly. (marionette test porting I think will have to be done after this lands, or it'll become unwieldy)

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8643760 [details] [review]
[gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

Switching to review now.

Comment 6

[Chris Lord [:cwiiis]]

Just a note about the review: The components from gaia-components shouldn't need review and have tests run on them in their own repository. gaia-app-icon and gaia-container have tests, but some review wouldn't hurt. Currently those tests aren't run automatically on check-in, but we're working on that.

We want to get this code in the tree to make it easier for people working on pin-the-web, it is of course not well-tested enough (wrt to automated tests, as well as QA) to switch to by default, and that won't happen until we have a decent suite of marionette and unit tests.

This review is mainly to check that there aren't any glaring errors, and that it conforms well enough to live in the gaia tree.

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8643760 [details] [review]
[gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

I will dig into this soon. Before doing so - the commits really need to be atomic in nature when landing something of this size (e.g., no breaking the linter in a one commit, then following-up in the next). They should also have a bug number, as well as reviewer per commit.

If you want to add bug numbers to all of these, I think that would be fine, alternatively squashing into a single commit might be the easier solution. Please rebase and flag me when ready. Thanks!

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #7)
> Comment on attachment 8643760 [details] [review]
> [gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master
> 
> I will dig into this soon. Before doing so - the commits really need to be
> atomic in nature when landing something of this size (e.g., no breaking the
> linter in a one commit, then following-up in the next). They should also
> have a bug number, as well as reviewer per commit.
> 
> If you want to add bug numbers to all of these, I think that would be fine,
> alternatively squashing into a single commit might be the easier solution.
> Please rebase and flag me when ready. Thanks!

Not a huge fan of losing the history, but understandable - done.

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8643760 [details] [review]
[gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

# General
Totally agree with the approach. Time to land this as an alternative homescreen and start iterating and marionette testing :)
I like the code organization a lot, it's really clean and easy to navigate.
Snapping behaves really well.

You should request a "stamp" from a Gaia peer (Tim or Vivien) to land a new app, but happy to continue with the code review itself!

# Blocking
Even with the webcomponent pref on, if I install the new homescreen as part of the gaia build process then select it in settings I always end up with an infinite crash loop :/
STR:
* make reset-gaia with the PR checked out
* enable the web-component pref
* select the homescreen in the Homescreens section of the settings app
-> crash loop

Not sure why this isn't happening when installing through the WebIDE.

Also we might want to add a warning in the "description" field of the manifest stating that you should not try this homescreen without the webcomponent pref on.
And a broken icon is showing up in the settings app for the new homescreen entry.

# UX
* I was a bit bummed that we only reorder on touchend, I'd excpect the icons to reorder if I keep my finger in place for ~300ms
* there might be a small bug where we snap to the next page at the end of a reordering when the destination is in the bottom row
* I miss the alternating background from the groups of the current homescreen, maybe alternating the background for snap pages would help
* The settings dialog when long-pressing in an empty corner is empty

# Performance
* Reflows at launch are pretty rough, looks like each gaia-icon does a separate 7-12ms reflow (on a flame), don't know if there's much to do about this (if it's the price to pay for component) but it won't scale super well :/

# Testing
* When possible I like to dispatch real events rather than calling handleEvent directly, especially when there isn't any marionette test covering the piece of code (which might change very fast :))

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Etienne Segonzac (:etienne) from comment #9)
> Comment on attachment 8643760 [details] [review]
> [gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master
> 
> # General
> Totally agree with the approach. Time to land this as an alternative
> homescreen and start iterating and marionette testing :)
> I like the code organization a lot, it's really clean and easy to navigate.
> Snapping behaves really well.
> 
> You should request a "stamp" from a Gaia peer (Tim or Vivien) to land a new
> app, but happy to continue with the code review itself!

Will do, and cheers :)

> # Blocking
> Even with the webcomponent pref on, if I install the new homescreen as part
> of the gaia build process then select it in settings I always end up with an
> infinite crash loop :/
> STR:
> * make reset-gaia with the PR checked out
> * enable the web-component pref
> * select the homescreen in the Homescreens section of the settings app
> -> crash loop
> 
> Not sure why this isn't happening when installing through the WebIDE.

I'm seeing this now, but I wasn't before... Possible Gecko regression? Will have to debug it, maybe I can work around it...

> Also we might want to add a warning in the "description" field of the
> manifest stating that you should not try this homescreen without the
> webcomponent pref on.

Good idea

> And a broken icon is showing up in the settings app for the new homescreen
> entry.

Also seeing this... Not sure why, maybe a build issue of some kind? :/

> # UX
> * I was a bit bummed that we only reorder on touchend, I'd excpect the icons
> to reorder if I keep my finger in place for ~300ms

Working on this in a branch, trying not to compromise performance but finding it tough :/

> * there might be a small bug where we snap to the next page at the end of a
> reordering when the destination is in the bottom row

Yeah, seen this - will fix after merging so the merge corresponds with the external tree.

> * I miss the alternating background from the groups of the current
> homescreen, maybe alternating the background for snap pages would help

mmm, Mike also suggested this, again post-merge :)

> * The settings dialog when long-pressing in an empty corner is empty

Not noticed this, will check it out.

> 
> # Performance
> * Reflows at launch are pretty rough, looks like each gaia-icon does a
> separate 7-12ms reflow (on a flame), don't know if there's much to do about
> this (if it's the price to pay for component) but it won't scale super well
> :/

Haven't really done any performance profiling, but certainly launch can be improved - I think I ought to add freeze/thaw methods on gaia-container for situations where you add lots of icons simultaneously... Or maybe a utility function to add multiple children at a time...

> # Testing
> * When possible I like to dispatch real events rather than calling
> handleEvent directly, especially when there isn't any marionette test
> covering the piece of code (which might change very fast :))

Not a bad idea :)

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8643760 [details] [review]
[gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

Hi both, flagging for approval to merge from whoever gets there first;

We'd like to start working on this in the Gaia tree now. I think enough people have looked it over that it shouldn't cause any issues when merging and tests are green.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8643760 [details] [review]
[gaia] Cwiiis:bug1174727-new-homescreen-checkin > mozilla-b2g:master

rubberstamping

Comment 13

[Chris Lord [:cwiiis]]

Merged, thanks all! https://github.com/mozilla-b2g/gaia/commit/c0aef6d09c9954204ce8824172c9ed8b9b35be19