media library: only allow "new" items after filtering into a list if they were part of a previously listed movie set #1631

Closed
wants to merge 1 commit into
from

3 participants

@Montellese
Team Kodi member

Up until now when the user is in a list and starts a filter and that filter happens to return items that were not part of the original list, we just appended those items to the list. The only use case where this can actually happen is if the unfiltered list contains movie sets and the applied filter reduces that set down to a single movie from within that set. That movie is a valid item for the active filter but not part of the unfiltered list.
Blindly adding all the additional items to the end of the list results in odd behaviour when e.g. in "Recently Added Movies" where we only list 25 movies. Applying a filter will result in filtering through all the items (not just the available 25) and appending all the new ones to the list.

With this change we check every new/unknown item against all the items in the unfiltered list and see if it was part of a previously listed set. To be future-proof I've introduced a new virtual method in CGUIMediaWindow which can be overriden by whatever class inherits CGUIMediaWindow. For the use case of movies in a set CGUIWindowVideoBase does this.

@jmarshallnz jmarshallnz was assigned Oct 16, 2012
@jmarshallnz
Team Kodi member

IMO there's 3 ways to deal with this (it's only a problem with limited lists, right?)

  1. Do all filtering completely outside the database. Obviously not an option for Frodo, and a lot of code for not much gain.

  2. Do filtering completely inside the database, and utilise the current filter (from the smart playlist) to restrict the items. i.e. it would be select * from movieview where and idMovie in (select idMovie from ). I suspect this may also be a fair chunk of code, but you'd be able to assess that better than me.

  3. Do what you've done above and hope you hit all the cases.

@Montellese
Team Kodi member

Yeah I considered all those options as well.

  1. Could become more powerful and in addition it could also be used for plugins etc but it would be a lot of work and probably a lot slower as well.
  2. I actually wanted to go that route first but then realized that it would still need special casing for movie sets as they wouldn't fit into the "idMovie in (select idMovie from ...)" condition.

So the only difference between 2 and 3 is that the special casing is handled at a different location but in both approaches the special casing has to be done.

@jmarshallnz
Team Kodi member

The second query is just the filter + limit from the first, which shouldn't have any information about sets?

@Montellese
Team Kodi member

"second query"? If we want to limit the result of the filtering based on the unfiltered list, we need to put together a SQL where clause containing all of the IDs of the items in the unfiltered list. That works fine for albums, artists, songs, tvshows, episodes and musicvideos but doesn't work for movies because there we have a mixed list of movies and movie sets so it will require special handling to put the SQL where clause together.

@jmarshallnz
Team Kodi member

The unfiltered list is either completely unfiltered (a library node) or is filtered via a smartplaylist (recently added et. al.)

Either is defined by a query - you don't need to construct a list of existing id's in the unfiltered list, as you can grab that list again from the database.

@Montellese
Team Kodi member

@jmarshallnz You ok with this going in with the additional comment I added in the last rebase?

@jmarshallnz
Team Kodi member

Yeah. I was considering how much work it'd be to just get the grouping in - seems not all that much, but obviously that depends on how @cptspiff thinks about it - hate putting temporary stuff in when the real solution is available.

https://github.com/Montellese/xbmc/tree/refs/heads/grouping

@Montellese
Team Kodi member

Something went wrong with that link. It's #1418 but it needs updating because I changed something today in the functionality to retrieve sets.

In general I agree that solving the problem with a real solution is preferrable over some temporary solution but I don't know about the impact of that PR. I thought I had the advanced filtering well integrated and tested but there are soo many use cases to remember and cover when it comes to the media library and all it's functionality that it's impossible to be sure that a change of this scale will not have any additional unexpected side effects.

@jmarshallnz
Team Kodi member

Well, that one is primarily moving the existing grouping code from inside the db to outside. IMO it solves so many problems that it's worth it.

@Montellese
Team Kodi member

I leave the decision up to you guys. I can rebase that commit and adjust it after I merged #1646.

@Voyager1
Team Kodi member

I've been testing this too, and have a bit of concerns with the recently added view, when using in combination with "group by set" and filtering.

First we have limit 25 but when you filter (and subsequently clear it) any movie that was part of those 25 but also a member of a set with more than 1 element, is now gone.

@Voyager1
Team Kodi member

I think I understand - there was one item that disappears, we go from 25 to 24. That one item is part of a set with 2 movies, but only 1 of them was part of the top 25 recently added, and as such not grouped as set in the recently added view. So the logic considers that this "set" was not in the recently added view, therefore all items not previously part of a set are discarded. Q.E.D. :-)

I hope this helps to find a solution...

@Voyager1 Voyager1 commented on the diff Oct 23, 2012
xbmc/video/windows/GUIWindowVideoBase.cpp
@@ -1868,6 +1868,27 @@ bool CGUIWindowVideoBase::CheckFilterAdvanced(CFileItemList &items)
return false;
}
+bool CGUIWindowVideoBase::CheckFilteredItem(const CFileItemPtr &item, const CFileItemList &items)
+{
+ // the only additional items that are valid in a filtered list are items
+ // that were previously part of a movie set
+ if (!items.IsVideoDb() || !item->HasVideoInfoTag() ||
+ item->GetVideoInfoTag()->m_type != "movie" || item->GetVideoInfoTag()->m_iSetId <= 0)
+ return false;
+
+ int setId = item->GetVideoInfoTag()->m_iSetId;
+
+ for (int i = 0; i < items.Size(); i++)
+ {
+ const CFileItemPtr &origItem = items[i];
+ if (origItem->HasVideoInfoTag() && origItem->GetVideoInfoTag()->m_type == "set" &&
+ origItem->GetVideoInfoTag()->m_iDbId == setId)
@Voyager1
Team Kodi member
Voyager1 added a line comment Oct 23, 2012

I guess the solution to the issue I discovered is to also compare the original item id with the item being checked's id, not just with its set-id.

@Voyager1
Team Kodi member
Voyager1 added a line comment Oct 23, 2012

like ..== setId) || origItem->GetVideoInfoTag()->m_iDbId == item->GetVideoInfoTag()->m_iDbId)

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

Did you try this in combination with my other PR? Because this PR is still based on current mainline and suffers from all its bugs/regressions.

@Voyager1
Team Kodi member

Yes, I tried it in combination with #1646 - had to manually resolve conflicts (relatively easy).

Did some tests/debugging this morning, and what I mentioned in the commit is actually not the complete story.
The actual element that disappears from the recently added (a single element from a set), seems to be already removed from resultItems prior to hitting the CheckFilteredItem loop. This makes that the additional condition I suggested is never hit.

The cause of that is IMO in CGUIMediaWindow::GetAdvanceFilteredItems in the part where you construct the resultitems list (either the smartplaylist excludes it, or the piece where you iterate over the original items and check whether they are still part of the filter).

The condition I suggested still makes sense, however we must find a way to not exclude single-items from a multi-item set (which happens with limited lists like recently added).

@Montellese
Team Kodi member

Yes the problem is that we do the set grouping and retrieval before the sorting and limiting, so if there's a set with 2 movies. This wouldn't be an issue if we'd go the "grouping outside of the database" approach.

@Voyager1
Team Kodi member

sorry if you received duplicate or strange messages here, github is not acting very well with MS IE, and messages disappeared without me interfering... this time I try using Firefox.

So, I have been hacking around trying to fix the issue, and I was able to produce a working fix.

in CGUIMediaWindow::GetAdvanceFilteredItems, where you check if items from the original list are still part of the filter, and use the lookup map:

    map<CStdString, CFileItemPtr>::iterator itItem = lookup.find(path);
    if (itItem != lookup.end())
    {
      // if the item is a folder we need to copy the path of
      // the filtered item to be able to keep the applied filters
      if (item->m_bIsFolder)
        item->SetPath(itItem->second->GetPath());

      // add the item to the list of filtered items
      filteredItems.Add(item);

      // remove the item from the lists
      resultItems.Remove(itItem->second.get());
      lookup.erase(itItem);
    }
    else
    {
       // here need to deal with the case when the item is part of a set (item->setID > 0)
       // check if "videodb://1/7/setid/" is part of lookup map
       // if so, filteredItems.Add(item);
    }

the problem at hand is that the item that disappears is NOT in the lookup list (the filter), because the list has the SET instead. So the "hackish" fix I made was to add an else block here that would get the set ID, construct the path videodb://1/7/setid/ and look that up in the lookup map instead. If found then "item" would also be added to filteredItems.

Now this is a hack because we are in CGUIMediaWindow and we don't want dependency on Video stuff like VideoInfoTags or sets... but this is just for illustration.

If you like the idea I can further develop it into a clean proposal, using virtual calls down to CGUIWindowVideoBase.

@Voyager1
Team Kodi member

update: here's a commit in a personal branch : Voyager1@5a50dd5

@Montellese
Team Kodi member

Updated this as well. Haven't incorporated @Voyager1's code yet because I don't like the IsPartOfSet() etc code in CGUIMediaWindow at all plus I'm not even sure I understand the problem. Will have to look into it a bit more if I have some time on Sunday.

I also update #1418 (which would be an alternative solution to this approach) but I stated a few problems with that approach in my latest comment on that PR.

@Voyager1
Team Kodi member

@Montellese - it's confusing, I know.
The problem is that in recently added items, the list is not grouped by sets, even though group by sets may be turned on. The other problem is that you have a limited list of 25, so even if it were grouped, you have the possibility to have 1 of these 25 being part of a set that contains more than 1 in the full movie list.
Now comes the filter, which loads the full list, grouped by sets - so sets are represented by the set itself, not the items. This causes set elements from the original list to be removed, as these elements are not in the filter list!

The commit I added does just that. If the item from the original list is not in the filter, it checks whether it's part of a set (set id >0) and whether that set is in the filter.

@Montellese
Team Kodi member

Which recentlyadded are we talking about? The one directly accessible from the home screen or the one in Videos -> Library -> Recently Added Movies? I can see this being a problem for the one being accessible from home but it shouldn't happen on the other one.

@Voyager1
Team Kodi member

I'm not talking about the 5 or 10 items that display in the skin on the home screen. I'm talking about the one you access in the library under Movies > Recently Added Movies

@Montellese
Team Kodi member

Yeah that's what I meant with the one directly accessible from home screen. Because that one currently differs from the one available at Videos -> Library -> Recently Added Movies. We have to decide for one or the other and stick with the decision for now. Having two different entry points for the same list is not really ideal.

Can you try your problem on the one in the library? You probably won't encounter it there because that one can have movie sets etc (which IMO shouldn't be the case though).

@Voyager1
Team Kodi member

I tried it in this node (Videos -> Library -> Recently Added Movies) and it works correctly. I also tried in the same node with your new PR to disable grouping in recently added. Works correctly too.

The problem in the other node (Movies > Recently Added Movies) is still there in both cases. I second your proposal to make these two nodes the same.

@Montellese
Team Kodi member

I just tried to get this integrated and noticed that there's a problem not covered by your implementation (but it's probably not easily possible anyway): Let's say you have a set "The Terminator Collection" with all four Terminator movies. All of them show up in the recently added movies node seperately (i.e. not in the set). Now you apply a filter with "director == James Cameron" which will match the first two of the four Terminator movies and the filter will return them as a set. So all four Terminator movies from the original list will not directly match an item in the filtered list so it falls back into your additional logic and checks if their set exists. This will be true for all four Terminator movies even though the filter only matches two of them so the resulting list will be wrong.

@Voyager1
Team Kodi member

your assessment is absolutely correct, I had thought about that too, but as you say, it's difficult to get around this unless you ungroup everything for filtering (on both the source list and the "filter list") and then regroup for presenting the result. Without, it's probably the closest you're going to get.

@Montellese Montellese closed this Nov 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment