Skip to content
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

Update design of video queue #281

Open
5 tasks
glennrfisher opened this issue Feb 8, 2017 · 1 comment
Open
5 tasks

Update design of video queue #281

glennrfisher opened this issue Feb 8, 2017 · 1 comment
Labels

Comments

@glennrfisher
Copy link
Contributor

  • Number the videos.
  • Match the color of (-) icon and delete button to the color of the "Add Videos" button.
  • Change "Delete" to "Remove."
  • Replace number / play icon with (-) icon when editing (i.e. don't move the thumbnail or text).
  • Make sure the previous video always hides its separator.

Numbering Challenges

The number label is already included in the xib, but we decided to hide it because of challenges when moving the rows.

In particular, when we move a row in the table, we need to update the number of all cells between the source row and the destination row. The code below works great when all of the rows to update are on the screen, but it fails when there are rows that need to update but are not currently visible.

// update the row numbering
let first = min(sourceIndexPath.row, destinationIndexPath.row)
let last = max(sourceIndexPath.row, destinationIndexPath.row)
let update = (sourceIndexPath.row < destinationIndexPath.row) ? -1 : 1
for row in first...last {
    let indexPath = IndexPath(row: row, section: 0)
    if let cell = tableView.cellForRow(at: indexPath) as? VideoQueueTableViewCell {
        if row == sourceIndexPath.row {
            cell.number = "\(destinationIndexPath.row + 1)" // set number of moved row
        } else {
            cell.number = "\(row + 1 + update)" // set number of intermediate row
        }
    }
}

The following code does not work, however. It causes visual glitches by trying to reload rows before the move takes place. For the same reason, I suspect simply reloading all rows would fail, too.

// update the video numbers
let first = min(sourceIndexPath.row, destinationIndexPath.row)
let last = max(sourceIndexPath.row, destinationIndexPath.row)
let rows = [Int](first...last).map() { row in IndexPath(row: row, section: 0) }
tableView.reloadRows(at: rows, with: .automatic)

Previous Video Challenges

There's a particular edge case that is not handled by the current code:

  1. Create a stream with 5 videos: [A, B, C, D, E].
  2. Select video C to highlight it.
  3. Notice how video B hides its separator, as desired.
  4. Delete video B.
  5. Notice how video A now hides its separator, as desired.
  6. Scroll down to move video A off the screen.
  7. Scroll back up to move video A onto the screen.
  8. Notice how video A now shows its separator. Why?

Using break points, I've found that video A gets removed when scrolled off screen, then reloaded with the cellForRowAt function when scrolled back on screen. But as far as I can tell, the isPreviousVideo property is properly getting set to true. So why isn't the separator being hidden?

@glennrfisher
Copy link
Contributor Author

The numbering issues and previous video issues may be related. The off-screen rows should be reloaded by cellForRowAt, which should properly set the number and isPreviousVideo properties. But instead, it almost seems like the cells are being set from a cache...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant