-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Playlists #90
Conversation
d51237b
to
e408283
Compare
wops triggered travis, I was preparing another branch with that, but got distracted finishing up the hash thing on this one. I'll get around to it this week |
@@ -2,3 +2,4 @@ github "jdg/MBProgressHUD" ~> 0.9.2 | |||
github "cbpowell/MarqueeLabel" ~> 3.1.4 | |||
github "pixelogik/ColorCube" "master" | |||
github "dennisweissmann/DeviceKit" ~> 1.7 | |||
github "GianniCarlo/SwiftReorder" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered a PR for this to the upstream repo? I think it will make maintaining this easier in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered it, but I ran into a problem making the original repo Carthage compatible, since I wanted to try it right away, I brute forced it in my fork recreating the whole project and just adding the library files without the example. There's an improvement besides the addition of the hover/drop callback events, in the original the calculation for the reorder was taking into consideration all the visible cells, when it's only necessary to take into account the adjacent cells. I'll get around to it someday 🤔
@@ -11,7 +11,7 @@ import AVFoundation | |||
import MediaPlayer | |||
|
|||
class PlayerManager: NSObject { | |||
static let sharedInstance = PlayerManager() | |||
static let shared = PlayerManager() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I welcome this, I wonder if it would be wise to do this in a separate PR, it makes this one quite complex.
@@ -31,12 +31,13 @@ class ChaptersViewController: UIViewController { | |||
|
|||
extension ChaptersViewController: UITableViewDataSource { | |||
func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int { | |||
return self.book.chapters.count | |||
return self.book.chapters!.count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to update this for the future, passing only a list of chapters into the ChaptersViewController
as this VC does not need to have knowledge of a book.
However when playlists are at play, we should consider showing a section per file in the playlist, so maybe we need to pass an list of lists here? Maybe there is a more appropriate data structure for this than simple arrays.
convenience init(title: String, books: [Book], context: NSManagedObjectContext) { | ||
let entity = NSEntityDescription.entity(forEntityName: "Playlist", in: context)! | ||
self.init(entity: entity, insertInto: context) | ||
self.identifier = title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not feel like a sound solution. The title will most likely be editable in the future. Use a UUID if an identifier beyond the objectID
which I don't think should be necessary.
So I think we can simply drop the identifier
completely here.
I do understand it's used in the PlayerManager for legacy reasons, but we should remove it's usage there as well as CoreData objects can be compared in a much more direct way than we were able to with the previous approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see this comment. that init is only called when the book is created for the first time, after its content is processed and the hash generated, the identifier is updated to reflect the hash. Mmmm this could be reworked, and instead of creating the book and comparing with the existing ones, I could just compare the hash that I'll be getting anyway 🤔
|
||
public class Book: LibraryItem { | ||
var fileURL: URL! | ||
var asset: AVAsset! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this will be quite a task, but do we actually need to retain the asset here?
We only need the asset to read the meta data which in turn will be stored into our data model. After that we only need the asset that is currently played. I assume that should reduce the number of assets kept in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid point, this can be reworked definitely
@@ -102,6 +104,18 @@ class PlayerManager: NSObject { | |||
} | |||
|
|||
self.currentBook.currentTime = audioplayer.currentTime | |||
let percentage = round(self.currentBook.currentTime / self.currentBook.duration * 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather consolidate any percentage calculations in one place as in the previous Book model and move the notification there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted. We can make the percentage
a getter property then, because we can't have a didSet
for currentTime
since it's a managedObject
@NSManaged public var start: Double | ||
@NSManaged public var duration: Double | ||
@NSManaged public var index: Int16 | ||
@NSManaged public var book: Book? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ?
necessary here? Is there any chance a chapter could be created without a book?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generated stubs from CoreData. I'll change it in code and see if it breaks
@@ -99,14 +99,14 @@ class PlayerViewController: UIViewController, UIGestureRecognizerDelegate { | |||
self.metaViewController?.book = currentBook | |||
self.controlsViewController?.book = currentBook | |||
self.progressViewController?.book = currentBook | |||
self.progressViewController?.currentTime = UserDefaults.standard.double(forKey: currentBook.identifier) | |||
self.progressViewController?.currentTime = currentBook.currentTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to set this separately any more, this can be moved to the didSet
in the PlayerProgressViewController
.
@@ -120,11 +120,15 @@ class PlayerViewController: UIViewController, UIGestureRecognizerDelegate { | |||
let blur = UIBlurEffect(style: colors.isDark ? UIBlurEffectStyle.dark : UIBlurEffectStyle.light) | |||
let blurView = UIVisualEffectView(effect: blur) | |||
|
|||
blurView.frame = backgroundImage.frame | |||
// blurView.frame = backgroundImage.bounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comments can be dropped, having the constraints works for me as well.
@@ -57,7 +58,7 @@ extension ChaptersViewController: UITableViewDataSource { | |||
extension ChaptersViewController: UITableViewDelegate { | |||
func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) { | |||
// Don't set the chapter, set the new time which will set the chapter in didSet | |||
PlayerManager.shared.jumpTo(self.book.chapters[indexPath.row].start) | |||
PlayerManager.shared.jumpTo((self.book.chapters![indexPath.row] as! Chapter).start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That are quite some !
for a single line, maybe guard
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
2a958d5
to
c28f42c
Compare
c28f42c
to
c96d69f
Compare
merged! 🎉 |
RootViewController
which enables us to have the mini player floating across library and playlist VCsLibraryViewController
intoBaseListViewController
to be reused forLibraryViewController
andPlaylistViewController
TODO:
Any suggestions are welcome