998906 - [Music] display an overlay when music is empty in picker mode

Status: VERIFIED FIXED

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

Description

[Dominic Kuo [:dkuo]]

Attachment: Screenshot: Music empty list in picker mode

I was pinged on IRC by ranjith and thanks to him/her, we know when there is no song in music app's picker mode, no overlay was displayed and the empty list might confuses our users, we should fix this.

Comment 1

[ranjith253]

Attachment: proposedpatch.patch

Hi Dominic,

Need your feedback on the proposed patch.

Comment 2

[Dominic Kuo [:dkuo]]

Comment on attachment 8410029 [details] [diff] [review]
proposedpatch.patch

Review of attachment 8410029 [details] [diff] [review]:
-----------------------------------------------------------------

ranjith, apologize for being late to feedback this, I think the patch is good but need some adjustment, and after I look on it, I realized it's actually a regression, but it's great you found it and fixing it! after you modified the patch, feel free to ask review from me again, thanks!

::: apps/music/js/music.js
@@ +1242,5 @@
> +              }
> +              else {
> +                showOverlay('empty');
> +              }
> +            }

The check point of the enumeration is correct, but probably only check when the record is null, which means the enumeration is finished. Also, you should use showCorrectOverlay() to display the correct overlay, which does the same thing as your code here, and I think we can just copy/move the showCorrectOverlay() from update() to here then it should does its work.

Comment 3

[ranjith253]

(In reply to Dominic Kuo [:dkuo] from comment #2)
> Comment on attachment 8410029 [details] [diff] [review]
> proposedpatch.patch
> 
> Review of attachment 8410029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ranjith, apologize for being late to feedback this, I think the patch is
> good but need some adjustment, and after I look on it, I realized it's
> actually a regression, but it's great you found it and fixing it! after you
> modified the patch, feel free to ask review from me again, thanks!
> 
> ::: apps/music/js/music.js
> @@ +1242,5 @@
> > +              }
> > +              else {
> > +                showOverlay('empty');
> > +              }
> > +            }
> 
> The check point of the enumeration is correct, but probably only check when
> the record is null, which means the enumeration is finished. Also, you
> should use showCorrectOverlay() to display the correct overlay, which does
> the same thing as your code here, and I think we can just copy/move the
> showCorrectOverlay() from update() to here then it should does its work.

Thanks for providing feedback,I think you have suggested two modification.

1. Check only when record is null, It means I need to move my check condition out of this 
    "if (this.dataSource.length === LIST_BATCH_SIZE || !record) " ?  I can do that .
2. I need to replace showOverlay() with showCorrectOverlay(), as you know this function shows overlay based on knownSongs.length.But knownSongs is not updated after enumeration, its done in showCurrentView() in this function we don't have callback support to show the current view. Due to this 
reason I was forced to call showOverlay().I will be very happy to see  any better approach from you.

Comment 4

[Dominic Kuo [:dkuo]]

Attachment: patch - use showCorrectOverlay() to display the overlay

ranjith, as we discussed on IRC, the calling sequence of how we check the dataSource/knownSongs was correct, but because the ListView cleaned the dataSource before every new enumeration, the knownSongs will point to the previous array, so we should set the length to 0 instead of assigning a new empty array to it, please check this patch and see if it works, and thanks for working on this!

Comment 5

[ranjith253]

(In reply to Dominic Kuo [:dkuo] from comment #4)
> Created attachment 8417215 [details] [review]
> patch - use showCorrectOverlay() to display the overlay
> 
> ranjith, as we discussed on IRC, the calling sequence of how we check the
> dataSource/knownSongs was correct, but because the ListView cleaned the
> dataSource before every new enumeration, the knownSongs will point to the
> previous array, so we should set the length to 0 instead of assigning a new
> empty array to it, please check this patch and see if it works, and thanks
> for working on this!


I have tested the patch it works fine.
I think you can take care of landing patch.

Comment 8

[Dominic Kuo [:dkuo]]

Update:

I am working on the integration tests for the patch and apologize for slow response to this bug.

Comment 9

[ranjith253]

Hi Dominic,

I have posted some of my comments on the PR can you address them ?

Comment 10

[Dominic Kuo [:dkuo]]

Hi ranjith, I haven't get time to revisit the patch, but I think you are correct about the dataSource, I should be able to address the issue next Monday and will let you know here, sorry about the slow response! (leaving the needinfo in case I forget this again...)

Comment 11

[ranjith253]

Remanding you about this issue.

Comment 12

[Dominic Kuo [:dkuo]]

Comment on attachment 8417215 [details] [review]
patch - use showCorrectOverlay() to display the overlay

Okay, I think I have added the necessary tests for this issue and ready for review, Jim, would you please review this? thanks.

Comment 13

[Jim Porter (:squib)]

Comment on attachment 8417215 [details] [review]
patch - use showCorrectOverlay() to display the overlay

Tests pass, and this looks reasonable, so r=me!

Comment 14

[Dominic Kuo [:dkuo]]

Thanks Jim.

master: 67f0e561b524f1e80468e9536a65cd7db6fcb8cb

Comment 15

[ankit93040]

Hi Jim

Please uplift these to v2.0 as well.

Thanks!

Comment 16

[Dominic Kuo [:dkuo]]

I think we need approval to land this on v2.0, please read the rules in:
https://wiki.mozilla.org/Release_Management/B2G_Landing

I will request it in the next comment if people want this to be uplifted.

Comment 17

[Dominic Kuo [:dkuo]]

Comment on attachment 8417215 [details] [review]
patch - use showCorrectOverlay() to display the overlay

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): regression.
[User impact] if declined: there is no info for the user if they don't have any songs when picking audio files from the Music app.
[Testing completed]: yes, with new integration tests added.
[Risk to taking this patch] (and alternatives if risky): low.
[String changes made]: none.

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 8417215 [details] [review]
patch - use showCorrectOverlay() to display the overlay

I would not really block on this issue and we are past our deadline to approve non-blocking patches on 2.0 but making an exception here mainly as this is regression (so we may have a oem/carrier partner raise this in certification) and given the low risk.

Also adding verifyme for QA to help with verification here once this lands.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/72e8f07eabbfff0dc1baa457f50eebcf8c2fd18f

Comment 20

[Dominic Kuo [:dkuo]]

Thanks bajaj and Ryan!

Comment 21

[jingmei.zhang]

[Blocking Requested - why for this release]:
I find the issue in 1.4,can we also launch the patch in 1.4?

Comment 23

[Hema Koka [:hema]]

NI Wayne for 1.4 triage. Thanks!

Comment 24

[Dominic Kuo [:dkuo]]

(In reply to jingmei.zhang from comment #21)
> [Blocking Requested - why for this release]:
> I find the issue in 1.4,can we also launch the patch in 1.4?

Release management should/will triage this, thanks.

Comment 25

[Wayne Chang [:wchang]]

I don't see this as a 1.4 blocker as it is already released, there is minimum to no user impact.

Comment 26

[Coler]

This issue has been verified successfully on Flame2.0&2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_Music.mp4

STRs:
  Precondition：Thre is no music file in device.
1. Launch message,create a new SMS.
2. Tap attachment button, select "Music".
**It will switch to "Select a track" view.
3. Check this view.

Actual Result:
3. There is an overlay(Add songs to get started) in this view.

Flame2.0 build version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141202000201
Version         32.0

Flame2.1 build version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0

Comment 27

[Coler]

Attachment: Verify_Flame_Music.MP4