929572 - When building a customized grid into a build, if the grid isn't valid, report a build error

Status: RESOLVED FIXED

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

Description

[Jason Smith [:jsmith]]

Motivation - https://bugzilla.mozilla.org/show_bug.cgi?id=927973#c7

We need to fire build errors in the case when the homescreen grid isn't valid in a customization given in a distribution directory.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Can you take a look at this, Albert?

Comment 2

[Albert [:albert]]

Attachment: Patch

Comment 3

[Albert [:albert]]

There is an important thing to keep in mind, is not possible to know at build time the max number of applications that can be accommodated per screen. Apps per screen is a dynamic value, at a runtime apps per screen is fixed to 16 in normal devices, 15 in tablets and then, depending on the scree size, it can be incremented. For this reason the patch will launch a warning is one screen has more than 16 apps but an exception.

Comment 4

[Albert [:albert]]

For testing:
- Clone https://github.com/acperez/firefoxos-gaia-testsbuild
- Launch GAIA_DISTRIBUTION_DIR=/path/to/firefox-gaia-testsbuild make reset-gaia

Comment 5

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Albert, 

John is rewriting variant.py in javascript on bug 921417 and I thought the progress is good to review in this week, so we should wait that commit landed and fix this issue in javascirpt version.

Is it okay to you?

Comment 6

[Albert [:albert]]

(In reply to Yuren Ju [:yurenju] from comment #5)
> Albert, 
> 
> John is rewriting variant.py in javascript on bug 921417 and I thought the
> progress is good to review in this week, so we should wait that commit
> landed and fix this issue in javascirpt version.
> 
> Is it okay to you?

Yes, it's fine

Comment 7

[Albert [:albert]]

Attachment: Patch

Javascript version

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 827034 [details]
Patch

Thanks for this patch.

I give it r- because a logic error in the code which makes qa-test-case not pass, please find them in github.

There is another thing I am not sure: what should we do when app.location is too large and no more wildcard apps?? In this patch, we throw an error. But I feel in this kind of case, the homescreen will append this app at the last.

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to John Hu [:johnhu] from comment #8)
> There is another thing I am not sure: what should we do when app.location is
> too large and no more wildcard apps?? In this patch, we throw an error. But
> I feel in this kind of case, the homescreen will append this app at the last.
I think that was exactly the intent for this patch: to throw an error when the configuration specifies a grid that we cannot satisfy exactly, instead of just creating an approximated but incorrect (incorrect meaning not exactly the specified one) grid.

So when a grid is specified that would create gaps (ie, the location is too big) then we should fail, which is what this patch does. Flagging Jason just in case we understood the intent of the bug incorrectly.

Comment 10

[Albert [:albert]]

Comment on attachment 827034 [details]
Patch

Changes from github comments

Comment 11

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #9)
> (In reply to John Hu [:johnhu] from comment #8)
> > There is another thing I am not sure: what should we do when app.location is
> > too large and no more wildcard apps?? In this patch, we throw an error. But
> > I feel in this kind of case, the homescreen will append this app at the last.
> I think that was exactly the intent for this patch: to throw an error when
> the configuration specifies a grid that we cannot satisfy exactly, instead
> of just creating an approximated but incorrect (incorrect meaning not
> exactly the specified one) grid.
> 
> So when a grid is specified that would create gaps (ie, the location is too
> big) then we should fail, which is what this patch does. Flagging Jason just
> in case we understood the intent of the bug incorrectly.

Yup, that sounds right.

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 827034 [details]
Patch

Nice patch. I think it's ok to land. Please find my latest small suggestion in the github.

Comment 13

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 827034 [details]
Patch

Albert, I left some comments on github, please set review flag to me again if you fix it.

the major issue is accessing init.json which may not exists if use variant.js in customization tool on bug 897325.

Comment 14

[Albert [:albert]]

Comment on attachment 827034 [details]
Patch

Changes from comment 13

Comment 15

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 827034 [details]
Patch

Okay looks good, thank you!

Comment 16

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

don't forget remove that two more 'n' :)

Comment 17

[Albert [:albert]]

(In reply to Yuren Ju [:yurenju] from comment #16)
> don't forget remove that two more 'n' :)

Ouch! I forgot the filename I was using for testing... fixed.

Thanks!

Comment 18

[Albert [:albert]]

Master: https://github.com/mozilla-b2g/gaia/commit/de5b918f61a7823fbbdfa48b8bc3cbc1765dc2a4