grouping by movie set outside of the database #1418

Merged
5 commits merged into from Nov 12, 2012

3 participants

@Montellese
Team Kodi member

This is more of an RFC than an actual PR as I'd like to get some opinions on the general idea and the implementation.

A while ago I had a discussion with @jmarshallnz about doing the grouping of movies into movie sets outside of the videodatabase. The advantage would be that the code GetMoviesByWhere would become a lot easier and GetMoviesNav/GetMoviesByWhere will return a list of movies and not a mixed list of movies and movie sets. The grouping into movie sets would then happen before the movies are displayed in the GUI. I had this idea when I was working on the advanced filtering. When applying a filter it can happen that a filter only matches one of the movies in a movie set so XBMC should only show that single movie and not the whole movie set. While that's not a problem to achieve the problem arises when it comes to loading the artwork for items etc which is done in the background and on the initial list. So the "new" item which was previously part of a movie set does not have any artwork and other details assigned yet.

So I have written a utility class called GroupUtils which is called from CGUIWindowVideoBase after a list of movies has been retrieved from the database and filtered. It currently only supports grouping by movie sets (based on CVideoInfoTag::m_setId).

So what are your opinions? Does the grouping outside of the database make sense at all?

@jmarshallnz
Team Kodi member
@Montellese
Team Kodi member

The current implementation uses the resolution and the language of the first audio stream to display the different versions of a movie so e.g. "1080p (English)" and "720p (English)" etc but obviously that won't work in all cases.

@jmarshallnz: Any opinion on the general idea of grouping outside of the db? I know we discussed this once before but maybe you've changed your opinion after seeing the implementation. I can remove the movie grouping because obviously that needs more thought and a better approach.

@jmarshallnz
Team Kodi member
@Montellese
Team Kodi member

I've removed all the commits related to grouping by the same movie so that only the code for grouping by movie sets outside of the database remains. Should make it easier to review.

@jmarshallnz jmarshallnz commented on an outdated diff Oct 2, 2012
xbmc/video/VideoDatabase.cpp
- GetSetsByWhere(setUrl.ToString(), setsFilter, setItems);
-
- CStdString movieSetsWhere;
- if (setItems.Size() > 0)
- {
- movieSetsWhere = "movieview.idMovie NOT IN (SELECT idMovie FROM movieview WHERE movieview.idSet IN (";
- for (int index = 0; index < setItems.Size(); index++)
- movieSetsWhere.AppendFormat("%s%d", index > 0 ? "," : "", setItems[index]->GetVideoInfoTag()->m_iDbId);
- movieSetsWhere += "))";
-
- extFilter.AppendWhere(movieSetsWhere);
- }
- }
-
- if (!CDatabase::BuildSQL(strSQLExtra, extFilter, strSQLExtra))
+ if (!CDatabase::BuildSQL(strSQLExtra, extFilter, strSQLExtra))
@jmarshallnz
Team Kodi member

A few whitespace issues (there's some extra spaces in front of sorting.sortAttributes above as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 2, 2012
xbmc/windows/GUIMediaWindow.cpp
@@ -863,6 +864,10 @@ void CGUIMediaWindow::OnFinalizeFileItems(CFileItemList &items)
CStdString filter(GetProperty("filter").asString());
GetFilteredItems(filter, items);
+
+ // Should these items be saved to the hdd
+ if (items.CacheToDiscAlways())
+ items.Save(GetID());
@jmarshallnz
Team Kodi member

Hmm, this saves the filtered list, not the unfiltered, or is GetFilteredItems() here supposed to be removed.

@Montellese
Team Kodi member

Hm I'll have to look into it. I'm getting confused in CGUIMediaWindow as I have multiple branches with changes in this area (advanced library filtering e.g.). It should filter the unfiltered list but maybe I messed this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz
Team Kodi member

Only thing I don't like is the separate queries per set. You could potentially get rid of that by joining the set table (there's only a single set per movie) though, so those can go away I think.

Nice work!

@Montellese
Team Kodi member

Yeah this code is from before we went the "one set per movie" road so once rebased and updated the extra queries to get the set's names aren't necessary anymore because in mainline that information is already stored in the movieview.

@Montellese
Team Kodi member

Based on the comments and looking at the current state of the code I think I messed up the commits when I pulled the "group by movie" functionality out because the original code didn't have any CGUIMediaWindow::FormatAndSort() anymore but had seperate calls Format() and Sort() and OnFinalizeItem() wasn't responsible for the filtering anymore but FilterAndGroup() was. Will have to check the original code and fix this up. Sorry for that.

@Montellese
Team Kodi member

Yup see Montellese@master...grouping_by_movie#diff-24 for the changes to CGUIMediaWindow in the initial form. I must have accidentally removed those changes when I removed part of the initial changes. The order should be

GetDirectory() -> ... -> FormatItems() -> OnFinalizeItems(cache unfiltered list and don't do any filtering) -> FilterAndGroup(first do the filtering then the grouping) -> SortItems()

@Voyager1
Team Kodi member

agree with both the accidentally removed changes and with the order. If you need someone to do a bit of testing before you merge, count me in!

@Montellese
Team Kodi member

OK I have fixed CGUIMediaWindow up as it was in the original PR, fixed the cosmetics and removed the extra db lookups for the set name as it's already available in the movie's CVideoInfoTag::m_strSet. So the code is now ready for a proper review (sorry again for the mess).

During my tests I noticed one thing that still needs fixing but I'm at a loss right now. When I enter the list of movies the movies are properly grouped into the existing sets but in list view sorted by name the "rating" value isn't displayed for the set. If I switch to e.g. sorting by year, the "year" value shows fine and when I go back to sorted by name the "rating" value is also displayed fine. This is independent of the sort method and property, it's just that when entering the list the first time the property is not displayed for sets. I've added some debug logging after the call to FilterAndGroup() and SortItems() in CGUIMediaWindow::Update() and the rating is stored in the set's CVideoInfoTag. @jmarshallnz any clue why this could be happening?

@Voyager1
Team Kodi member

Would it be related to the place where you call the Format() method on the group items? I had a similar (not sure) issue here: #1288

@Montellese
Team Kodi member

You're right, that's most likely it. FormatItems() is called before FilterAndGroup() (question is if it could be moved to be called after calling FilterAndGroup() which would actually allow to re-unite the calls to FormatItems() and SortItems() into FormatAndSort()) so it formats the original list of items and isn't applied to the CFileItemPtr containing the set. @jmarshallnz any particular reason why FormatItems() would have to be called before OnFinalizeItems (which essentially copies the item list to m_unfilteredItems and caches it to disc) and before FilterAndGroup()?

@Voyager1
Team Kodi member

I don't know whether there is any risk with Filtering needing the result (values) of formatting, or worse, you could end up not formatting the items that are actually inside groups. So what if you just call FormatItems once more as part of GroupItems(), only for those "group" items? Sounds simpler than it is, you'd probably need to do that in the GroupUtils...

@Montellese
Team Kodi member

Well filtering happens through the database (except for the simple title filter (which will soon be replaced) which operates on the label but that filter is always reset before calling CGUIMediaWindow::Update()) so I don't really see any issues there. Not formatting the items which are grouped together is not a problem either because they are not displayed so no need for them to be formatted in that view.
Sure I could just call FormatItems again in CGUIWindowVideoBase::GetGroupedItems() when GroupUtils told me that the list has changed but getting the view from the database and doing the formatting is a rather "expensive" process (especially on embedded devices) so if possible I'd like to avoid that. And doing it in GroupUtils is out of the question as a utils method shouldn't really need to call back into CGUIMediaWindow IMO.

@jmarshallnz
Team Kodi member

One presumes it's due to the title filtering applying to the label - can't filter on the label if it's not formatted up yet. If the filtering never looks at the label, then this order isn't needed.

@Montellese
Team Kodi member

The title filter is always reset in CGUIMediaWindow::GetDirectory() which is called at the beginning of CGUIMediaWindow::Update() so at the time we do the filtering in Update() there's actually nothing to filter because it was just reset a bit earlier. I'll see if moving it past the FilterAndGroup() will cause any obvious issues.

@Montellese
Team Kodi member

I have cleaned up the necessary changes to CGUIMediaWindow and it's a lot cleaner now. But there's still one issue with the thumb of the grouped item not showing up because the thumbloader operates on m_unfilteredItems. Changing this to m_vecItems works but only for unfiltered lists. Once a filter is applied the grouped item is a new CFileItemPtr instance and therefore can't use the details of the old one from the unfiltered list.

@Montellese
Team Kodi member

OK I found the right spot to call the videothumbloader (see f7e5761) in CGUIWindowVideoBase::GetGroupedItems() which is called after every filtering i.e. both after the filtering done during CGUIMediaWindow::Update(), after using the title filter in a library view and after changing the "Show/Hide watched" toggle. The call to the thumbloader in CGUIWindowVideoBase::Update() could probably be removed but I left it there just in case.

@Voyager1
Team Kodi member

@Montellese - did some testing with this branch and it looks good. I get the impression that when toggling hide watched status, some thumbs are reloaded each time even though the item is unwatched (and stays on the view). Any idea why?

@Montellese
Team Kodi member

All the thumbs for the sets are reloaded because everytime you change the list, the grouping is re-done which results in a new CFileItemPtr for any set. Any other thumbs shouldn't be reloading or rather I can't think of a reason why they should.

@Voyager1
Team Kodi member

update2 - I think I'm becoming crazy ^_^ - the few thumbs that briefly flashed were ALL sets (I didn't know it for some of them). So this is all good!

@Montellese
Team Kodi member

I updated this PR in case we want to go this route instead of #1631 to solve the problem of additional items appearing in the list after filtering.

Unfortunately there are (at least) 3 issues I'm aware of:

  • There's no way to turn of grouping by sets for smartplaylists which would e.g. be desirable for the recently added movies list. This is already a problem in the current code and is not caused by these changes. This would probably need some additional configurable value in CSmartPlaylist to solve.
  • Sorted smartplaylists are messed up after doing the grouping stuff because the playlist order is based on the original items and the sets created during the grouping don't fit into that pre-sorted list anymore. I can't see any easy fix for this caused by the current intermediary state our sorting code is in.
  • Because lists now change on every run of Update() (as long as there are sets and grouping by sets is enabled), the list of movies jumps on every list refresh (e.g. when marking a movie as watched). The selected item stays the same but it usually jumps to the top or bottom of the currently visible area even if it was somewhere in between before the refresh. I'm not 100% how this is handled.
@jmarshallnz
Team Kodi member

Is 3 already present anyway, perhaps only if the list is filtered? I'm not sure why the old technique wouldn't jump - filtering still would have taken place so that items could be removed (e.g. if hide watched was enabled). Is it worse now because the list always changes? The code that handles the selected item is in the list containers themselves, so something must be resetting the offset there (or it's doing something a bit silly).

Number 2 is tricky, agreed - you can't maintain the sorted order just by inserting items where the first movie was (sort by title will break) so the only way is to re-sort the list using the correct sort order, which you don't know atm from the playlist. The latter is related to the fact that "sort by playlist" is a duplicated sort mode in many library nodes, in addition to being non-descriptive.

@Montellese
Team Kodi member

Concerning number 3 I have to admit that I'm not 100% sure what is happening. In current master when I'm in a list (filtered or not) and I mark an item as watched, all that happens is that the watched overlay appears and the selection moves on to the next item. But with this code the whole visible part of the list moves and the selection moves to the next item which is then the topmost visible item (in a vertical list) and the item that I just marked as watched isn't part of the visible list anymore.

Number 2 is kind of a deal-breaker here because it results in really odd results :-/ The list is already resorted after grouping but as you said, the grouped items don't match into the sort by playlist mode.

@Montellese
Team Kodi member

See #1692 for a possible solution for number 1 and #1686 for a possible solution for number 2.

@Montellese
Team Kodi member

@Voyager1 could I get you to play around with https://github.com/Montellese/xbmc/tree/grouping_outside_db? It contains all the commits necessary to get grouping outside of the db to work properly and should also fix any issues with limited lists (without any hacks like in #1631).

@Voyager1
Team Kodi member

I have played with it before (#1418) and now again, still looking very good. I tried a those few filtering issues that broke before and it works fine now.

Does it make #1631 completely obsolete or just the hack I added?

@Montellese
Team Kodi member

It makes #1631 completely obsolete as we alwaye retrieve a pure movie list so no need for any hacks and grouping is alwas done after filtering.

@Voyager1
Team Kodi member

I hope it can still make the Frodo release :-)

@Montellese
Team Kodi member

@jmarshallnz and @cptspiff: rebased and updated (had to add logic to not group movies into sets when in Recently Added Movies node).

@jmarshallnz jmarshallnz commented on an outdated diff Nov 10, 2012
xbmc/windows/GUIMediaWindow.cpp
@@ -968,17 +974,22 @@ void CGUIMediaWindow::OnPrepareFileItems(CFileItemList &items)
}
+// \brief This function will be called by Update() before
+// any additional formating, filtering or sorting is applied.
@jmarshallnz
Team Kodi member

typo formatting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Montellese added some commits Jun 28, 2012
@Montellese Montellese add GroupUtils for generic grouping of items in a list (e.g. into mov…
…ie sets)
3ff48bf
@Montellese Montellese videodb: change retrieval of sets based on GroupUtils
GetSetsByWhere calls GetMoviesByWhere with custom JOIN clauses to only
retrieve movies being part of a set and the uses GroupUtils to group them
into sets. GetMoviesByWhere (and therefore GetMoviesNav) returns a list of
movies with no sets in it. To get sets GroupUtils::Group() has to be called
afterwards.
a88a8e7
@Montellese Montellese [win32] updated project files 142ddea
@Montellese Montellese media library: integrate grouping by sets fc44e6f
@Montellese Montellese CGUIWindowVideoBase: always exectue the videothumbloader after filter…
…ing and grouping
8c517ac
@Montellese
Team Kodi member

OK I messed up the commit/push yesterday but reflog saved my ass. I fixed the type @jmarshallnz mentioned and rebased onto latest master.

@jmarshallnz
Team Kodi member

Signed off. Over to you @cptspiff

@ghost ghost merged commit fc0d58b into xbmc:master Nov 12, 2012
@tru tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Nov 27, 2014
@tru tru Make sure that GetAllServers respects home flag as well.
This lead to managed users not getting playlists for example.

Fixes #1418
ef34ed3
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment