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

Add contextmenu option to remove resume points #12708

Merged
merged 2 commits into from Sep 6, 2017

Conversation

razzeee
Copy link
Member

@razzeee razzeee commented Aug 22, 2017

Description

Adds a new contextmenu item for videos which enables users to remove a resume point.
Not 100% sure about the wording, so if anybody has an better idea, let me know.

Motivation and Context

I basically got a forum PM asking for this inside script.trakt. As you can't remove a resume point without loosing all your watched data. Before this PR you needed to "Mark as unwatched", which gets rid of all view counts and the view date.

How Has This Been Tested?

Locally on windows 64

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed

@razzeee
Copy link
Member Author

razzeee commented Aug 22, 2017

Some things that might be weird:

  • This won't remove items from the homescreen listing for movies in process unfortunatly
  • If you have the item multiple times in your view, it will only visually update the selected one. For e.g. one movie shows in both in process movies and recently added on your homescreen. If you remove the resume point it will only update the resume point visualisation of the selected one.

@ksooo
Copy link
Member

ksooo commented Aug 23, 2017

What about fixing the stuff not working before merging features that only work half way?

@Montellese
Copy link
Member

I always click on the video I want to remove the resume point from, choose "Start from beginning" and then immediately stop the video again. That way I can reset the resume point and keep the watched status. But TBH the only time I have to do that is after testing something which requires me to play a video.

@razzeee
Copy link
Member Author

razzeee commented Aug 23, 2017

@ksooo
Not 100% sure what your refering to. If you mean the other merged PR, that's still on my radar, but I figured yesterday that that's probably not a 30min task. Unlike this. Time was rare the last months.

@Montellese
Correct, that works.
But
a) Emits all Video started and Video stopped events
b) Might not be very self explaining for non-developers?

And yes, I would probably only do that for tests too. The user that contacted me said this "An example would be, you just want to watch that one scene again."

To be clear, no hard feelings if we don't want this in core.

@ksooo
Copy link
Member

ksooo commented Aug 23, 2017

Not 100% sure what your refering to. If you mean the other merged PR, that's still on my radar, but ...

No, I was referring to this PR here. I wanted to suggest not to merge this PR if we already know about "Some things that might be weird" after merging, but to fix the "weird" things first.

@razzeee razzeee added the WIP PR that is still being worked on label Aug 23, 2017
@razzeee
Copy link
Member Author

razzeee commented Aug 23, 2017

Yes, I will have another iteration on this. Wasn't expecting it to get merged after 15mins anyway. Wanted to see if anyone will shoot it down first or if I missed something that was already there.

@ksooo
Copy link
Member

ksooo commented Aug 23, 2017

Cool, thanks.

@razzeee
Copy link
Member Author

razzeee commented Sep 1, 2017

Still struggling to find an event to refresh the item in all our frontends. It even seems like home screen has that problem in more places, it's just not as obvious. So for e.g. watching something and aborting in the middle should imidiatly add it to the home screens "in progress movies".

@tamland
Copy link
Member

tamland commented Sep 2, 2017

@razzeee You need to use announcer (see how the other methods in CVideoDatabase are done). Then the directory provides will pick up on it. I would just change DeleteResumeBookMark to take a CFileItem and do it there.

@razzeee
Copy link
Member Author

razzeee commented Sep 2, 2017

I tried using the annoncer, but I probably haven't had the right event then. Will have a look at DeleteResumeBookMark

@razzeee
Copy link
Member Author

razzeee commented Sep 2, 2017

Thanks @tamland

Works like a charm now. Adresses both mentioned problems.

I've tested this with movies, episodes and files outside of the library. All work fine as far as I can tell.

@razzeee
Copy link
Member Author

razzeee commented Sep 3, 2017

jenkins build this please

@razzeee
Copy link
Member Author

razzeee commented Sep 3, 2017

So jenkins likes this. I would like to get reviews and/or info if we want this :)

@ksooo
Copy link
Member

ksooo commented Sep 4, 2017

Please do not merge. This needs some more love to get this working for PVR recordings.

@razzeee
Copy link
Member Author

razzeee commented Sep 4, 2017

@ksooo Thanks for checking

@ksooo
Copy link
Member

ksooo commented Sep 4, 2017

I will try to do the rework needed to get pvr recordings working as well. You can cherry pick this, then.

@ksooo
Copy link
Member

ksooo commented Sep 4, 2017

@razzeee feel free to cherry-pick: ksooo@ebdd1c4

Works well with normal videos and pvr recordings now.

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

@Razzee code is ready to get merged, imo.

Functionwise, I think this is a useful feature. I like it.

@ksooo ksooo added Type: Improvement non-breaking change which improves existing functionality Component: PVR v18 Leia Component: Video labels Sep 6, 2017
@ksooo ksooo added this to the L 18.0-alpha1 milestone Sep 6, 2017
@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

jenkins build this please

@razzeee
Copy link
Member Author

razzeee commented Sep 6, 2017

👍

I wanted to runtime test your changes again (build forever yesterday night and I needed to go to bed). But you probably did that and your implementation is way better anyway :)

@MilhouseVH
Copy link
Contributor

@Razzee:
This PR seems to have changed since yesterday, with the addition of extra files, one of which can't be found on Linux:

xbmc/video/jobs/VideoLibraryResetResumepointJob.cpp
needs to be renamed:
xbmc/video/jobs/VideoLibraryResetResumePointJob.cpp (Linux is case-sensitive)

Currently it doesn't build.

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

xbmc/video/jobs/VideoLibraryResetResumepointJob.cpp needs to be renamed: xbmc/video/jobs/VideoLibraryResetResumePointJob.cpp

Ooops. My fault. Just a typo. I introduced the new files and @razzeee cherryicked my commit.

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

@razzeee shall I fix my commit and you cherrypick again or do you want to fix it directly on your PR?

@razzeee
Copy link
Member Author

razzeee commented Sep 6, 2017

Should be fixed. I also just tested again and it works like a charm.

Will merge this if jenkins is happy :)

@MilhouseVH
Copy link
Contributor

Doesn't look fixed to me...
s1

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

@MilhouseVH try again, I just fixed my commit.

@MilhouseVH
Copy link
Contributor

Nothing changed here - are you force pushing?

@MilhouseVH
Copy link
Contributor

Yes, it needs a capital P, as it is here:
https://github.com/xbmc/xbmc/pull/12708/files#diff-6090027e717ee17261b4d8f2e929008fR7

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

@MilhouseVH I know what the problem is and I fixed it, but @razzeee needs to cherrypick again!!!

@MilhouseVH
Copy link
Contributor

Ah ok sorry! :)

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

ksooo@436d323

@MilhouseVH
Copy link
Contributor

All good now.

@ksooo
Copy link
Member

ksooo commented Sep 6, 2017

Okay, let's bring this to an end. ;-)

@ksooo ksooo merged commit e802494 into xbmc:master Sep 6, 2017
@ksooo ksooo removed the WIP PR that is still being worked on label Sep 6, 2017
@razzeee razzeee deleted the contextmenu-remove-resume branch September 6, 2017 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Component: Video Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants