custom library nodes: allow optional <path> tag in filter nodes #1602

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@Montellese
Owner

@jmarshallnz this is more of a work around than a proper fix.

The problem is that since we switched the recentlyadded XML nodes to a filter and therefore doing the retrieval through CSmartPlaylistDirectory and not through CVideoDatabaseDirectory as before we get a different path for the list of items (e.g. videodb://2/2/-1/-1/ instead of videodb://5/ for recently added episodes) which results in the media window logic picking the "wrong" GUI view state. The recentlyadded-specific GUI view state doesn't allow any extra sorting whereas the media-specific GUI view states allow sorting and don't contain the Sort By Playlist functionality which results in the list of recently added items being re-sorted (By Name by default) after retrieval and not being in the proper order anymore.

With this fix we retrieve the tag also for filter nodes (if it's set) and assign it to the list of items. This will still result in going through CSmartPlaylistDirectory instead of CVideoDatabaseDirectory but the media windows logic will be able to determine the proper GUI view state based on the path of the items.

Let me know if anyone has another idea/approach to solve this problem.

@jmarshallnz jmarshallnz was assigned Oct 11, 2012
Member

I guess the ideal would be for it to go through the smartplaylist logic in the view state stuff - that way the sort logic is still available, and sort by playlist is the default.

Alternatively, if we want sorted nodes to never be able to be sorted any other way, we could add the sort order to the CFileItemList on directory fetch perhaps?

Owner

I'm not sure what I'd expect. IMO for recentlyadded it makes sense to not being able to sort it by anything else because then you don't have the items in the recently added order anymore. But for smartplaylists in general it could make sense to specify a sort method but then wanting to change it dynamically in the GUI when actually using it.

Member

Yeah, I'm not sure either - the order could be specified just so you get the top 100 rated movies for example, yet you'd still want that sorted by name at a later point.

IMO what we need is a way to ensure that the sort by playlist is available and made the default for all filters. This could possibly be done using a property on the fileitemlist if we don't have information about the path (the path would normally contain a .xml otherwise).

Owner

Yeah that approach would work. The only downside will be that "Sort by Playlist" is not a very good indicator of what it is actually sorted by in case of recently added.

Member

Yeah - what we really need is a way to specify the default sort method (which could be taken from the filter) and allow the other sort methods to be added to it if not already available.

Owner

Montellese@master...library_node_sorting is a very basic approach to setting the default sort method to SORT_METHOD_PLAYLIST_ORDER if a smartplaylist has a special sort method specified. It works fine for recently added but the problem with this approach is that it will completely disable the dynamic sorting functionality for smartplaylists. I first wanted to pass the actual sort method to dynamically add it to the list of available sort methods in a view and then setting it as the default sort method but that currently doesn't work because smartplaylists use the "new" sorting implementation with SortByFoo whereas CGUIViewState still uses the old implementation with SORT_METHOD_FOO. I have a commit (Montellese@master...sorting_cleanup) to get rid of SORT_METHOD_FOO everywhere except in the python API for backwards compatibility but it touches a lot of code and needs rebasing which is why I never made a PR for it. But obviously it needs to be done at some point.

Member

I think it can be fixed by just removing the url.GetProtcol() == "library" || from CGUIViewState::GetViewState().

The problem is that we've lost the fact that we had a library:// URL as it's been set to a videodb:// URL, presumably for filtering purposes.

Owner

Closing as solved differently in 7125e47.

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