[music] Improve the initial-run experience for the music app

RESOLVED FIXED

Status

Firefox OS
Gaia::Music
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

({polish})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
The initial-run experience in the music app is pretty bad. We repeatedly try to hide the search box and reset the scroll position in the Tiles view, meaning you can't actually scroll at all until the scanning process is completely finished. This makes a pretty poor first impression.

I have a patch to fix this, as well as to simplify some of the relevant code so it's easier to follow (and less fragile). Things I did:

1) Use the LazyLoader more consistently, and don't rely on other places having already lazy-loaded a file.
2) Move UI logic out of db.js and into the appropriate files.
3) Simplify TilesView's interface.
4) Add smooth-scrolling for the hideSearch() function.
Created attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Note: the first commit in this is actually for bug 1147032, so you can skip reviewing that here.
Attachment #8582725 - Flags: review?(dkuo)
Attachment #8582725 - Flags: review?(dflanagan)
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Jim: this sounds great to me. I don't feel familiar enough with the part of the Music app to review, so I'd like to leave the review to Dominic.  If you really want my review (or if Dominic) doesn't have time, you can set the r? flag again.

Did you consider using the new 'enumerable' event from musicdb instead of 'ready' as part of this patch? You might get a startup time improvement if you switch to that.
Attachment #8582725 - Flags: review?(dflanagan)
(Assignee)

Comment 4

3 years ago
(In reply to David Flanagan [:djf] from comment #3)
> Did you consider using the new 'enumerable' event from musicdb instead of
> 'ready' as part of this patch? You might get a startup time improvement if
> you switch to that.

I plan to do that in a followup. This patch is mainly about fixing the scrolling issues, and making the code a bit easier to understand. (I'm doing my best to avoid gigantic patches in the music app, since I've done enough of those already! :))
Jim does this relate to bug 1064929 ?
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1064929
(Assignee)

Comment 7

3 years ago
Yeah, same bug, but I had thought that was about the other horrible startup issues I haven't fixed (like how the framerate drops to maybe 10 Hz if you're lucky). Duped.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Updated

3 years ago
Blocks: 1122648

Updated

3 years ago
Keywords: polish

Comment 8

3 years ago
Jim, Bug 1147032 looks like part of this bug so I am going to review that bug first.
(Assignee)

Comment 9

3 years ago
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Ok, I've rebased this onto master. David: Do the changes I've made to the "enumerable" code in db.js look ok to you? I was able to simplify it a bit with the refactoring.
Attachment #8582725 - Flags: feedback?(dflanagan)
(Assignee)

Updated

3 years ago
Depends on: 796422
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Jim,

Yes, those changes look good. Its nice to have only one version of the ready event handler.
Attachment #8582725 - Flags: feedback?(dflanagan) → feedback+

Comment 11

3 years ago
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Hey Jim, I tried to review this patch today but I found it conflicts, would you please update it then set the the review to me again? apologize for being late to review it, thanks!
Attachment #8582725 - Flags: review?(dkuo)

Updated

3 years ago
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Ok, I fixed the bitrot.
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8582725 - Flags: review?(dkuo)

Comment 13

3 years ago
Comment on attachment 8582725 [details] [review]
[gaia] jimporter:music-initial-load > mozilla-b2g:master

Jim,

thanks for working on this, I think this patch does improved the first-run experience of the music app, especially when the users already have many songs in their devices, the annoying resetting TilesView's position, also the scrolling of the search bar looks nice when it becomes smooth. I have few minor github comments and you might want to take a look on them.
Attachment #8582725 - Flags: review?(dkuo) → review+
Created attachment 8607162 [details] [review]
[gaia] jimporter:music-initial-load-no-smooth-scroll > mozilla-b2g:master
(Assignee)

Comment 15

3 years ago
Comment on attachment 8607162 [details] [review]
[gaia] jimporter:music-initial-load-no-smooth-scroll > mozilla-b2g:master

Pulling r+ forward; this PR is just the previous one with the last two commits stripped, since they cause Marionette test failures. I'll file a followup for those.
Attachment #8607162 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8582725 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
No longer depends on: 796422
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#-0Hn05QxRwCrOqcSbJgBTA

The pull request failed to pass integration tests. It could not be landed, please try again.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.