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
Reviewer Review Type Date Requested Status
Mike McCracken (community) Approve
Review via email: mp+147017@code.qastaging.launchpad.net

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://ubuntuone.com/1Qba6PUSOgHgk6bNp29ktt

http://ubuntuone.com/7E2Eq9pSTdCYgmC8co7mY0

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.
Revision history for this message
Mike McCracken (mikemc) wrote :

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
like "+clockStringFromSeconds:"...

2. UIImage+UbuntuOne: (~MINOR)

reflectedImage:(1) toImageView:(2) wasn't clear from the name what it
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) toReflectionOfImageView:(1)" or something.

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
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:withStartingIndex:".

(MINOR): IMO, enums are more readable with underscores: e.g. REPEAT_OFF

review: Needs Fixing
Revision history for this message
Paul Hummer (rockstar) wrote :

Yeah, I noticed the problems with currentIndex yesterday, but there's also another problem where we're not fetching enough of the songs from the U1AutoDownloadsManager (that needs to be replaced because it's both not smart enough and too smart...)

283. By Paul Hummer

Move NSNumber+UbuntuOne to NSString+UbuntuOne

284. By Paul Hummer

Move [NSNumber secondsToClock] to [NSString secondsToClock]

285. By Paul Hummer

Rename secondsToClock to clockStringFromSeconds

286. By Paul Hummer

Move UIImage+UbuntuOne to UIImageView+UbuntuOne

287. By Paul Hummer

Change the reflection method to be a UIImageView category method

288. By Paul Hummer

Fix downloadManager bug and the off by one errors in next/prev

289. By Paul Hummer

Reset album art image view

290. By Paul Hummer

Merge in trunk, resolve conflicts

Revision history for this message
Paul Hummer (rockstar) wrote :

Changes have now been made. I found a few other bugs while I was testing this, which I fixed here, because that was most appropriate.

Revision history for this message
Mike McCracken (mikemc) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches