930544 - [HOMESCREEN] Applications configured with incorrect order in the file "variant.json" are installed in incorrect positions

Status: VERIFIED FIXED

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

Description

[rafael.marquez]

Device: Unagi
Branch: 1.2
variant.json configuration:

{
    "id": "Twitter",
    "screen": 3,
    "location": 12
},
 {
    "id": "Accuweather",
    "screen": 5,
	"location": 0
},
{
	"id": "Wikipedia",
	"screen": 4,
	"location": 0
}


* Procedure:
1. Execute the command "GAIA_DISTRIBUTION_DIR=(firefoxos-gaia-testsbuild directory) PRODUCTION=1 make reset-gaia" to flash the device.
2. Complete the FTE.

* Expected result
Twitter app is installed on the desktop 3
Accuweather app is installed on screen 5 in the location 0
Wikipedia app is installed on screen 4 in the location 0


* Actual result
Twitter app is installed on the desktop 3
Accuweather app is installed on screen 4 in the location 0
Wikipedia app is installed on screen 4 in the location 1

Comment 1

[Jason Smith [:jsmith]]

Note for testing - the typical grid layouts Karen from content management has built in past releases always place apps on the 2nd & 3rd screen. I don't think I recall an instance where they've ended up on the 4th, 5th, or 6th screen.

Comment 2

[Preeti Raghunath(:Preeti)]

koi+ for cert blocker

Comment 3

[rafael.marquez]

If you have a build that has only Core and Common 3rd party apps on screen 1 and has the variant.json:

{
"id": "Accuweather",
"screen": 3,
	"location": 0
},
{
id": "Wikipedia",
"screen": 2,
"location": 0
}

We have the same problem

Comment 4

[Jason Smith [:jsmith]]

(In reply to rafael.marquez from comment #3)
> If you have a build that has only Core and Common 3rd party apps on screen 1
> and has the variant.json:
> 
> {
> "id": "Accuweather",
> "screen": 3,
> 	"location": 0
> },
> {
> id": "Wikipedia",
> "screen": 2,
> "location": 0
> }
> 
> We have the same problem

Wouldn't the code above actually be:

{
  "id": "Accuweather",
   "screen": 1,
   "location": 0
},
{
  "id": "Wikipedia",
  "screen": 1,
  "location": 1
}

FWIW - I saw a problem on screen 1 when I inserted the SIM after the FTE, not before. See bug 930305.

Comment 5

[Carmen Jimenez Cabezas]

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to rafael.marquez from comment #3)
> > If you have a build that has only Core and Common 3rd party apps on screen 1
> > and has the variant.json:
> > 
> > {
> > "id": "Accuweather",
> > "screen": 3,
> > 	"location": 0
> > },
> > {
> > id": "Wikipedia",
> > "screen": 2,
> > "location": 0
> > }
> > 
> > We have the same problem
> 
> Wouldn't the code above actually be:
> 
> {
>   "id": "Accuweather",
>    "screen": 1,
>    "location": 0
> },
> {
>   "id": "Wikipedia",
>   "screen": 1,
>   "location": 1
> }
> 
> FWIW - I saw a problem on screen 1 when I inserted the SIM after the FTE,
> not before. See bug 930305.

No, in fact the initial description of Rafa shows what the problem is.

The bug happens when the next circunstances concur:
- There are two apps defined on the variant.json configuration file with a different screen each.
- None of the screen assigned to the apps have any other app (i.e. desired screens do not exits).
- The app with the higher numbered screen is installed first.
 
The reason it fails is that the app which has a desired screen of n + 2 where n is the maximun number of screen defined previously to the install of the app is installed on the n + 1 screen (because we don't allow empty screen). Later when the other app is installed on the n + 1 screen the first app isn't moved to its correct screen

Comment 6

[Jason Smith [:jsmith]]

Re-Triage - Is it possible to workaround this bug by keeping the app grid locations all is order of lowest to highest? If so, then it's not a blocker.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #6)
> Re-Triage - Is it possible to workaround this bug by keeping the app grid
> locations all is order of lowest to highest? If so, then it's not a blocker.

I would say no. The order the apps are actually installed is non deterministic (the apps installation are launched in order but then events happen as they might).

Comment 8

[Jason Smith [:jsmith]]

Okay, removing qawanted then. Let stay blocking on this then.

Comment 9

[Preeti Raghunath(:Preeti)]

Hello Carmen,

We agree this bug is a blocker.  Can we please help get a timeline for when the fix can land? I'll move it to the right milestone.

Comment 10

[Carmen Jimenez Cabezas]

I'll upload the patch along this afternoon

Comment 11

[Carmen Jimenez Cabezas]

Attachment: proposed patch v1

Comment 12

[Cristian Rodriguez (:crdlc)]

Comment on attachment 825452 [details] [review]
proposed patch v1

From my point of view, this implementation should follow this paradigm:

* _insertIcon: this method just inserts an icon and it shouldn't return an array of icons that they desire to be in other screen (avoid to create fake icons, ...). Imagine that we have another kind of icons in the future with other behavior, this implementation won't be scalable. _insertIcon only knows about inserting icons and that's all in my honest opinion

* Once the single variant app has been inserted, one new method should examine the current screen/page and propagate icons which desire to be in other page if it is possible 

Ask for a new review when it will be available. If you aren't agree with my approach we could ask for feedback to Vivien who is owner too

I don't want you spend more time on it because you're busy but honestly this solution is not so much clean and scalable in my opinion

Comment 13

[Carmen Jimenez Cabezas]

I think when you say scalability you actually mean extensibility. And taking that into account would be as easy as having a Icon constructor from a <li> item. 
I can make the change you request, but I'll be slower (and thus less scalable, actually ;) ) since it'll have to read the screen twice, once to insert the icon and another to extract the icons that have to be moved. It's not a huge problem though.
Anyway, that's just my point of view. I'll change the code so it works as you prefer

Comment 14

[Cristian Rodriguez (:crdlc)]

I explained my idea badly perhaps. When I said scalable I meant: "I want to have a code with the ability of handling new features in a capable manner". If we have an object called "Page" which defines a method called "insertIcon", nobody expects in my honest opinion that it inserts the icon, searches for other icons that should be in other pages, creates fake descriptors, generates new icons and returns these set of icons. 

This is my point of view and I don't want to start a discussion (because you and me are in the same boat and we can talk offline about it). If you aren't agree with my approach you don't have to do it if you don't want (I don't have the absolutely true, I hope so but no :D). I would be comfortable if you want to ask for another opinion (e.g. Vivien).

Thanks for your collaboration Carmen

(In reply to Carmen Jimenez Cabezas from comment #13)
> I think when you say scalability you actually mean extensibility. And taking
> that into account would be as easy as having a Icon constructor from a <li>
> item. 
> I can make the change you request, but I'll be slower (and thus less
> scalable, actually ;) ) since it'll have to read the screen twice, once to
> insert the icon and another to extract the icons that have to be moved. It's
> not a huge problem though.
> Anyway, that's just my point of view. I'll change the code so it works as
> you prefer

Comment 15

[Kai-Chih Hu [:khu]]

koi+ is for 1.2. We should use 1.2 target milestone rather than 1.3 target milestone.

Comment 16

[Cristian Rodriguez (:crdlc)]

Comment on attachment 825452 [details] [review]
proposed patch v1

Great work Carmen as usual! Thanks a lot for this patch

Comment 17

[bhavana bajaj [:bajaj]]

Adding check-in needed sa this has a review

Comment 18

[Gregor Wagner [:gwagner]]

Travis failed. I re-triggered to see if it's related to this PR.

Comment 19

[Gregor Wagner [:gwagner]]

Thanks Carmen!

https://github.com/mozilla-b2g/gaia/commit/82e8266e56b8d7dc94832d2f94c7f4deef658c96

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 82e8266e56b8d7dc94832d2f94c7f4deef658c96
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 21

[Carmen Jimenez Cabezas]

I've merged it on my own v1.2 branch, commit https://github.com/mcjimenez/gaia/commit/a7d9c295d7d27f0ec5b26f949b06bf4469f3ac63, but I don't have push permission to the official repo. Do I make a PR to the v1.2 branch or can you take it from my branch?

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

v1.2: d90ae864147eb3da2ad1084918035bafb1a90907

Comment 23

[rafael.marquez]

Verified:
Device unagi
Branch 1.2
Gecko 6ac8a1e
Gaia 264c604