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

Mark as watched when Watch Now or Close is pressed #75

Open
dagwieers opened this issue Oct 30, 2019 · 29 comments
Open

Mark as watched when Watch Now or Close is pressed #75

dagwieers opened this issue Oct 30, 2019 · 29 comments
Assignees
Labels
enhancement New feature or request

Comments

@dagwieers
Copy link
Collaborator

dagwieers commented Oct 30, 2019

So I have the following problem that when an episode is almost finished and I press "Watch Now" to go to the next episode, the episode is not marked as watched in Kodi because it was not played until the end. (Or the cut-off time to consider it watched is not in sync with the Up Next interval).

I guess what is needed is a way to update this ignorepercentatend (in fact we would need it based on seconds, not on percentage) from the advancedsettings.xml in Kodi. This would require improvements to Kodi to either read and change this value, or make it part of the Kodi settings altogether.
https://kodi.wiki/view/HOW-TO:Modify_automatic_watch_and_resume_points

In the meantime we should document in the Wiki that the user should keep this in sync with the Up Next settings.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Oct 30, 2019

Another option is that (as the title suggests) Up Next would mark this episode as watched (by updating the resumetime to be the same as the totaltime). Unfortunately there is a bug in Kodi that prevents this. xbmc/xbmc#15897

Also see: add-ons/plugin.video.vrt.nu#520 (comment)

@dagwieers
Copy link
Collaborator Author

dagwieers commented Oct 31, 2019

I know why this happens for me so often. It seems for our add-on VRT NU we return a notification_time of 60 secs, which overrides the user-configured notification time of 30 secs.

This was confusing me before as well.

@mediaminister Why are we forcing a notification time of 60 seconds and not leave this up to the user? I think 60 secs is a long time for typical TV show episodes and it causes an episode to not be completely watched when pressed :-/ (And the kids apparently press Watch Now when Up Next comes up too ;-))

Update: Using the same margin for Up Next as we use for resumepoints fixes this in VRT NU. See: add-ons/plugin.video.vrt.nu#561

@dagwieers
Copy link
Collaborator Author

dagwieers commented Oct 31, 2019

Because Files.SetFileDetails() (see #77) does work for local media, we could fix this from within Up Next for local media, but add-ons need to implement this themselves when notified upnextprovider.

Update: Maybe we need to document this?

@dagwieers dagwieers changed the title Update resumetime when Watch Now pressed Mark as watched when Watch Now pressed Oct 31, 2019
@mediaminister
Copy link
Contributor

I forced 60 seconds because the credits of most fiction series have a duration that is much longer than 30 seconds. As I didn't succeed to detect the timecode of starting credits, it's indeed better to let the user decide which notification is used.

@dagwieers
Copy link
Collaborator Author

@mediaminister My PR has it now forced to 30 seconds, do you think we ought to make this configurable in the VRT NU add-on, or should we use the Up Next configured value ?

I can see the benefit of having this configurable for VRT NU specifically, as typically TV content does not have very long credits.

@mediaminister
Copy link
Contributor

No problem to make a config option in VRT NU add-on.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Oct 31, 2019

I have to reconsider my opinion here (again). It is better to show the dialog earlier, than too late. When I was watching the first Punk episode (1h30) the end credits start 50 seconds before the end. (So I had to wait 20 secs into the credits to have Up Next ask to go to the next) Which makes tagging an episode as Watched even more important. And I think Kodi needs to be redesigned in that aspect. (advancedsettings globally is not a good approach)

To be honest, I think a better approach is to make the dialog pop up at different intervals depending on the length of the video.

  • Videos less than X mins: autoplay (this already exists)
  • Videos less than 40 mins: 15 secs (typically short shows, talk show, kids shows)
  • Videos less than 1 hour: 30 secs (typically TV show episodes, short documentaries)
  • Videos longer than 1 hour: 60 secs (typically movies and longer documentaries)

I think this is something we may want to leave to Up Next though. A better default heuristic could be useful for all add-ons that don't know better.

@im85288 What do you think?

@im85288
Copy link
Owner

im85288 commented Oct 31, 2019

I like the approach but we would need to have this as an option in the settings for people who prefer this sort of “intelligent” pop up time.

The best integration I have encountered so far is with the Netflix addon which knows exactly when the credits start and sends that across to this addon to pop up the up next dialog. Unfortunately this is not available via any other means (as far as I am aware) which means having it at 30 seconds was a good compromise.

So yes it does seem a good approach but it should be an option the user turns on so they are aware of the assumptions made.

Remember this addon ignores all types other than episodes so it’s not often we will get anything more than 1 hour

@dagwieers
Copy link
Collaborator Author

@im85288 Right, movies should not have been listed. But you do have multi-episode documentaries that taken more than an hour each. The Punk documentary is one (and it seems to be a four 45 min episodes, which was re-cut to two 1:30 episodes on VRT NU 🙄).

Netflix is the odd duck though, it is part of what they sell, and maybe someday other VOD providers will have better metadata too :-)

My approach would be to have 3 sliders in the settings instead of 1, but rest assure if this information is provided, it overrides these default values.

Looking at the current settings, I think we could re-organize them better as well. I will make a proposal (and open a new issue for this).

@im85288
Copy link
Owner

im85288 commented Oct 31, 2019

Cool sounds good to me, yeah better metadata would be the ideal solution but I doubt that will happen any time soon!

@notoco
Copy link
Contributor

notoco commented Oct 31, 2019

I'm not a programmer, just a regular user (who sometimes throws an idea, finds a bug), but the above idea sounds great.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 28, 2019

This is what it currently looks like. Feedback appreciated!

screenshot022

@im85288
Copy link
Owner

im85288 commented Nov 28, 2019

@dagwieers Looks good to me, offers a high level of customisation. I think it may be better to have these options hidden behind a radio button. So for example by default we would have a radio button with the text 'Customise pop-up time (default 30 mins)' Once selected the options you show above would be shown.

@dagwieers

This comment has been minimized.

@im85288
Copy link
Owner

im85288 commented Nov 28, 2019

Excellent, that was exactly what I was thinking about.

Perhaps for the text: Duration in seconds for notification to be shown

@dagwieers

This comment has been minimized.

@dagwieers
Copy link
Collaborator Author

Perhaps for the text: Duration in seconds for notification to be shown

Sure I can do that.

@dagwieers
Copy link
Collaborator Author

I wasn't sure if we really needed 5 different options for the customization. Maybe we could do with only Short, Medium, Long, Very long. But since in this new scheme you cannot customize exactly how long Short is, I thought it would be better to have 2 options (< 10 mins and < 20 mins).

I am open to have a different offering altogether, but we can't easily allow to set both the length and duration for each option. I think that would become too confusing.

Now we have this:

screenshot027

screenshot028

So this would mean that:

  • By default (first user) it would use 30 seconds for all episodes or (existing users) use what was already configured
  • By default, the behaviour of what would happen for short episodes will stop working (port of customizations now)
  • Users need to explicitly specify Customize autoplay duration to configure the behaviour for short episodes (and every other length episode)

So this would be a general change of behaviour, unless we also add the existing configuration options for short videos in the default setting. It is a decision between "Old behaviour" or "Simplified interface".

@dagwieers
Copy link
Collaborator Author

The other question is whether the different sections are ok?

Originally it was:

  • Settings
  • Advanced
  • Developer

But it is unsure what really belonged to what section, so I chose to separate by function:

  • Interface
  • Behaviour
  • Expert → Everything that is not Interface, Behaviour or too specialized

I did not use separators because there are too few options in each of these.
screenshot029

screenshot030

@im85288
Copy link
Owner

im85288 commented Nov 28, 2019

I wasn't sure if we really needed 5 different options for the customization. Maybe we could do with only Short, Medium, Long, Very long. But since in this new scheme you cannot customize exactly how long Short is, I thought it would be better to have 2 options (< 10 mins and < 20 mins).

I am open to have a different offering altogether, but we can't easily allow to set both the length and duration for each option. I think that would become too confusing.

Now we have this:

screenshot027

screenshot028

So this would mean that:

  • By default (first user) it would use 30 seconds for all episodes or (existing users) use what was already configured
  • By default, the behaviour of what would happen for short episodes will stop working (port of customizations now)
  • Users need to explicitly specify Customize autoplay duration to configure the behaviour for short episodes (and every other length episode)

So this would be a general change of behaviour, unless we also add the existing configuration options for short videos in the default setting. It is a decision between "Old behaviour" or "Simplified interface".

It would be better somehow if we could keep exactly the same config out of the box as it was before the changes. To be honest I cannot recall how it handled short episodes except for recalling I added something that rejects showing the popup if the length of the episode was less that 5 minutes (or something along those lines).

But thinking about it a bit more, maybe this approach is better where anyone who wants specific times for short videos is forced to configure it themeseleves

@im85288
Copy link
Owner

im85288 commented Nov 28, 2019

The other question is whether the different sections are ok?

Originally it was:

  • Settings
  • Advanced
  • Developer

But it is unsure what really belonged to what section, so I chose to separate by function:

  • Interface
  • Behaviour
  • Expert → Everything that is not Interface, Behaviour or too specialized

I did not use separators because there are too few options in each of these.
screenshot029

screenshot030

That's fine for me, I called it Developer as the info there was only relevant for skin developers trying to integrate it. However Expert is more Kodi so is also fine with me

@dagwieers
Copy link
Collaborator Author

I can add the older settings back in the original form, but fear they may be more confusing than this new scheme. So I'll leave it up to others to decide.

Transitioning is hard. Do we want to make it easier for the existing users? Or do we want to make it easier for future users? And which group is the largest over time.

The more important question is, do we have good defaults?
(In my opinion the default of 30 seconds for all episodes is too low as a default)

@im85288
Copy link
Owner

im85288 commented Nov 28, 2019

For me the defaults are fine, 30 seconds seems to have been a sweet spot over the years. Let's see what others think on this.

I'm happy enough to go for the new settings, they are clear enough for existing/new users.

@dagwieers
Copy link
Collaborator Author

Let's continue the discussion in #100

@dagwieers dagwieers added the enhancement New feature or request label Dec 25, 2019
@dagwieers dagwieers self-assigned this Dec 26, 2019
@dagwieers dagwieers changed the title Mark as watched when Watch Now pressed Mark as watched when Watch Now or Close is pressed Jan 13, 2020
@im85288
Copy link
Owner

im85288 commented Mar 7, 2022

@dagwieers I am trying to release v1.1.6 and cannot work out how to auto build the release. Could you give me some pointers?

@michaelarnauts
Copy link

@im85288 I think you need to modify the release pipeline and use actions/setup-python@v2 instead of actions/setup-python@v1.

@michaelarnauts
Copy link

@im85288 I think you need to modify the release pipeline and use actions/setup-python@v2 instead of actions/setup-python@v1.

Sorry, I'm wrong. That's for the failing CI tests, not sure about the release.

@im85288
Copy link
Owner

im85288 commented Mar 7, 2022

@michaelarnauts thanks, not familiar with that but will have a look into it.

@im85288
Copy link
Owner

im85288 commented Mar 7, 2022

No worries, need to get the failing tests sorted too. If I cannot work it out for the release I'll just do a manual push to the kodi repo like I did in the old days.

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

No branches or pull requests

5 participants