Cleanup ISO stack resume code #1099

Merged
merged 2 commits into from Jul 5, 2012

Projects

None yet

3 participants

@Voyager1
Member

this pull request addresses a number of things in the sphere of resume-play of ISO stacks.

  1. move the intelligence down from GUIWindowVideoBase to the VideoDatabase. All we need to know in the GUIWindowVideoBase is what to do with a Bookmark that contains a partnumber > 0.
    The VideoDatabase methods GetBookmarksForFile and GetResumePoint know how to deal with ISO stacks now, which makes the code more reusable for non-GUI callers.

  2. the resume "flag" as well as "percent complete" for an ISO stack (the little white triangle in Confluence lists) now correctly shows. This was not the case due to the VideoInfoTag resume bookmark not set (prior to the code changes described above in 1). The rule is that the bookmark of the "highest" part counts.

  3. additional code cleanup and refactoring - see comments below.

@theuni
Member
theuni commented Jun 22, 2012

Not my area of code to comment on, but kudos on splitting out the whitespace. That makes all the difference for review.

@Voyager1
Member

I've added more commits:

  • moving the GetResumeItemOffset code to a more logical place i.e. VideoDatabase. This way Application doesn't have to call GUIWindowVideoBase any longer
  • got rid of the crazy bit manipulation to code the part number as part of the startoffset.
@Voyager1
Member

the work is done. I've added one final commit to make sure the SaveFileStateJob properly updates the list. The reason is that for an ISO stack, the current item is actually the part being played, which in itself does not show up in the library list. Therefore, we need to notify the list to update that stack item. Otherwise, we have unexpected results with a bookmark of the "wrong part" being used (unless you reload the list by stepping out and coming back).

@jmarshallnz
Member

On the whole it looks nice. Please squash down the fixup commits and I'll give it another going over.

The only thing I don't particularly like is the non-videodb code in the videodatabase. Not sure if there's a "nice" way to deal with that though.

@Voyager1
Member

hi jmarshall - thanks for the code review. I've addressed the pointer comment and retested the code. Commits are squashed.

Regarding the videodatabase code, I agree, I've used Directory/FileItem classes to parse out the stack parts and also there's two new methods that take FileItem ptrs as arguments (the code copied down from GUIWindowVideoBase). The only way I see is to pass a VideoInfoTag instead of a FileItem ptr. But I would then have to move some checking code back to GUIWindowVideoBase (and wherever else this is called).

@Voyager1
Member

any other comments - if not, can we assign it to the July milestone?

@Voyager1
Member
Voyager1 commented Jul 4, 2012

@jmarshallnz - I've addressed all your comments (I believe).

  • The missing delete is addressed through a smart pointer, consistent with other code.
  • The vector is now cleared using clear()
  • The CApplication member (now protected) is no longer read by another thread, but passed by value to the savefilestatejob.

note: I rebased & left this for your review as a separate commit. When you're ok with final code, I'll squash before you pull.

@jmarshallnz
Member

Fixes look OK - rebase down and I'll give it another overview in case I've missed a simpler way to do it (I don't think so though - ISO stacks are really messy)

@Voyager1
Member
Voyager1 commented Jul 4, 2012

@jmarshallnz - done - rebased and squashed (except indentations for easy review)

@jmarshallnz
Member

Ok, the code looks OK, but I really think the two new functions to VideoDatabase don't really belong there - eg GetResumeItemOffset has a path that doesn't hit the db at all (and thus HasResumeItemOffset also has such a path).

IMO having those as statics in VideoBase seems just as reasonable - the only addition I think would be some videodb open/closing internal to GetResumeItemOffset, rather than being external?

@Voyager1
Member
Voyager1 commented Jul 5, 2012

@jmarshallnz - once again, done.

@jmarshallnz
Member

Forget to push?

@Voyager1
Member
Voyager1 commented Jul 5, 2012

no, it's all there. See commit 02eff8f

@jmarshallnz
Member

Sorry, I wasn't clear (thus didn't notice the difference) - the two statics should be in CGUIWindowVideoBase rather than CVideoDatabase.

@Voyager1
Member
Voyager1 commented Jul 5, 2012

ah. The whole point was move them out of CGUIWindowVideoBase because CApplication calls them. I don't like that, as it violates the layering.
EDIT:
let me know if you prefer CApplication calling CGUIWindowVideoBase - I can make that change but it wouldn't make me happy :-(

@Voyager1
Member
Voyager1 commented Jul 5, 2012

I could also move the code back to CGUIWindowVideoBase as you suggest and have CApplication call a stripped down version of it (essentially the part that gets the Bookmark from the VideoDatabase. That would probably be the best way overall.

@Voyager1
Member
Voyager1 commented Jul 5, 2012

@jmarshallnz - once again, I have implemented your comments, I believe it must be ready for pulling now ;-)

@jmarshallnz
Member

Looks good.

@jmarshallnz jmarshallnz merged commit 76f766f into xbmc:master Jul 5, 2012
@Voyager1 Voyager1 deleted the Voyager1:cleanup-resume-iso-stack-code branch May 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment