1042105 - [User Story] Icon prioritization in the status bar

Status: RESOLVED FIXED

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

Comment 1

[Rob MacDonald [:robmac]]

The latest version of the UX spec is located here:

https://mozilla.box.com/s/tu5wlt6z385dh44kr5my

- Rob

Comment 2

[Guillaume Marty [:gmarty]]

FYI, my WIP branch is here: https://github.com/gmarty/gaia/tree/Bug-1042105-Icon-prioritization-status-bar

Comment 3

[Guillaume Marty [:gmarty]]

Attachment: Github wip branch

Hey Michael, can you have a quick look at the work and see if the approach looks good to you. It's working but need optimisations (like caching icon width to avoid workflows each time) and some edge cases need to be addressed.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8464807 [details]
Github wip branch

I'm giving it a fb+, since I think the overall approach of measuring the width of space available and then showing/hiding icons by priority is sound.

I do have some concerns about some of the implementation details, and I left those as comments on github. Let me know if you have any questions.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Video] Statusbar transition with rocketbar

Btw Guillaume, here is what the transition should look like when opening and closing the rocketbar. You don't need to do this in your patch obviously, but it is something to keep in mind.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

Just as an update to anyone who is following along: we are going to go with a different approach suggested by Vivien where we have 2 versions of the statusbar, one minimized and one maximized. This will make it easier to achieve the transition shown in attachment 8465121 [details].

Comment 7

[Guillaume Marty [:gmarty]]

A wip of the new approach is in this branch:
https://github.com/gmarty/gaia/tree/Bug-1042105-Icon-prioritization-Dual-Status-bar

Comment 8

[Guillaume Marty [:gmarty]]

Attachment: Github branch

Michael, can you please give me your feedback on the new approach?
To try this patch, you must also apply the patch by Vivien from https://bugzilla.mozilla.org/show_bug.cgi?id=1047988 (no conflicts).

Comment 9

[Guillaume Marty [:gmarty]]

Attachment: Github branch

I tried to work on an implementation that doesn't clone the status bar on each icon refresh, as suggested by Vivien. But it turns out there is a lot of code duplication and it's harder to read if we want to maintain performance.

I kept this work in a separate branch. Michael, can you have a quick look at that branch too? I'm pretty sure it's too messy to continue in this way, but wanted your thought on that.

So maybe for the time being keep the previous code (that clones a DOM element regularly) and work on this optimisation later.

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8467159 [details]
Github branch

Left some comments on github. Overall though, no matter what approach we take we will need something like this.

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8467881 [details]
Github branch

I see what you mean by messy. There are some things we can do to clean make this neater I think. What was Vivien's reason for wanting to not do cloneNode? Performance? Has this been measured?

Comment 12

[Guillaume Marty [:gmarty]]

Attachment: Github PR

Because I haven't noticed any performance issues on a 256Mb Flame and we want this to land ASAP, I think that's better to start with this DOM element cloning approach.
We can open a followup bug to measure and fix potential performance issues.

Alive, can you have a look at this PR?

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8468446 [details] [review]
Github PR

I am not sure what is going on and I am surprised that there no tests for lots of new code and the reason is "we need to land it asap".

I cannot review something like this without strict follow up plan.

If the plan is totally rewrite because of spec change tremendously and it is worthy to, what's the reason to keep this?

Land something quickly to unblock other's work sounds fairly nice, but as my experience people won't care the code landed anymore because the feature completed and just works. Sorry, please find another reviewer if you want to land it anyway. Vivien seems the one better than me.

Comment 14

[Guillaume Marty [:gmarty]]

Comment on attachment 8468446 [details] [review]
Github PR

I picked you because you had an empty review queue, but you are totally right, Vivien is a more appropriate reviewer here.
I fixed the test suite so that it works with the code while I'm writing new ones to test the new features in a followup bug.

Vivien, can you please review this PR?

Comment 15

[Guillaume Marty [:gmarty]]

Now we have proper unit tests for that feature, see the PR for details.

Comment 16

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

Comment on attachment 8468446 [details] [review]
Github PR

The code needs a bit more love but overall the approach should work. The only cases where I'm worried about cloning everything is when there is some network activity.

When a web page loads it will trigger some network activity, and this icon will be added / removed multiple times. That may slow down page loading as it will results into relatively big changes into the DOM.

We should probably land this code as if (with the comments addressed) and then open a followup to find a better way of updating the icons in order to optimize this use case and avoid to regress page load.

Feel free to ask r? again once the comments are addressed.

Also I discussed a bit with gmarty on IRC and some of the icons may need more adjustment in the minimized case. For example the battery icon could be vertical instead of horizontal in this case, which will save some spaces.
Also the second SIM indicator does not dissapear yet if there is no SIM into it. That's probably a separate issue though, but it worth keeping tracks of those.

Comment 17

[Guillaume Marty [:gmarty]]

Comment on attachment 8468446 [details] [review]
Github PR

Vivien, I addressed your remarks in the latest version of this PR and left some comments in the obsolete one.

Can you please review it again?

Comment 18

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

Comment on attachment 8468446 [details] [review]
Github PR

r+ with nits.

Comment 19

[Guillaume Marty [:gmarty]]

Landed in https://github.com/mozilla-b2g/gaia/commit/8f955d80d175e52283275d3197e4eae2574b389f

Comment 20

[Kevin Grandon :kgrandon]

Pains me to do this as this is a super-important feature for 2.1, but we've had to back this out due to Gu test timeouts[1]. It looks like the time between test runs takes longer. Note: we were also seeing this in the other statusbar patch that Mike was working on. The timeout is also apparent in the gaia-try that this originally landed with as well [2]. Ni on Mike and Guillaume for notification.

[1] https://github.com/mozilla-b2g/gaia/commit/e78e62125eb43c3a28cdc047987ba54430694a2f
[2] https://tbpl.mozilla.org/?rev=620a053bac02aff3bcd22a66b475cd8ea31f1ead&tree=Gaia-Try

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

As an user, I am really, really surprised that the time's priority is lower than some icons.
https://bugzilla.mozilla.org/show_bug.cgi?id=1052333

Could we have a permanent time?

Comment 22

[Peter Dolanjski [:pdol]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> As an user, I am really, really surprised that the time's priority is lower
> than some icons.
> https://bugzilla.mozilla.org/show_bug.cgi?id=1052333
> 
> Could we have a permanent time?

Take a look at the spec above which defines the priorities:
https://mozilla.box.com/s/tu5wlt6z385dh44kr5my

What I'm seeing is that there seems to be enough room for the time, yet it's not being shown. (particularly when on the homescreen - the entire width should be used, per spec)  
I think there is still some tweaking going on, but we may need to make some modifications once we can play with it more.

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks Kevin for filing bug 1052241. This is now a huge priority since it blocks a bunch of work for 2.1.

Comment 24

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] reland icon prioritization

Now that bug 1052241 has been fixed, let's see if the icon stuff passes gaia-try.

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/6d02931dc9248edf66207066a0c01ebd3e279f8d

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Spec 2.1] Statusbar Icon Priority

Attaching spec so I can link to it.