[database] - add setting for hiding watched movies/episodes in recently added lists

[Memphiz]

This is the c++ alternative to #5745

Not sure if i got the intention right. Not even sure why i joined that party tbh.

This basically adds 2 settings "Hide watched movies in recently added list." and "Hide watched episodes in recently added list." to the videos->Library section which does exactly that (and yes - this setting influences all recently added lists for movies/episodes - not only the ones on the home screen).

@MartijnKaijser let me know if this hits the feature request (i was kinda sick to read it completely and have the feeling that the bikeshedding didn't happen yet - so lets start it).

I* as its a feature... (this is btw not even compile tested yet ...)

[MartijnKaijser]

yep look like this nailed it what user requests intended.
here's the bikeshed part (well option open for discussion) ;)

• Either split or single option to hide them (you've now done split)
• Should they only be hidden on homescreen or in recently added view as well (atm it influences all)

[Memphiz]

You nailed the bikeshed points (+ the translation of course :D)

[da-anda]

yay, bikeshedding. As I prefer blue it would be nice to add a contextual comment to all new labels, not just the settings description. Thanks.

Let me add that if the setting "hide watched" in the sideblade of Confluence is enabled for the movie library, this already affects the "recently added" library node. With having this overruled by the new global setting users might get confused that the sideblade settings no longer has an effect (untested - but as I don't see this condition treated in your code changes I assume that's the case). IMO it might be sufficient to respect the library setting also on home screen widget.

[piejanssens]

Not ideal and restricts recently added movies view to unwatched only:
On the homescreen "the widgets" should be able to display the latest/unwatched items, but if you press down on 'movies' and choose 'recently added' you want to see everything or hide watched using the view filter "hide watched". IMO the library setting should not affect the home screen widgets, because when you setup recently unwatched items on the homescreen for fast access to the latest content, you'd still want to be able to view all your latest items in the library (and use the view menu to control watched/unwatched).

I see that GetRecentlyAddedMoviesNav is being called to set those home window properties for LatestMovie. GetRecentlyAddedMoviesNav is probably separately called for the movie list view
Thus, adding an argument/parameter based on the skin settings to GetRecentlyAddedMoviesNav solves the issue here. I guess the directory view is using this code? https://github.com/xbmc/xbmc/blob/2228e7f209fbf244bb0f9da970903016395043f2/xbmc/filesystem/VideoDatabaseDirectory/DirectoryNodeRecentlyAddedMovies.cpp

So the call for the window properties to GetRecentlyAddedMoviesNav can pass a parameter based on the system settings (e.g. "unwatchedOnly = true), whereas the call to GetRecentlyAddedMoviesNav for the recently added directory view can always pass "unwatchedOnly = false".

[Montellese]

Haven't looked at the code yet but since it only touches CVideoDatabase the concerns from @da-anda that new "hide watched" flags could influence the recently added node list is invalid for two reasons:

• The "Recently Added Foo" nodes in the video library don't call CVideoDatabase::GetRecentlyAddedFoo()
• The "Hide watched" flag in the library views is manually applied to the list of retrieved items and not to the database query.

[Memphiz]

@Montellese but CDirectoryNodeRecentlyAddedMovies::GetContent(CFileItemList& items) calls it (is this not the thing we talk about?)

[Memphiz]

This would also influence the list returned to json-rpc (which is bad imo) - so i go for a different approach which only influences the recentlyaddedjob

[Memphiz]

Updated. I tend to also merge the 2 settings - at least if no one comes up with a valid reason why someone would like to see the already atched recently added episodes but wants to hide those from the movie list...

[piejanssens]

Looks good. Don't forget about albums (same logic).
Merging the settings makes sense to me.

[Montellese]

@Memphiz: You're right. I thought we replaced the "Recently Added Foo" nodes with smartplaylist-based filter nodes but they still use videodb://recentlyaddedfoo/ so they end up going through CVideoDatabase::GetRecentlyAddedFoo(). IIRC we changed it once but had some issues so we changed it back (which is what I forgot).

[Montellese]

Code changes look good but the settings name/description doesn't make it obvious that we are only talking about the recently added list on the home screen.

[Memphiz]

The stupid thing is that enabling the recently added widgets is a skin setting but this one needs to be in core. Its not really intuitive from a user pov.

[Memphiz]

playcount is not available in the musicdatabase / albumsview ... @Montellese this would need what? (i guess version bump + migration path or so?)

[Memphiz]

updated:

1. merged the settings
2. added musicvideos
3. changed the string a bit.

as said albums is a pita @piejanssens wanna take this over? ^^

[Montellese]

Yeah I also dislike the fact that this setting only makes sense if the skin uses the builtin recently added functionality. If a skin doesn't use it or relies on some other way to get the items the setting is useless and confusing.

[MilhouseVH]

Build failure on OpenELEC/Pi:

RecentlyAddedJob.cpp: In static member function 'static bool CRecentlyAddedJob::UpdateVideo()':
RecentlyAddedJob.cpp:60:22: error: 'CSettings' has not been declared
   bool hideWatched = CSettings::Get().GetBool("videolibrary.hiderecentlywatchedvideos");
                      ^
In file included from /home/neil/projects/OpenELEC.tv/build.OpenELEC-RPi.arm-devel/kodi-14-88a9a44/xbmc/guilib/GUIControl.h:30:0,
                 from /home/neil/projects/OpenELEC.tv/build.OpenELEC-RPi.arm-devel/kodi-14-88a9a44/xbmc/guilib/GUIControlGroup.h:28,
                 from /home/neil/projects/OpenELEC.tv/build.OpenELEC-RPi.arm-devel/kodi-14-88a9a44/xbmc/guilib/GUIWindow.h:31,
                 from RecentlyAddedJob.cpp:26:
/home/neil/projects/OpenELEC.tv/build.OpenELEC-RPi.arm-devel/kodi-14-88a9a44/xbmc/guilib/GraphicContext.h: At global scope:
/home/neil/projects/OpenELEC.tv/build.OpenELEC-RPi.arm-devel/kodi-14-88a9a44/xbmc/guilib/GraphicContext.h:297:150: warning: 'g_graphicsContext' defined but not used [-Wunused-variable]
 XBMC_GLOBAL(CGraphicContext,g_graphicsContext);
                                                                                                                                                      ^
/home/neil/projects/OpenELEC.tv/build.OpenELEC-RPi.arm-devel/kodi-14-88a9a44/Makefile.include:93: recipe for target 'RecentlyAddedJob.o' failed
make[2]: *** [RecentlyAddedJob.o] Error 1
make[2]: *** Waiting for unfinished jobs....

Can I ask a really dumb question - assuming this only affects the widgets on the Home screen, why isn't this being handled by the skin.widgets addon itself?

[Montellese]

@MilhouseVH: Because we are talking about confluence which doesn't use the skin.widgets addon but uses the builtin RecentlyAdded skin labels.

[MartijnKaijser]

there are two options. do it proper in core like this PR or rip it completely out of core and put it in a "team" maintained addon (skin.widgets wouldn't be my first choice). Using an Add-on is way way slower due to python+json-rpc.

[MilhouseVH]

Oh wow, I thought it did! Told you it was a dumb question :)

[Memphiz]

compilation fixed

[MilhouseVH]

Ok, have tested this and it's working in reverse - the Homescreen is only showing watched items!

The "Recently Added" view is showing all items, so that's good.

[Montellese]

@MilhouseVH: Yup the WHERE condition is wrong and should be playcount <= 0.

Are we aiming to get this in for Helix? Because I'm uncomfortable with the additional setting as it might be confusing. Another approach that could get rid of the whole recently added in core (if it's fast enough) would be to use the <content> tag introduced by @jmarshallnz. That way it would be in complete control of the skin.

[MilhouseVH]

added musicvideos

It's a bit weird having a setting in Video Library also affect how recently added Music is shown - would it not be better/more logical to have a separate Music Library setting?

Edit: Oh hold on - this is musicvideos not Music? Hmmm.

[Montellese]

Music videos are part of the video library so that's correct.

[piejanssens]

I think we should move the settings over to the skin. We can add it to the default skins now and skinners will update their skin whenever they want to benefit from the increased performance. A skin without homescreen tiles doesn't have to use it.

Update: as it's currently implemented by Memphiz it looks solid: good description saying that it depends on the skin that is being used. Thus no room for surprises.

[piejanssens]

Oh I didn't know there was no play count on album level. I see that the widgets addon is also just returning all recently added. We could either apply the same logic using the param played/unplayed in GetRecentlyAddedAlbums and check that playcount of all songs belonging to that album must be 0 if unplayed param is used or change the core player and set/change a playCount on album level which is going to open another discussion (playCount +1 when first song is played/90% is played/all songs played).

BUT... Personally I don't care about unplayed recent albums on the homescreen because we use music in a different way. I don't want to watch a movie two times in a row or in the same week, but with music it's fine and so in that sense it might even be desired not to filter played albums from the homescreen...

[Memphiz]

Yep exactly. This makes no sense for music at all ...

[Memphiz]

@piejanssens i got an email with a comment from you but don't see it here strange

[piejanssens]

Yeah I removed it out of humongous shame. Couldn't see the effect after compiling, turned out I wasn't using the correct branch from your fork.. :)

Meanwhile compiled and tested -- WORKS GREAT

Thanks man.

[MilhouseVH]

Could this be rebased (I'd like to use it in an x86 build), perhaps even merged?

Popcornmix has a version of this PR in his RPi newclock4 branch which I've been including in my test builds for quite a while now, and this feature works very well.

[MilhouseVH]

Time to push the button on this one? It works great.

[MilhouseVH]

What's going on with this PR? Close it or use it?

[Memphiz]

Rebased - well this is as good as it gets from my seide. @Montellese & @MartijnKaijser for decision.

[Montellese]

The code changes look ok. But my concerns regarding adding a setting which only applies depending on how skins are written are still there.

[Memphiz]

mine too - but i don't see a way to make it generic and skin independend. Skins can always do stuff which is not ment to be like that though.

[Hitcher]

In my humble opinion this should be a change to Confluence only otherwise you'll end up with people reporting that it doesn't work when using other skins. As far as I'm aware there aren't any other Kodi settings that are skin specific like this. I also thought there was discussion some time ago about doing away with the home window info labels.
The easiest way to do this in Confluence would be to switch to using smart playlists to fill these fields and add them to the skin settings.

[ronie]

very little skins still use the built-in 'recently added job' to present recently added items on their homescreen. most have switched to an addon or other methods to provide that info.

it's not worth to add a setting for it imo.

[Hitcher]

@ronie Are you replying to me or the thread?

[Memphiz]

+1 for handling it somewhere else tbh. The builtin is more used for remote controls i guess who also could do this kind of filtering on their own.

[ronie]

@ronie Are you replying to me or the thread?

just my thoughts in general.
PR can be closed as far as i'm concerned.

[mikatrash]

The PR is closed but if the appropriate place to implement this feature is in Confluence, was there a feature request created to have a chance to have this feature available in the future ?

[razzeee]

@ronie has this changed now? does estuary use the job and does this make sense again all of a sudden?

[ronie]

3 x no ;-)

[foss-]

Pardon me for not reading comment backlog of 2 years, but why is this not a feature that get's implemented in estuary?

[NedScott]

That's a question best asked on the Kodi forums because it will, no doubt, result in too many replies on github.