1015247 - [Music] Update to use gaia-header

Status: RESOLVED FIXED

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

Comment 1

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 2

[Yan Or]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Code looks good.  On use "!important" for the custom icon, check out the changes to gaia_header/style.css I'm doing here:

https://github.com/mozilla-b2g/gaia/pull/19743/files

That should make it possible to set custom icon image.

Comment 3

[Wilson Page [:wilsonpage]]

squib: Comments addressed :)

Comment 4

[Jim Porter (:squib)]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Unfortunately, this doesn't seem to work right. See my comments on GitHub.

Comment 5

[Wilson Page [:wilsonpage]]

squib: Due to some of the difficulties around web-component platform support we went ahead and made the header changes to the existing header building-block. This meant that is conflicted with our web-component patches (like this one).

In v2.1 we will be merging the building-block work into the gaia-header web-component, then this patch will need amending.

Sorry for the confusion!

Comment 6

[Wilson Page [:wilsonpage]]

squib: Patch looks good to me in normal use, but need some info from you on how to invoke the Music app via an activity to check all looks good there.

As gaia-header ships with the gaia-icons icon-font, I thought it would be worth switching out the other PNGs while I was at it.

Comment 7

[Jim Porter (:squib)]

If you want to load the music app as a pick activity, the easiest way is probably Settings -> Sounds -> Manage Ringtones -> "+", which will let you pick a song to turn into a ringtone.

Comment 8

[Jim Porter (:squib)]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Some issues I see with the latest version:

* The "play" icon at the top right shows up even when there's nothing playing. It should be hidden in this case. This also applies to the pick activity.

* When the app starts up, the first tab at the bottom (the mix tab) isn't colored blue. Selecting others tabs colors the icon blue, though.

* Tapping the "play" icon at the top right to return to the "now playing" card results in a weird glitch in the bottom left corner. Part of the "mix" icon is visible.

Comment 9

[Wilson Page [:wilsonpage]]

(In reply to Jim Porter (:squib) from comment #8)
> Comment on attachment 8431691 [details] [review]
> pull-request (master)
> 
> Some issues I see with the latest version:
> 
> * The "play" icon at the top right shows up even when there's nothing
> playing. It should be hidden in this case. This also applies to the pick
> activity.

Fixed this to be hidden by default.

> * When the app starts up, the first tab at the bottom (the mix tab) isn't
> colored blue. Selecting others tabs colors the icon blue, though.

Fixed this by making sure the app starts at index.html#mix

> * Tapping the "play" icon at the top right to return to the "now playing"
> card results in a weird glitch in the bottom left corner. Part of the "mix"
> icon is visible.

I can't see this glitch. I see the player slide in from the right to cover the currently active UI.

Comment 10

[Dominic Kuo [:dkuo]]

Wilson, I found this is actually a big patch, including the bower part which I need to figure out how it works, so it might take some time for me to review it, just let you know first :)

Comment 11

[Wilson Page [:wilsonpage]]

(In reply to Dominic Kuo [:dkuo] from comment #10)
> Wilson, I found this is actually a big patch, including the bower part which
> I need to figure out how it works, so it might take some time for me to
> review it, just let you know first :)

FWIW you can ignore everything inside bower_components/. As I said if you don't want to use Bower I can refactor the patch to use shared/.

Comment 12

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Updates:

- Fixed 'open' activity
- Updated to latests icon-font ready for performance boost when gecko patch lands
- Rebased against master

Comment 13

[Dominic Kuo [:dkuo]]

Attachment: Player_screenshots_after_patch.png

Comment 14

[Dominic Kuo [:dkuo]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Wilson,

Thanks for addressing the previous issues, though I sill found some and please see attachment 8470665 [details], here are the issues after the updated patch:

1. I think the back button has cover a little bit on the song title, it's probably the italic issue and maybe we should give a margin to prevent that.
2. If the song title overflows, it should be ellipsis.
3. The player icon should be hidden when it's in the player page.
4. The background color seems different from the current one.

These are noticeable issues so we better address them before landing it, would you please check it again? thanks!

Comment 15

[Wilson Page [:wilsonpage]]

(In reply to Dominic Kuo [:dkuo] from comment #14)
> 1. I think the back button has cover a little bit on the song title, it's
> probably the italic issue and maybe we should give a margin to prevent that.
> 2. If the song title overflows, it should be ellipsis.

The new header 'font-fit' logic takes care of the text sizing. It's quite complex, but essentially it will shrink text until it can't shrink anymore, then it will apply ellipsis. I think positioning in attachment 8470665 [details] is correct, but there is some strange sub-pixel font rendering issue on the first and last letters.

This looks like a platform/rendering issue. I'm not really sure how to address this.

> 3. The player icon should be hidden when it's in the player page.

Ah, I was unaware of this, I will fix.

> 4. The background color seems different from the current one.

This is by design. All media apps should have the same header color now. This instruction is from Visual Team.

Comment 16

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Updated

Comment 17

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

dkuo: all looks good to me now, could you take another look?

Comment 18

[Wilson Page [:wilsonpage]]

dkuo: You able to take another look?

Comment 19

[Dominic Kuo [:dkuo]]

Attachment: Music_title_not_ellipsis.png

Comment 20

[Dominic Kuo [:dkuo]]

Wilson, thanks for addressing those issues in comment 14, though I still see the title is not ellipsis in attachment 8472840 [details], looks like the 'font-fit' logic miscalculated the width then caused this problem?

Comment 21

[Wilson Page [:wilsonpage]]

Attachment: fixed-ellipsis.png

I got to the bottom of the issue. If was related to `text-overflow: hidden` not working on `display: flex` elements.

Comment 22

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

UPDATED

- Fixed font-fit logic in gaia-header and updated to v0.3.6
- Updated gaia-icons v0.5.0

Comment 23

[Wilson Page [:wilsonpage]]

dkuo: Think were good now. See attachment 8472960 [details].

Comment 24

[Wilson Page [:wilsonpage]]

I also noticed the search button had a strange default background shaded background image on so this has been fixed too.

Comment 25

[Dominic Kuo [:dkuo]]

Wlison, I think we almost there and I would like to take a last look before r+ing this patch, would you please address those new issues I found on github then notify me again? thanks!

Comment 26

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

UPDATED

- Changed manifest to point entry-points to index.html#mix
- Fixed missing shuffle and play icons in artist song list
- Removed commented out CSS
- Removed extra `navigator.mozl10n.get()` call

Comment 27

[Stephany Wilkes]

Marking this bug for 2.1 since the web components Header is the one committed web component for 2.1. Other web components for 2.2 depend on Header going first. 

Sorry for doing this late; Hema just brought this bug to my attention today, but it reflects agreed scope and is in the agreed 2.1 plan. Just updating the flags appropriately for release tracking.

Comment 28

[Dominic Kuo [:dkuo]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

Thanks a lot, Wilson! I think the patch looks good to me now, only some minor issues to address before landing it.

Also bug 837675 is landed, do you think it's possible for us to measure the startup time before/after this patch applied? I can do it if you are still working on the other gaia-header patches.

Comment 29

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

UPDATED

- Addressed all Github comments

Comment 30

[Wilson Page [:wilsonpage]]

dkuo: It would be great if you could look at startup perf! You must make sure that Music app is using the same version of gaia-icons as shared/elements/gaia-icons to get maximum startup perf. This is because Gecko pre-loads the shared/ gaia-icons font. You can check the gaia-icons/bower.json to be sure.

You make need to do a `make reset-gaia` to make sure the pre-loading has taken place. Ping me if you have any questions.

Comment 31

[Dominic Kuo [:dkuo]]

(In reply to Wilson Page [:wilsonpage] from comment #30)
> dkuo: It would be great if you could look at startup perf! You must make
> sure that Music app is using the same version of gaia-icons as
> shared/elements/gaia-icons to get maximum startup perf. This is because
> Gecko pre-loads the shared/ gaia-icons font. You can check the
> gaia-icons/bower.json to be sure.
> 
> You make need to do a `make reset-gaia` to make sure the pre-loading has
> taken place. Ping me if you have any questions.

Thanks Wilson, I think I should be able to perf the startup time tomorrow and will update the status here.

Comment 32

[Wilson Page [:wilsonpage]]

Comment on attachment 8431691 [details] [review]
pull-request (master)

LANDED https://github.com/mozilla-b2g/gaia/commit/313384274a7158d222f41e6fe9a2ebb149bd8049

Comment 33

[Wilson Page [:wilsonpage]]

I've had to land as we committed to landing by today. We'll use Datazilla to track any performance regressions and handle them in the stability sprint.