Skip to content

Options to (not) group movies into sets in smartplaylists #1692

Closed
wants to merge 5 commits into from

2 participants

@Montellese
Team Kodi member

Currently the "Recently Added Movies" node in Video -> Library -> Recently Added Movies (which is a custom library node) behaves differently than the recently added movies node available through videodb://4/ and "Videos,RecentlyAddedMovies" (used by confluence for the "Recently Added" node in the "Movies" sub-menu). The list based on the custom library node, which is just a smartplaylist sorted by dateadded and limited to 25, groups the movies into sets which is not desirable IMO and therefore a bug. To remedy this we need to be able to specify that a list of movies retrieved through a smartplaylist should not group movies into sets independent of what a user has specified in the GUI settings. For that purpose I've added a new enum SmartPlaylistOption and a new member m_options to CSmartPlaylist. This option is currently not available in the GUI editor for smartplaylists but it can be specified through XMLs (which is all that is needed for custom library nodes). The main changes are in SmartPlaylist.h/.cpp and SmartPlaylistDirectory.cpp.

Most of the changes of this PR are a cleanup in the database code that I should have done before. During filtering we need to be able to retrieve those extra options from an XSP encoded in a URL which is possible through CDatabase::GetFilter() but we need to be able to pass this information back to the caller. Instead of adding yet another "reference out" parameter to retrieve that info I've replaced the existing reference parameters "filter" and "sortDescription" (which was added very recently) with a struct CDatabase::QueryData which contains all that information. That way we won't have to change the method definitions anymore next time we need to add new information that needs to be passed back to the caller.

If anyone has a better/easier way to achieve this I'm grateful for the feedback.

@jmarshallnz

weird indenting - mayaswell fix it up seeing as you're changing the lines here anyway.

Team Kodi member

Will do.

@jmarshallnz
Team Kodi member

The database changes look OK, save the minors.

For the playlist changes, I wonder if it's better to have a generic map like you've done in the options in the database. Reason is that at the moment it's specific to movies. Further, if it's a generic map you could read it directly from the XML just by iterating over the options nodes. Also, I think you can drop the need for ignore - no option suggests you take the UI option, a specified option says you do what it says.

I'd tend to make the options boolean as well unless there's a good reason to allow strings - easy enough to change later on if it's needed.

@Montellese
Team Kodi member

I don't think that we can drop the ignore option. Currently all smartplaylists group movies into sets when enabled in the GUI (which IMO makes sense and is to be expected). We can't just change the default behaviour to not group movies into sets unless the "groupbyset" option is defined for a smartplaylist, especially not as long as there's no GUI support for it. That's why I added both "groupbyset" and "ignoregroupbyset" which, when set, ignore the GUI setting. For 99.9% of the playlists everything stays the same i.e. grouping by set depends on the GUI setting because none of these options is set. Only with both of these options can we provide full control over the grouping behaviour on a per-smartplaylist level.
I don't really care whether we use an enum or a map. Might be easier to work with a map.

@jmarshallnz
Team Kodi member

Sure you can:

<groupbyset>false</groupbyset>  <!-- doesn't group no matter what -->
<groupbyset>true</groupbyset>  <!-- groups no matter what -->

No tag in the XML: Use the UI setting.

The same applies to the option map: If the option is present, use it. If not, use the UI setting.

I'm not sure I like the specificity of the tag. You really want something like

<group>set</group>

but then as you point out, you don't have the 3 states in the XML... Maybe

<group>none</group>
@Montellese
Team Kodi member

Ah well if we go the map route, then it is possible. With the enum-flag approach I chose it wasn't possible because either it's specified or not, there's no three-state there.

I considered using something like but IMO that's misleading. I have some work (similar to some work you did in the past) that allows to e.g. specify

<group>studio</group>

and the result list will be a list of studios and not a list of movies (or tvshow or whatever). So there's also the case of

<group>set</group>

which would result in a pure list of sets and not a mixed list of movies and sets. Therefore I wanted to avoid this. But any idea is welcome.

EDIT: Ok maybe we can use the approach nonetheless but add an attribute named "mixed" which defaults to false so

<group>set</group>

would result in a pure list of sets whereas

<group mixed="true">set</group>

would result in a mixed list of movies and sets. The only problem is that we don't have the grouping code/feature yet and sets are currently the only case where a mixed list makes sense (well maybe tags but we don't have that yet either).

@jmarshallnz
Team Kodi member

I think the last one is probably the best, assuming that specifying none has the grouping turns of the current behaviour. This would then be a specific grouping variable in the smartplaylist - don't think you need an options map?

@Montellese
Team Kodi member

OK I changed the logic to use

<group mixed="true">set</group>

to force grouping by set into a mixed list and

<group>none</group>

to force disable any grouping (which is what we are mainly after). This syntax should be future proof for when we support more grouping methods etc.

@jmarshallnz
Team Kodi member

I've reviewed, and once the commented minor changes are done (dropping no longer needed includes/typedef), this is good to go.

@cptspiff: your signoff please.

Montellese added some commits Oct 28, 2012
@Montellese Montellese CSmartPlaylist: add <group> support (currently only grouping by set)
Specifying no <group> data will result in normal retrieval of the list of
media items. Specifying <group>none</group> will result in not doing any
grouping, independent of any conflicting GUI settings. Specifying
<group mixed="true">set</group> will result in grouping movies in the result
list into sets where possible which will lead to a list containing mixed
items (movies and sets). <group>set</group> (and other values) is currently
not supported but is already introduced in anticipation of future features.
2672388
@Montellese Montellese CSmartPlaylistDirectory: support SmartPlaylistOption values for (igno…
…ring) grouping by set
6ceebd5
@Montellese Montellese database: add group to CDatabase::QueryData to support grouping a6c91c0
@Montellese Montellese don't group by sets in recentlyaddedmovies.xml 4147bb0
@Montellese
Team Kodi member

Thanks for the re-review. Fixed the minors.

@Montellese Montellese was assigned Nov 15, 2012
@Montellese
Team Kodi member

Closing this in favor of a more sophisticated version I'm working on.

@Montellese Montellese closed this Dec 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.