899995 - Do not display the Homescreen section if there is only one homescreen

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Since it is unclear if it will be possible to have multiple |privileged| homescreen for 1.2 I think it would be good to hide the section if there is only one.

That will let devs prototype on it while it will not distract users.

Comment 1

[Olav Nymoen [:olav]]

Attachment: github_pointer

This approach iterates over apps on settings startup. Not ideal and very interested in hearing about better approaches.

Comment 2

[[:fabrice] Fabrice Desré]

To do this really correctly, you need also need to listen to mozApps.mgmt.oninstall and onuninstall to make sure you don't end up out of sync with the real list of installed apps.

Comment 3

[Olav Nymoen [:olav]]

Absolutely. Added that.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 783787 [details] [review]
github_pointer

Can you also check that this does not slow down Settings startup?

Comment 5

[Olav Nymoen [:olav]]

The mozApps.mgmt.oninstall / onuninstall was already used by settings/js/apps.js so I changed to a scheme similar to the system app with window events for applicationinstall / applicationuninstall .

As for the performance test I didn't quite nail it.

|APP=settings make test-perf| has some runtime bugs, but printing the results gave me:

With patch:
[ { time: 522, type: 'w' },
  { time: 522, type: 'w' },
  { time: 522, type: 'w' },
  { time: 522, type: 'w' },
  { time: 939, type: 'c' },
  { time: 939, type: 'c' },
  { time: 939, type: 'c' },
  { time: 939, type: 'c' },
  { time: 869, type: 'c' },
  { time: 869, type: 'c' },
  { time: 869, type: 'c' },
  { time: 869, type: 'c' },
  { time: 863, type: 'c' },
  { time: 863, type: 'c' },
  { time: 863, type: 'c' },
  { time: 863, type: 'c' },
  { time: 870, type: 'c' },
  { time: 870, type: 'c' },
  { time: 870, type: 'c' },
  { time: 870, type: 'c' } ]

Without patch:
[ { time: 437, type: 'w' },
  { time: 437, type: 'w' },
  { time: 437, type: 'w' },
  { time: 803, type: 'c' },
  { time: 803, type: 'c' },
  { time: 803, type: 'c' },
  { time: 789, type: 'c' },
  { time: 789, type: 'c' },
  { time: 789, type: 'c' },
  { time: 786, type: 'c' },
  { time: 786, type: 'c' },
  { time: 786, type: 'c' },
  { time: 792, type: 'c' },
  { time: 792, type: 'c' },
  { time: 792, type: 'c' } ]

So there seems to be startup issues with iterating over the apps like this.

Comment 6

[[:fabrice] Fabrice Desré]

If load time regressed by almost 100ms that's quite bad for sure :( Not sure how to that differently though since the entry is in the first panel.

Comment 7

[Olav Nymoen [:olav]]

As per https://groups.google.com/forum/#!topic/mozilla.dev.gaia/lnYs9kjUtKE I moved all mozApps.mgmt into a lazyloaded script and feel stupid for not doing that earlier :)

Comment 8

[Cristian Rodriguez (:crdlc)]

Hi Olav, IMHO I am not the correct person to review code in Settings app, the part of the  homescreen sounds good for me. Does someone tell you that I should review it? or could we move this review to other person who more knowledge on Settings app?

Comment 9

[Olav Nymoen [:olav]]

I was told Fabrice was the wrong reviewer.

Somewhere between Fernando Rodriguez Sela on the Settings app and you on the Homescreen app and my patch for adding homescreen section to settings I mixed things up :)

I'll move it along!

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

We need this patch in koi since there is no way to point of having this section here since homescreen are still certified app in this version.

Comment 11

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> We need this patch in koi since there is no way to point of having this
> section here since homescreen are still certified app in this version.

Agreed during triage.

Comment 12

[Alex Keybl [:akeybl]]

This bug appears to be tracking the fact that you can only have one homescreen, so there's an unnecessary Settings section. Please just remove the section for 1.2, rather than doing anything fancier.

When will this work be done? Hopefully in the next day.

Comment 13

[Olav Nymoen [:olav]]

PR up at https://github.com/mozilla-b2g/gaia/pull/13162 against master

This only removes the UI for selecting the section, not the logic for changing homescreens itself.

Should it instead be targeted directly at 1.2 branch? If so, should all logic for replaceable homescreens go with it?

FYI: https://github.com/mozilla-b2g/gaia/pull/11271 against master still applies nicely :)

Comment 14

[Olav Nymoen [:olav]]

Attachment: Remove homescreen section links from settings

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 823834 [details] [review]
Remove homescreen section links from settings

This patch just remove the Homescreen section in the panel for 1.2. Let's not land that in 1.3 please, but only in 1.2.

Comment 16

[Olav Nymoen [:olav]]

Attachment: Remove homescreen section links from settings in 1.2

Comment 17

[Olav Nymoen [:olav]]

Merged github.com/mozilla-b2g/gaia/pull/13180 into v1.2

Added vingtetun as reviewer for github.com/mozilla-b2g/gaia/pull/11271 against master

Comment 18

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

According to comment 15 this bug should only be fixed in v1.2 right?

I am marking this as fixed so this won't get pop-up in koi+ query...

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Re-opening since the 1.2 patch is mostly done as a quick version of the real fix for shipping purposes.

Comment 20

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> Re-opening since the 1.2 patch is mostly done as a quick version of the real
> fix for shipping purposes.

This will caused this bug to be flagged as unfixed koi+. Do you mind do that elsewhere?

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 783787 [details] [review]
github_pointer

Let's land the real fix.

Comment 22

[Olav Nymoen [:olav]]

Merged! https://github.com/mozilla-b2g/gaia/pull/11271