Store sorting information for smartplaylists and library nodes #1686

Merged
16 commits merged into from Nov 8, 2012

Projects

None yet

2 participants

Member

Cosmetically, this allows the sort by label for library nodes and smartplaylist to be specific to the playlist (i.e. if the playlist is sort by date added, then that's what we'll use) rather than displaying the ambiguous "sort by playlist".

It also allows some of the grouping changes that @Montellese has been doing to work a little nicer, as we can re-sort the list after grouping without problems.

Owner

Looks good to me and certainly would help one of the problems I'm having with grouping and sorting. Maybe we could add the string ID to the table in SortUtils instead of having to ask CSmartPlaylist about it? But I guess then the information would be duplicate.

Did I see that right that if the playlist has an order that isn't available to GUIViewStates (e.g. SortByVideoAspectRatio) that it will fall back to sorting by playlist? I guess in these cases there would still be a problem with sorting and grouping but the question remains if anyone actually uses those.

Member

Yes - if the sort mode doesn't have a corresponding SORT_METHOD_* then it'll drop back to sorting by playlist. This has to remain that way at least until SORT_METHOD_* is replaced with SortBy*.

I thought about popping the string in the LUT - it would save inclusion of the smartplaylist header but have the disadvantage of duplicate information being present. Will see if I have time tomorrow to do it up, as I think it's better that way (and it gives something useful for the LUT to give once SORT_METHOD_* is gone.

@Montellese Montellese commented on an outdated diff Oct 28, 2012
xbmc/GUIViewState.cpp
@@ -469,6 +477,27 @@ void CGUIViewState::SaveViewToDb(const CStdString &path, int windowID, CViewStat
}
}
+void CGUIViewState::AddPlaylistOrder(const CFileItemList &items, LABEL_MASKS label_masks)
+{
+ SORT_METHOD sortMethod = SORT_METHOD_PLAYLIST_ORDER;
+ int sortLabel = 559;
+ SortOrder sortOrder = SortOrderAscending;
+ if (items.HasProperty(PROPERTY_SORT_ORDER))
+ {
+ SortBy sortBy = CSmartPlaylistRule::TranslateOrder(items.GetProperty(PROPERTY_SORT_ORDER).asString().c_str());
Montellese
Montellese Oct 28, 2012 Owner

Any reason why you store the sort order as a string instead of just the value of the SortBy enum? You have to convert it to the string when storing it and back from a string when reading it.

Owner

As we got a way to retrieve the original sort method and order with this from the CFileItemList we could add some additional logic to CGUIMediaWindow which looks for those properties before sorting in case the sort method is set to SORT_METHOD_PLAYLIST and use the properties from the list instead of m_iPlaylistOrder for sorting. That would also cover the other sort methods that are not covered by SORT_METHOD_FOO. If the properties are not present we could still fall back to sorting by m_iPlaylistOrder.

Owner

I'm playing around with your code and noticed that if a smartplaylist has a sort method that is not available through SORT_METHOD_FOO the code in AddPlaylistOrder will fall back to SORT_METHOD_NONE instead of SORT_METHOD_PLAYLIST_ORDER so that will need an extra check. I'll see if I can add it in and upload a commit.

Owner

See the last three commits at Montellese@master...sort_those_nodes. The first one solves the case where we can't translate from SortByFoo to SORT_METHOD_FOO (and should fall back to SORT_METHOD_PLAYLIST_ORDER and not SORT_METHOD_NONE). The second and especially the third one would be more something for my grouping work and would allow to fall back to the sort method specified in the CFileItemList's properties instead of using SORT_METHOD_PLAYLIST and should solve all of the sorting-related problems in my grouping work.

Member

Do you need the SORT_METHOD_NONE case in GUIMediaWindow::SortItems given the other commit? I think we need to clearly comment in SortItems that the block is required only to support SortBy* that don't have an equivalent SORT_METHOD_*. The inverting of order isn't particularly obvious either (it is correct), so could do with a comment. I'll pull those commits in. Will also just use SortBy field directly as suggested I think to save the translation to and from string, and pop the localised string in the lut.

Owner

I first added the SORT_METHOD_NONE because it didn't work properly without it, then found the root cause but I guess it doesn't hurt to be there or does it? Sure I can add comments, just wanted to see if it works and get your opinion on the approach.

Member

It doesn't hurt, but doesn't really help either. I'm happy to do up the comments over lunch.

Member

Updated with SortUtils::GetSortLabel.

During the process, I went through and found a bunch of inconsistency, where we often used alternate (and sometimes incorrect) labels for sorting in the UI (i.e. the label shown after Sort By: in the GUI). They're all consistent after the last 8 commits, but I'm not sure if they're all required or desired. Changes are:

  1. HTSP seemed to use "TV Shows" when it sorted by library (one presumes, due to it feeding TV Shows in?)
  2. Sorting by Title and Label used Title and Label interchangeably.
  3. Sorting by Genre in music used "Label" in the UI (it's the only sort order available).
  4. Sorting by Artist in music used "Label" in the UI (it's the only sort order available).
  5. Sorting by Play count used "Times played" rather than "Play count". The latter is consistent with how it's displayed in the playlist editor.
  6. Sorting by duration or runtime used "Time" and sometimes "Duration" - now always uses Duration.

@cptspiff signoff is up to you. I'm happy to drop commits that you think are not required.

Owner

Most of those fixes you added with your latest commits are somewhere in one of my local branches as well so +1 for those. I just never got around to finish going through all the sort methods added in a specific view.

@Montellese Montellese commented on the diff Nov 2, 2012
xbmc/dialogs/GUIDialogSmartPlaylistEditor.cpp
@@ -296,7 +296,7 @@ void CGUIDialogSmartPlaylistEditor::UpdateButtons()
for (unsigned int i = 0; i < orders.size(); i++)
{
CGUIMessage msg(GUI_MSG_LABEL_ADD, GetID(), CONTROL_ORDER_FIELD, orders[i]);
- msg.SetLabel(CSmartPlaylistRule::GetLocalizedOrder(orders[i]));
+ msg.SetLabel(SortUtils::GetSortLabel(orders[i]));
Montellese
Montellese Nov 2, 2012 Owner

This causes all the SortByFoo methods which don't have a corresponding SORT_METHOD_FOO (and therefore aren't listed in the map in SortUtils) to appear as "None" in the smartplaylist editor dialog. Either add the missing SortByFoo to the map in SortUtils or keep CSmartPlaylistRule::GetLocalizedOrder() for now (as the map it uses is still present anyway).

jmarshallnz
jmarshallnz Nov 2, 2012 Member

fixed in the rebase

Owner

Previously everything included sortfileitem.h (which included SortUtils.h) for the SORT_METHOD_* enums, while actually used SortUtils.h for the sorting. SortFileItem.h will (eventually) be dropped completely - it is now included only here (for the enums).

I believe because we want it in the order specified.

Yup - it's there as otherwise we'd need to increase the SORT_METHOD_* with all the stuff SortBy* supports.

@ghost ghost merged commit 73c9e9f into xbmc:master Nov 8, 2012
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment