changed: Save (video) file state on shutdown #745

Merged
merged 1 commit into from May 1, 2012

Projects

None yet

3 participants

@arnova
Member
arnova commented Feb 29, 2012

Currently the (video) file state is not saved when one shuts down XBMC. This is unfortunate when you forgot something was eg. paused and you already turned off your TV. This fixes it. With this change we could also consider re-enabling timer shutdown for paused videos (?)

@jmarshallnz
Member

This is certainly wrong: You're calling a job after we've already asked the JobManager to close down.

If you want to do it, do it without the job

@arnova
Member
arnova commented Mar 3, 2012

@jmarshallnz: Thanks for the comment, I already had a feeling it was wrong. I've changed it, I assume it's ok now?

@jmarshallnz
Member

Don't use new as you've forgotten to delete it again, just use CSaveFileStateJob job(params); job.DoWork();

Next thing to check is what (if anything) the job uses that might be already torn down.

@arnova
Member
arnova commented Mar 4, 2012

Ah yes, silly c&p error. Fixed it in the last commit. When implementing this I already made sure (as far as possible) that there was eg. nothing that might already be torn down when shutting down.

@arnova
Member
arnova commented Mar 14, 2012

@jmarshallnz: What do you think about it?

@jmarshallnz
Member

I haven't yet gone through a thorough analysis. Either way, it's post-Eden stuff anyway, right?

@arnova
Member
arnova commented Mar 15, 2012

Yeah, post-Eden stuff. It's not THAT important ;-)

@jmarshallnz
Member

I think this should be OK (it's done reasonably early in the shutdown phase - in fact, I'd change it so it's the first thing in the shutdown phase), but would like if someone else could look over it.

@arnova: in the meantime, please squash the commits down so it's a little easier for others to review.

@arnova
Member
arnova commented Mar 31, 2012

Sorry, I completely screwed up the squash (never did it before). I'll try to fix it up.

@arnova
Member
arnova commented Mar 31, 2012

Squashed & fixed the commit. It's already almost in the beginning of CApp::Stop(). I'm afraid to put it before the CancelJobs() call as it may collide with each other (not sure what will happen if CFileStateJob() is called from 2 threads: CApp & CJobManager).

@jmarshallnz
Member

When is the player stopped?

@arnova
Member
arnova commented Apr 1, 2012

The player is stopped a few lines below in CApp::Stop(). After looking over the code one more I think we're fine. We would only miss the case we're we stop the player and immediately shutdown as that would abort the FileStateJob(). Although this scenario is rather unlikely, I guess..

@jmarshallnz
Member

I think this should be OK.

@vdrfan you've played with this stuff (db stuff) before - see any potential issues?

@mkortstiege
Member

Nope, no issues from what i can see but I'd move this to CApplication::StopPlaying() so the file state is saved on all possible shutdown methods (quit/suspend/hibernate). StopPlaying() is called from the PowerManager.

This would require a small re-factor in CApplication::Stop though. It's not yet using the method - just deleting the player.

@arnova
Member
arnova commented Apr 6, 2012

Good suggestion, I'll have a look

@arnova
Member
arnova commented Apr 6, 2012

Well, that's not that straightforward since StopPlaying() is also called in other places where saving the file-state would interfere with file-state-save job. I think it would be better to wrap the foreground filestate-saving into a function and call that from both the powermanager & CApp:stop() functions.

What do you think?

@mkortstiege
Member

Sounds like a plan ;)

@arnova
Member
arnova commented Apr 6, 2012

Something like this?

Unfortunately I somehow managed to screw GIT in a way this commit can no longer be automatically merged (probably something went wrong with rebase (I'm still "learning" git) :/

@arnova
Member
arnova commented Apr 13, 2012

I managed to properly rebase & squash the commits into on that can be merged into master.

Any objections against this going to master?

@jmarshallnz
Member

It's fine to go in during the May merge window IMO.

@mkortstiege
Member

+1 (in case it matters) ;)

@arnova arnova merged commit 6147e6a into xbmc:master May 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment