1044752 - [dolphin][flame] Music play abnormal when play a music which does not exist

Status: RESOLVED DUPLICATE of bug 1149930

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

Description

[Peipei Cheng (needinfo if you need my action)]

Steps to reproduce
---------------------------------------------------------------------------
0. Prepare two Songs A and B on SD card
1. Launch Music, you will see both A and B are in music list
2. Play Music B
3. Install file manager from Marketplace
4. Launch File Manager and moved music A from sd card to internal storage
5. Go back to Music app, and try to Play music A
   --> You will see the UI shows A is playing, but actually the sound of music is still B. 

I think this is bcz music list is not refreshed on time. So song A in Music list still links to a non-existing file.
But anyway, we should not showing music A but still playing B.

Comment 1

[Peipei Cheng (needinfo if you need my action)]

Buri V1.3, Flame v1.4 and Dolphin all have this problem.

Comment 2

[jingmei.zhang]

while I move the audio file from sdcard0(external storage) to sdcard(internal storage),and then go to the music to play this audio,it will output such information:
>Content JS ERROR at app://music.gaiamobile.org/shared/js/mediadb.js:939 in getFile/getRequest.onerror: >MediaDB.getFile: NotFoundError

and then I output the path it reads:
>storage.name-sdcard0

That says the app has searched the audio file in a wrong path.

Comment 3

[Peipei Cheng (needinfo if you need my action)]

In Step 4, if user directly remove A using File Manager, the same behavior will also appears. So the problem of this issue is: Music didn't handle the case that music file is not found.

Comment 4

[Jason.Liu]

Dear Marvin,
  This si 3rd APP issue, Can you help to push it slove asap ?

Comment 5

[Wayne Chang [:wchang]]

(In reply to Jason.Liu from comment #4)
> Dear Marvin,
>   This si 3rd APP issue, Can you help to push it slove asap ?

I think we should attach this from the music app instead.

However, requesting target fix on 2.1. Too late for any previous release, I dont see this as a blocker.

Comment 6

[Dominic Kuo [:dkuo]]

Currently we don't handle this case in music and I feel this is a same issue as bug 915582, if we fix that we should also consider this case, though I think it's not a blocker because the user can kill music app to recover it to the right state.

We will need some ux inputs to fix bug 915582, but I am not sure if we have time in 2.1 because the music app has the plan in bug 1012613 to work on in 2.1.

Comment 7

[Sri Kasetti]

Dominic,
Is this because we didn't add the delete functionality?
It seems such a basic case that should work.
How difficult is the fix for this.

Comment 8

[Dominic Kuo [:dkuo]]

(In reply to Sri Kasetti from comment #7)
> Dominic,
> Is this because we didn't add the delete functionality?
> It seems such a basic case that should work.
> How difficult is the fix for this.

Probably not because the way we reproduce this issue is to remove some file which is already appear on the ui, but currently there is no event to notify the music app the file is removed then need to refresh the ui. If we have the delete functionality then the music app should be able to know the selected files are deleted then force to refresh the ui, so they are actually different issues.

If this really blocks something(though I think the use case is not common, and re-launch music could fix it), I would suggest just fix bug 915582 which we can treat the missing files as some corrupted files, and how we handle the error cases should be the same, such as: prompt some error message -> refresh the ui -> auto skip to next track, so we don't really need to distinguish what error we are facing from the ux perspective.

It shouldn't be too hard to fix this but just need to confirm the ux behaviours.

Comment 9

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request

Dominic, 
Could you take a look at this patch and give some feedback? thanks.

Comment 10

[Dietrich Ayala (:dietrich)]

This definitely should be fixed.

However, this isn't a regression and doesn't meet any of the other criteria outlined for blockers at https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines, and has already shipped on devices which passed certification. So triage decided we are not holding the release for it.

-> backlog.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 8479728 [details] [review]
Pull request

Sherman, thanks for working on this. I reviewed the patch and found you just stop the player when ever the MediaDB(getFile()) gets error, that should works but probably not the best flow we could have. As the flow I suggested earlier in comment 8, we can keep playing without interrupting the user's playlist, if there is any error occurs, also, the user probably won't notice there was an error because it just automatically skip to the next track, which is nice for the user I believe.

This is just my idea and let's have ux people to give some inputs here :)

Comment 12

[Dominic Kuo [:dkuo]]

Hey Jacqueline, we need some inputs about handling the errors in the music app, would you please help here? thanks!

Comment 13

[Jacqueline Savory [:jsavory] UX]

I agree with Dominic that the music player should not stop on an error file but simply skip it and continue playing. 

I think it would be best if the user received a status bar indicator, explaining that there was something wrong with the file. For example the message in the status bar would read "An error has occurred with <file name>". This way the user is not interrupted but will see that there was an error. 

If the user is not in the music app, we could send a notification and proceed with the next track as suggested. However, I believe we have missed string freeze so I don't think we will be able to add the error message for 2.1.

Comment 14

[Sherman Chen [:chens] (inactive)]

Attachment: pull request

Hi Dominic, I've update the PR and try to play next music when possible, could you take a look and give some feedback?

Comment 15

[jingmei.zhang]

hi Sherman,

I have tested the patch in my locale.
it works fine.when the file is onerror,it just skips and goes on playing.

Thank you!!

Comment 16

[jingmei.zhang]

(In reply to Sherman Chen [:chens] from comment #14)

hi Sherman,

After testing in my locale for many times,I think there is a something improper for the patch.
1.when we choose a broken file.we will skip to play the next file.it works fine.
2.when we go back to the song list,as we have not updated the song list,the broken file will exist all the time,and this will cause dab effect on the user.

so ,could we add something to update the song list everytime we go back to the song list from the player??

Comment 17

[Sherman Chen [:chens] (inactive)]

Thanks, I've also notice this and still figuring it out how to update the song list. 
But I don't think the broken file will exist all the time, because music db is scanned again once we have problem on file, so if you change to other tabs you can find the broken file will disappear.

Comment 18

[jingmei.zhang]

(In reply to Sherman Chen [:chens] from comment #17) 
> But I don't think the broken file will exist all the time, because music db
> is scanned again once we have problem on file, so if you change to other
> tabs you can find the broken file will disappear.

yes,if we change to other tabs,the broken file will disappear.
but if we stay in the current tab,song list will not update,this should be a issue.
Should we update the song list evertime the music visibility is true?
1.scan the music db when music visibility is true.
2.if there is no change,keep the cuurent state,if there are any new files inserted or delete,the update the song list.
Is that ok?

Comment 19

[Sherman Chen [:chens] (inactive)]

Thanks for the feedback, and as comment 17 described, still investigating and will update once we have some progress on it.

Comment 20

[Sherman Chen [:chens] (inactive)]

Hi Dominic, 

I've update the patch, music player will try to play next possible music when it comes to a non-existent music file, and also force update previous view in respond to the changes. Could you take a look and give some feedback? thanks!

Comment 21

[jingmei.zhang]

Attachment: bug1044752.patch

hi Sherman,I have learned your patch in the pull request.it seems a little comlicate.
in my locale,I have maded a little adjust on the top of your previous patch.

So,would you please give some advices?
Thank you very much!!

Comment 22

[Sherman Chen [:chens] (inactive)]

If you call showListView when a broken file comes, it can play next music though, but it also change current view from player to list view. Another concern is user might come from list view, sub list view, search view and so on, simply update list view will not update results in other views.

Comment 23

[jingmei.zhang]

(In reply to Sherman Chen [:chens] from comment #22)
> Another concern is user might come from list view, sub list view, search view and so
> on, simply update list view will not update results in other views.

Thank you so much,Sherman.I have not consider this case in the patch.
Thank you~

Comment 24

[jingmei.zhang]

hi Sherman,I have tested in my locale.May be there are still something needed to modify.
for a extreme case:
1.choose the album tab
2.play a album which contains only a song
3,remove the song from the sdcard storage to internal storage
4.press the back button to play the song.
then we can still not play the song.
can you help to check the issue?

Comment 25

[jingmei.zhang]

hi Sherman,just my opinion for your patch,if it is wrong,let me konw.
>musicdb.ondeleted = function(event) {
>+ if (ModeManager.currentMode === MODE_PLAYER) {
>+ ModeManager.forceRefresh = true;
>+ }
I do not think we should always stay on the MODE_PLAYER mode when the issue occur.if the broken file is the last file in the song list,then we will back to the MODE_SUBLIST or MODE_LIST mode.

Comment 26

[Sherman Chen [:chens] (inactive)]

I've made some update and separate UI refresh part, could you check that?

Comment 27

[Dominic Kuo [:dkuo]]

Sherman, sorry to keep you waiting, the phase 1 of music refactoring(bug 1055043) has landed and that was a huge patch with many changes, would you please rebase you patch first? thanks!!!

Comment 28

[Sherman Chen [:chens] (inactive)]

Thanks, I will rebase and update PR later.

Comment 29

[Dominic Kuo [:dkuo]]

Comment on attachment 8505269 [details] [review]
pull request

Cancel the review flag first because patch needs update.