Stream queue #190
Stream queue #190
Conversation
| @@ -100,6 +100,7 @@ class AddVideosViewController: UIViewController { | |||
| return | |||
| } | |||
| inviteVC.stream = stream | |||
| inviteVC.videoQueue = viewModel.selectedVideos.map{$0} | |||
There was a problem hiding this comment.
I think you can avoid the map and just use viewModel.selectedVideos.
| @@ -20,6 +20,8 @@ class InviteStreamViewController: UIViewController { | |||
| let headerCellHeight = CGFloat(47.0) | |||
|
|
|||
| var stream: Stream? | |||
| // hold for streamVC | |||
| var videoQueue: [Video]? | |||
There was a problem hiding this comment.
Are there different semantics for empty vs. nil? Would it make any sense to make this non-optional instead, but be initialized to an empty array? (And just a reminder that if you change it to be non-optional, there may be properties in other classes that can be non-optional as well, like computed vars that fetch this value.)
There was a problem hiding this comment.
I made it a nil since when it's the participant view the queue isn't just empty, it doesn't exist. It could be an empty array if that would make the code cleaner.
There was a problem hiding this comment.
Ah, okay. That makes sense. Sounds like there are separate meanings for [] and nil then.
| "playsinline" : 1, | ||
| "modestbranding" : 1, | ||
| "showinfo" : 0, | ||
| "controls" : 1 |
There was a problem hiding this comment.
Making a note for myself to remember to check these lines when hiding controls for participants.
| "controls" : 1, | ||
| "playlist": "7D3Ud2JIFhA, 2VuFqm8re5c" | ||
| ]) | ||
| if viewModel.isHost, let queue = viewModel.videoQueue, queue.count > 0 { |
There was a problem hiding this comment.
Cool! I like the way you used multiple conditions here. Simple and easy to read.
| @IBAction func backTapped(_ sender: Any) { | ||
| playerView.previousVideo() | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't quite follow why some of these @IBAction functions were removed, but I'm sure you have good reason. Just noting it here on the off chance that you didn't intend to commit it.
There was a problem hiding this comment.
These are from the debug arrows, they aren't needed anymore and don't work with the new system.
| if tableView.tag == chatTableTag { | ||
| return cellFor(chatTableView: tableView, at: indexPath) | ||
| } | ||
| else { |
There was a problem hiding this comment.
This might be sliiiiightly easier to maintain if you use another if statement instead of an else.
| if tableView.tag == chatTableTag { | ||
| return viewModel.messages.count //TODO: limit this to 50 or whatever performance allows | ||
| } | ||
| else { |
There was a problem hiding this comment.
This might be sliiiiightly easier to maintain if you use another if statement instead of an else.
| } | ||
| else if destinationIndexPath.row == currentVideoIndex { | ||
| viewModel.currentVideoIndex = destinationIndexPath.row + (sourceIndexPath.row > destinationIndexPath.row ? 1 : -1) | ||
| } |
There was a problem hiding this comment.
I don't think these conditions handle the case where a video is moved "over" the current video, from one side to the other.
For example: [video1, video2, current, video3, video4] -> [video1, video3, video2, current, video4]
// left-to-right
if sourceIndexPath.row < currentVideoIndex, currentIndexPath < destinationIndexPath.row {
viewModel.currentIndexPath -= 1
}
// right-to-left
if sourceIndexPath.row > currentVideoIndex, currentIndexPath > destinationIndexPath.row {
viewModel.currentIndexPath += 1
}
| @@ -537,6 +625,12 @@ extension StreamViewController: YTPlayerViewDelegate { | |||
| } | |||
| break | |||
| case .ended: | |||
| if viewModel.isHost, let queue = viewModel.videoQueue, var queueIndex = viewModel.currentVideoIndex { | |||
| queueIndex = queueIndex < queue.count - 1 ? queueIndex + 1 : 0 | |||
There was a problem hiding this comment.
queue.count - 1 is the last index of the array, so I think this will run-off the end if queueIndex is currently pointing to the last item. Consider changing the condition to queue.count - 2 instead.
There was a problem hiding this comment.
The condition is saying it will only increase the queueIndex if the current index is less than queue.count - 1. A bit hard to read but it is correct.
There was a problem hiding this comment.
Gotcha! You're right.
| @@ -23,18 +23,31 @@ class StreamViewModel { | |||
|
|
|||
| weak var delegate: StreamViewModelDelegate? | |||
|
|
|||
| var stream: Stream | |||
| var stream: Stream? { | |||
| didSet { | |||
There was a problem hiding this comment.
Not often that I see didSet. Neat to see it being used.
|
These changes look good! I'll get this design reviewed then merge the changes. |
|
There are some design elements that need to be updated. Since this screen functions correctly, I think it's important to merge the changes and tackle other screens in the app. Then we can return to update the design. So I'm going to merge this PR, but opened #196 to track the necessary design changes. |
* video queue is collected and transfered to streamVC * implemented queue and behaviors * removed skip to stream button * small changes based off feedback
* video queue is collected and transfered to streamVC * implemented queue and behaviors * removed skip to stream button * small changes based off feedback
Resolves #127.
Check with design before merging.