Merge lp://qastaging/~rockstar/ubuntuone-ios-music/replace-streaming-player into lp://qastaging/ubuntuone-ios-music
Proposed by
Paul Hummer
Status: | Merged |
---|---|
Approved by: | Paul Hummer |
Approved revision: | 290 |
Merged at revision: | 248 |
Proposed branch: | lp://qastaging/~rockstar/ubuntuone-ios-music/replace-streaming-player |
Merge into: | lp://qastaging/ubuntuone-ios-music |
Diff against target: |
1495 lines (+773/-133) 19 files modified
Music/Categories/NSString+UbuntuOne.h (+13/-0) Music/Categories/NSString+UbuntuOne.m (+24/-0) Music/Categories/UIImageView+UbuntuOne.h (+13/-0) Music/Categories/UIImageView+UbuntuOne.m (+53/-0) Music/Models/Song.h (+1/-1) Music/Storyboard_iPhone.storyboard (+121/-19) Music/UOMusic.xcdatamodeld/UOMusic.xcdatamodel/contents (+2/-2) Music/Utilities/UOPlayer.h (+48/-0) Music/Utilities/UOPlayer.m (+286/-0) Music/View Controllers/AlbumViewController.m (+9/-15) Music/View Controllers/ArtistViewController.m (+4/-4) Music/View Controllers/PlayerViewController.h (+17/-0) Music/View Controllers/PlayerViewController.m (+105/-30) Music/View Controllers/SongsViewController.m (+7/-14) Music/View Controllers/UOIndexedViewController.m (+4/-4) Music/Views/Table Cells/SongCell.m (+2/-3) U1Music.xcodeproj/project.pbxproj (+26/-0) utilities/StreamingPlayer.m (+1/-1) xibs/SongViewController.xib (+37/-40) |
To merge this branch: | bzr merge lp://qastaging/~rockstar/ubuntuone-ios-music/replace-streaming-player |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mike McCracken (community) | Approve | ||
Review via email:
|
Commit message
Replace StreamingPlayer with UOPlayer
Description of the change
This branch adds the polish to the Player View Controller. Most of the UI is copied directly from the older app. I just learned while working on this that you can tap the album art and get extended controls (scrubbing, shuffle, and repeat). Here's some screenshots.
http://
http://
I also implemented a new player. The one problem I still have is that I can't quite get the app to react to remote control events. I'll look into that once I have the polish applied to the other views.
To post a comment you must log in.
Needs-fixing because of the currentIndex issues in UOPlayer, otherwise please treat my naming comments as friendly suggestions, I won't complain if you decide you don't like them.
General comment: this really could have been split into a few smaller
branches to ease reviewing. For example, these all seem like
independent changes:
- adding the CGImage reflection
- updating duration to int64
- the secondsToClock change
- adding UOPlayer
- maybe also the XIB changes, if only to de-clutter the other branches
1. NSNumber+UbuntuOne:
Why is secondsToClock a category on NSNumber? NSNumber isn't
involved. Why not just make it a function?
Or if you really want a category, maybe a category on NSString omSeconds: "...
like "+clockStringFr
2. UIImage+UbuntuOne: (~MINOR)
reflectedImage:(1) toImageView:(2) wasn't clear from the name what it mageView: (1)" or something.
does. For one, both args are image views but the name makes it sound
like they're different. Also I think it needs a verb, like
"setImageView:(2) toReflectionOfI
Just my 2c, it's up to you.
3. UOPlayer.m:
In -next, I think that the two tests of (currentIndex +1 >
[... count]) might be off by one - if there are ten tracks and I'm on
track #10 (index 9), this will evaluate (10 > 10) = false and set
currentIndex to 10, which will be an invalid index into songs.
In prev, if currentIndex is 0, won't we get an invalid index there
too? I'm not 100% sure what unsigneds do when you do 0-1, but it's
probably not a good idea anyway. What should it do in that case?
Actually, prev looks unfinished, right? Should it always set the state
to stop?
addSongs:atIndex: was a confusing name, and it made it hard to understand withStartingInd ex:".
prepareForSegue in the view controller, because I was wondering what
happened to the indexing if there were already songs in the player
when you called it from prepareForSegue...
But - we're not *adding* songs, we're *replacing* the songs array
completely. I think the method would be much clearer if it was named
something like "setSongs:
(MINOR): IMO, enums are more readable with underscores: e.g. REPEAT_OFF