Ubuntu One Music App for iOS

Merge lp://qastaging/~rockstar/ubuntuone-ios-music/polish-list-views into lp://qastaging/ubuntuone-ios-music

  1. polish-list-views
  2. Merge into trunk
Proposed by Paul Hummer
Status: Merged
Approved by: Paul Hummer
Approved revision: 292
Merged at revision: 249
Proposed branch: lp://qastaging/~rockstar/ubuntuone-ios-music/polish-list-views
Merge into: lp://qastaging/ubuntuone-ios-music
Prerequisite: lp://qastaging/~rockstar/ubuntuone-ios-music/replace-streaming-player
Diff against target: 1800 lines (+1037/-204)
27 files modified
Dependencies/SSPullToRefresh/LICENSE (+20/-0)
Dependencies/SSPullToRefresh/SSPullToRefresh.h (+16/-0)
Dependencies/SSPullToRefresh/SSPullToRefreshDefaultContentView.h (+17/-0)
Dependencies/SSPullToRefresh/SSPullToRefreshDefaultContentView.m (+86/-0)
Dependencies/SSPullToRefresh/SSPullToRefreshSimpleContentView.h (+16/-0)
Dependencies/SSPullToRefresh/SSPullToRefreshSimpleContentView.m (+80/-0)
Dependencies/SSPullToRefresh/SSPullToRefreshView.h (+205/-0)
Dependencies/SSPullToRefresh/SSPullToRefreshView.m (+349/-0)
Music/Models/Album.h (+2/-0)
Music/Models/Album.m (+5/-0)
Music/Storyboard_iPhone.storyboard (+78/-160)
Music/Utilities/UOPlayer.h (+2/-0)
Music/Utilities/UOPlayer.m (+1/-1)
Music/View Controllers/AlbumViewController.h (+1/-0)
Music/View Controllers/AlbumViewController.m (+12/-2)
Music/View Controllers/AlbumsViewController.m (+5/-0)
Music/View Controllers/ArtistViewController.m (+11/-5)
Music/View Controllers/ArtistsViewController.m (+4/-0)
Music/View Controllers/PlayerViewController.m (+2/-2)
Music/View Controllers/PlaylistsViewController.m (+4/-0)
Music/View Controllers/SongsViewController.m (+10/-6)
Music/View Controllers/UOIndexedViewController.h (+1/-0)
Music/View Controllers/UOIndexedViewController.m (+23/-27)
Music/Views/Table Cells/ArtistCell.m (+2/-0)
Music/Views/Table Cells/SongListCell.h (+15/-0)
Music/Views/Table Cells/SongListCell.m (+34/-0)
U1Music.xcodeproj/project.pbxproj (+36/-1)
To merge this branch: bzr merge lp://qastaging/~rockstar/ubuntuone-ios-music/polish-list-views
Reviewer Review Type Date Requested Status
Mike McCracken (community) Approve
Review via email: mp+147028@code.qastaging.launchpad.net

Commit message

Polish the list views

Description of the change

I just did a bunch of tweaks that make things more presentable on the list views and such. I changed the font to be more in line with what design had envisioned in the original settings view (and the new settings view followed suit), and made things a bit more pixel lined up (using Xcode's guides).

I also fixed a bug where you'd navigate from an artist, to a various artist album and the album listing shows all the songs, rather than filtered down to only the artist's songs on that album.

To post a comment you must log in.
Revision history for this message
Mike McCracken (mikemc) wrote :

Separate changes I see in here that could have been multiple branches:
- adding SSPullToRefresh
- songsForArtistID bug fix
- songlistcell art downloading

1. The license for sspulltorefresh probably needs to be included:
https://github.com/soffes/sspulltorefresh/blob/master/LICENSE

1a. Do we need the GPL on each of the source files? That's what we do
in the python apps...

2. Do we localize this app? The hard-coded english pluralization made
me wonder.

3. In SongListCell.m, should the downloader completionBlock also set
song.art? Otherwise it looks like we'll be downloading it every time
it shows up...

review: Needs Information
lp://qastaging/~rockstar/ubuntuone-ios-music/polish-list-views updated
288. By Paul Hummer

Merge from previous pipe

289. By Paul Hummer

Merged replace-streaming-player into polish-list-views.

290. By Paul Hummer

Merged replace-streaming-player into polish-list-views.

291. By Paul Hummer

Reset the detailTextLabel

292. By Paul Hummer

Add SSPullToRefresh LICENSE

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

With small changes, I don't see any problem in putting those changes in the same branch.

(1) Added LICENSE
(1a) I'll add the GPL headers to files prior to release. I'm trying to keep the diff size down, especially with stuff that is not needed in code review.
(2) We aren't currently focused on localization in any of the Ubuntu One properties. It might be nice in the future, but not now.
(3) art is actually a method that checks the filesystem for the file itself, so it won't get downloaded again. There is a chance that it'll get downloaded and re-written, but that's super unlikely at this point, and there are bigger problems with the album art (like storing multiple copies of the default album art...) At some point, I'll make the downloader smarter, and not queue a download if it already is queued.

Revision history for this message
Mike McCracken (mikemc) wrote :

I won't fight you on branch size - I'll just explain my bias toward branches as small as possible.
On the desktop projects, we've tried pretty hard to make separate branches out of separate changes, and I've noticed that they're easier to review, issues with one change don't block landing other changes, and when doing bzr-blame log searching while fixing bugs, big multi-feature branches make tracing and understanding historical changes harder.

We also had a size guideline at one point: we decided to try to keep branches ~500 lines or less, even to the point of committing half of a feature in one branch and the other half in another. I thought that extreme was overkill (obviously e.g. adding a framework with headers is going to be big), and the specific number that makes sense will be different for different languages, but 500 seemed like a good rule of thumb.

Thanks for taking the time to write a detailed reply, too. I appreciate it.

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