1064929 - Music app is basically unusable while indexing

Status: RESOLVED DUPLICATE of bug 1147173

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

Description

[Chris Lord [:cwiiis]]

While the music app is indexing new music, it's pretty much impossible to use. It seems for every new track, it scrolls to the top of the music list, and the non album-art views don't seem to update at all while indexing.

This is especially troublesome on the first startup of the music app when you have a lot of music on an SD card - the app will remain unusable for a very long period, and it's extremely frustrating to try.

Either the app should be made inaccessible during indexing (quick, but nasty work-around), or these issues with interactivity while adding new songs need to be fixed.

Comment 1

[Hubert Figuiere [:hub]]

It is not a nasty workaround to make the music scanning modal. It only happens in the app startup when music needs to be added to the database.

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Hubert Figuiere [:hub] from comment #1)
> It is not a nasty workaround to make the music scanning modal. It only
> happens in the app startup when music needs to be added to the database.

I consider it nasty as the scanning is so slow - it takes *minutes* for my music library (which isn't especially large), and I think it's reasonable to be able to play music as it scans new things. It also seems like weird behaviour if the user were to add a few albums to their phone that they shouldn't be allowed to play any music at all until they're indexed.

Comment 3

[Chris Lord [:cwiiis]]

I was having a look at this, and this seems to be caused by 'hideSearch' being called constantly while the app is updating. There are a few things wrong here:

1- It uses a setTimeout, but doesn't store the result, and so multiple calls to update will end up queueing many calls to hideSearch, instead of delaying the hide on subsequent update calls

2- hideSearch works by scrolling the search box away, but the search is in-line with the contents of the view, so this will scroll the view and possibly override any async scrolling that the content thread hasn't caught up with.

3- The container isn't sized so that hideSearch can be scrolled off if there isn't enough content to fill the page in the view.

I'm going to take a crack at fixing this.

Comment 4

[Chris Lord [:cwiiis]]

Actually, I was wrong, that is still a problem, but the problem is the call to TilesView.clean for every song, which just sets the scroll position to zero... I don't know why the scroll position is reset when calling this function?

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1064929-music-reset-position-on-index > mozilla-b2g:master

Comment 6

[Chris Lord [:cwiiis]]

I was wrong about (3) too, there's no problem there.

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8567254 [details] [review]
[gaia] Cwiiis:bug1064929-music-reset-position-on-index > mozilla-b2g:master

This fixes possible multiple calls of hideSearch, and also doesn't reset the scroll position of the tiles view during indexing.

The experience isn't great as the tiles move around a lot, but it's a lot better than the scroll position constantly moving back to the start.

Comment 8

[Dominic Kuo [:dkuo]]

Chris, thanks for working on this, I haven't get a chance to review this in detail(just a quick look), but do you think it's possible to add some tests for the patch? please let me know if you think it's okay or not easy to add tests for this.

Comment 9

[Chris Lord [:cwiiis]]

I guess we could test that the scroll position isn't reset on the tile view when new music is indexed... I'll look into how easy that is.

Comment 10

[Chris Lord [:cwiiis]]

TL;DR: Can we do without a test?

Ok no, that test is not easy to write... It seems this app has very little/no UI-related testing as it is, and it's pretty hard for someone that's not at all familiar with this code to start writing these, or to decide how best to do it.

Personally, I'd like this test to be a marionette test where we add enough albums to cause the tile view to scroll, we scroll down, record the scroll position, add another album, wait for it to finish adding, then verify that the scroll position is the same.

I think this requires either a lot of sample media, or a server that can generate mp3s on demand (much like the workload music-generator script). Either way, I think this is starting to cause what is otherwise a small polish bug to become something rather huge.

So can we just fix this thing for now and postpone the test part?

Comment 11

[Hubert Figuiere [:hub]]

(In reply to Chris Lord [:cwiiis] from comment #10)
> TL;DR: Can we do without a test?

Would you in a module you own?

> Ok no, that test is not easy to write... It seems this app has very
> little/no UI-related testing as it is,

That's because I have a very long queue of pending tests waiting to be reviewed to land - just to pay this debt off. Bug 1121184.

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Hubert Figuiere [:hub] from comment #11)
> (In reply to Chris Lord [:cwiiis] from comment #10)
> > TL;DR: Can we do without a test?
> 
> Would you in a module you own?
> 
> > Ok no, that test is not easy to write... It seems this app has very
> > little/no UI-related testing as it is,
> 
> That's because I have a very long queue of pending tests waiting to be
> reviewed to land - just to pay this debt off. Bug 1121184.

Sorry, this wasn't meant to be a dig - just it's a hard situation when a tiny polish bug gates on a huge amount of work. I don't think I have the time to figure out the music app, write a load of other UI tests, then write this UI test, and then get that all into 2.2. At least, not without sacrificing the other dozen polish bugs I want to fix. Possibly selfish, but how it is.

This is also a test that we don't do something - I always find the rationale over such tests a bit weird. Are we in danger of adding in code that resets the scroll position when music is indexed again? I'd like to think we aren't, and as such, the test is a bit redundant (should we start testing for other things we don't want this app to do?) It's certainly a nice to have, but I don't think we're in danger of breaking this any time soon (feel free to disagree, of course - if we are, maybe the fix needs to be a bit more comprehensive...)

Also, feel free to take ownership of this bug :) I haven't gotten as much traction as I'd like over the polish issue, especially considering that we (Gaia contributors) voted it as the most wanted thing to address for v3, so any help is appreciated.

Comment 13

[Chris Lord [:cwiiis]]

Also, for some situations, yes, I would allow some bug-fixes to go in without tests... But I should probably be stricter about that :)

Comment 14

[Hubert Figuiere [:hub]]

Just to be clear. For 2.2 I forgo tests. This is no longer a moving target and therefor we carefully cherry-pick.

Comment 15

[Dominic Kuo [:dkuo]]

(In reply to Chris Lord [:cwiiis] from comment #10)
> So can we just fix this thing for now and postpone the test part?

Thanks Chris and Hub, I know it should be hard to add tests so that's why I ask in comment 8, just thought Chris might have some ideas about the tests. But since it looks hard to do it, I think we can land the patch first then filed a new bug for the tests.

I will try to test and review this patch though I am cleaning my review queue, feel free to update more if anyone has ideas.

Comment 16

[Chris Lord [:cwiiis]]

Hi Dominic, any ETA on when you'll be able to get to this?

Comment 17

[Dominic Kuo [:dkuo]]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Hi Dominic, any ETA on when you'll be able to get to this?

Chris, sorry about the pending review and I should have time to review this tomorrow, hope I don't block your work too much.

Comment 18

[Dominic Kuo [:dkuo]]

Update:

Chris, I found although we cancel resetting position of the tiles view while indexing, because the music database is still updating and the tiles view will re-render more, is it what you expect that the scrolled position kept but the tiles will re-arrange? cause for me this is still confusing the users.

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Dominic Kuo [:dkuo] from comment #18)
> Update:
> 
> Chris, I found although we cancel resetting position of the tiles view while
> indexing, because the music database is still updating and the tiles view
> will re-render more, is it what you expect that the scrolled position kept
> but the tiles will re-arrange? cause for me this is still confusing the
> users.

Yes, this is still confusing, but it's a lot less confusing than constantly resetting the scroll position to zero and it allows you to start playing some music if you happen to see the album you want.

This doesn't improve the initial import experience very much, but massively improves the 'just copied a couple of albums on' experience, as the tile view is unlikely to change that much after the first couple of songs have imported and it allows you to carry on using the view without the scroll position being reset on every subsequent song that gets indexed.

Passing back n? for review/comment.

Comment 20

[Dominic Kuo [:dkuo]]

I agree with the idea that we shouldn't reset the scroll position while the music database is still indexing the new songs, this is a good improvement and gave me another idea that, we could just probably add the newly scanned albums below the existing albums, so that we don't need to re-render all the tiles.

And the strategy for rendering the tiles could be:
1. After the music app launched, display the known albums in tiles view.(We already did)
2. Music app scans for new songs/albums every time it launches, after a batch of songs found, only add the new albums after the last tile in tiles view.
3. Because we add the newly found albums from the end of the tiles, we don't have to re-render all the tiles view every time we found some new songs, so resetting the scroll position will not happen and we don't have to prevent it.
4. Next time when music app launches, the newly found albums should be in order because they are indexed in the database already.

I think the newly found albums are added from the end of tiles could be acceptable, cause this is something not confusing and not blocking the users while music app is indexing, though we usually think the newest albums should place on the top of the tiles(but this is not happened yet), we just simply place the tiles in ascending order.

Does it make sense? :)

Comment 21

[Chris Lord [:cwiiis]]

(In reply to Dominic Kuo [:dkuo] from comment #20)
> I agree with the idea that we shouldn't reset the scroll position while the
> music database is still indexing the new songs, this is a good improvement
> and gave me another idea that, we could just probably add the newly scanned
> albums below the existing albums, so that we don't need to re-render all the
> tiles.
> 
> And the strategy for rendering the tiles could be:
> 1. After the music app launched, display the known albums in tiles view.(We
> already did)
> 2. Music app scans for new songs/albums every time it launches, after a
> batch of songs found, only add the new albums after the last tile in tiles
> view.
> 3. Because we add the newly found albums from the end of the tiles, we don't
> have to re-render all the tiles view every time we found some new songs, so
> resetting the scroll position will not happen and we don't have to prevent
> it.
> 4. Next time when music app launches, the newly found albums should be in
> order because they are indexed in the database already.
> 
> I think the newly found albums are added from the end of tiles could be
> acceptable, cause this is something not confusing and not blocking the users
> while music app is indexing, though we usually think the newest albums
> should place on the top of the tiles(but this is not happened yet), we just
> simply place the tiles in ascending order.
> 
> Does it make sense? :)

Yes, makes sense to me :) I wonder if it's worth re-ordering after you switch views though, or even not re-ordering and keeping it in whatever order they get indexed (but only for that view)?

Either way, I think what you suggest is a good step and we shouldn't wait for the perfect solution - I'll see if I can get onto that this week.

Leaving n?me so I don't lose track.

Comment 22

[Dominic Kuo [:dkuo]]

Comment on attachment 8567254 [details] [review]
[gaia] Cwiiis:bug1064929-music-reset-position-on-index > mozilla-b2g:master

Thanks Chris, then after you have some updates on the patch, feel free to set review to me and let's discuss more in detail, I am cancelling the review first so that I can get the review notification again.

Comment 23

[Chris Lord [:cwiiis]]

ok, I've discovered that for every song that gets created, the app rebuilds its entire UI. This basically isn't going to work without significant change or significant hackery, so I'm going to unassign myself for now :(

Comment 25

[Jim Porter (:squib)]

(In reply to Chris Lord [:cwiiis] from comment #23)
> ok, I've discovered that for every song that gets created, the app rebuilds
> its entire UI. This basically isn't going to work without significant change
> or significant hackery, so I'm going to unassign myself for now :(

Technically, it batches it, but I was fixing this anyway in bug 1147173.