Add View feature

[schellingb]

Description

Add saving of a filter set in the Explore menu into a so called "View" file which then gets listed alongside playlists. This also adds the ability to filter a category by range in the Explore menu and not just filter on exact matches.

The views are saved into .lvw (libretro view) files that just like playlist .lpl (libretro playlist) files are in JSON format and are stored in the same playlists directory.

Here's some examples of views that are now possible with the new range filtering:

• Games made between 1992 and 1996 by developers Capcom and Capcom Production Studio 1
• Games supporting 5 to 8 players
• Games containing "Mario" in the title released between 1988 and 1994

It's a fairly big change that touches ozone and xmb due to how the horizontal list (aka. sidebar in ozone) now shows two kinds of files (.lvw views and .lpl playlists). A small simplification in xmb and ozone removes the variable playlist_collection_offset by storing the view/playlist list offset in entry_idx and thus avoiding complex counting done separately both in xmb and ozone.

Because this PR touches a bunch of things it's probably better to test it before merging. A review would also be appreciated.

Related Issues

Related Pull Requests

Reviewers

Maybe @sonninnos ?

[gouchi]

Tested quickly so far so good.

git fetch origin pull/14467/head:pr14467
git checkout pr14467

Save view

View (xmb)

View (ozone)

Year 2k.lvw

{
	"filter_equal": {
		"releaseyear": "2000"
	}
}

[i30817]

Only positive filters like the UI right?

Followup i'd suggest would be moving these filters to their own dedicated zone in the tab.

[schellingb]

@gouchi Thanks for testing! Looks like nothing broke :-)

@i30817

Only positive filters like the UI right?

Yeah, I kept it simple mostly because making the UI for it is the most time consuming. If there's a demand to add features that can't be set in the UI but needs manual .lvw file editing I guess that could be done easier than adding more features to the UI.

Followup i'd suggest would be moving these filters to their own dedicated zone in the tab.

Similar reason, adding features to the UI is quite time consuming due to it often requiring touching each of the menu drivers separately. I would have loved to make them stand out better or have an icon. Maybe even allow customization of the icon.

[sonninnos]

Looking good, except a few tiny things:

• "By System Name" menu shows bogus thumbnails, when there should be none
• When browsing via a view into entry "Information", the playlist index + thumbnail goes kaboom, requiring extra browsing to recover
• (Ozone) Left sidebar size does not animate like in playlists, making the available space for items rather narrow with thumbnail sidebar visible

[Tatsuya79]

Seems to be working fine. (material ui, win10) Just that it shows the .lvw playlist in qt and that it's empty.

[LibretroAdmin]

Hmm, one issue i can foresee here - it seems XMB/Ozone/MaterialUI are now dependent on a function in menu_explore.c. However, for Griffin builds, this file only gets compiled in if HAVE_LIBRETRODB is defined. How will we deal with this? Can the necessary function possibly be divorced from menu_explore.c, or how should this be resolved @schellingb ?

[schellingb]

@sonninnos I just committed some fixes that should address all these things. If you could try it again I'd be grateful.

@Tatsuya79 I don't have a QT build environment to try this myself but it should be fairly simple to fix (just skip entries with .lvw extension when filling out the list of playlists. If possible could you try this patch on this branch and see if that works?

diff --git a/ui/drivers/qt/qt_playlist.cpp b/ui/drivers/qt/qt_playlist.cpp
--- ui/drivers/qt/qt_playlist.cpp
+++ ui/drivers/qt/qt_playlist.cpp
@@ -1212,8 +1212,11 @@
       QIcon icon;
       QString iconPath;
       QListWidgetItem   *item = NULL;
       const QString     &file = m_playlistFiles.at(i);
+      if (file.endsWith(".lvw", Qt::CaseInsensitive))
+         continue
+
       QString fileDisplayName = file;
       QString        fileName = file;
       bool            hasIcon = false;
 

@LibretroAdmin All relevant code sections are inside #ifdef HAVE_LIBRETRODB blocks and have been since these functions were added by #14365, so this shouldn't be a concern.

[i30817]

Denylisting is worse than allowlisting because the next time another specialized format happens you'll have to do it again.

Imo, this should go to another directory even, because you can't be sure that code you don't control isn't actually reading the playlists directory naively (my utilities that do it filter by file extension and don't have that problem, but i doubt i'm the only one messing around with those files outside the project).

I sort of guess why you didn't, too much code to change for a proof of concept, but i'd still suggest changing it 'later' before announcing the feature in a release.

[schellingb]

I did change it first to have its own directory but reverted that. The way RetroArch manages its directories means forcing an empty directory for every user out there that doesn't use this feature. If the directory gets deleted it gets recreated the next time RetroArch launches. If it were possible to only create the directory when first saving a view I'd have kept it. Views do belong to playlists though, as the views are filtering the playlists. Also the place where RetroArch shows playlists it now also shows views, it can enumerate just one directory of the file system and get all the things to be shown.

Maybe a subdirectory /playlists/views/ like the .lrtl files in /playlists/logs/ that only gets created as necessary and not at program start?

Edit: Now that I think about it, QT should behave the same as the main RetroArch UI does. So I'd actually call it a bug that QT lists files with a name that doesn't end with ".lpl". So maybe the patch should be if (!file.endsWith(".lpl", Qt::CaseInsensitive)) continue; instead?

[i30817]

Unfortunately, most apis (python included) make no hard distinction on getting a file or subdirectory (except of course, open() a directory fails, which is probably better by failing earlier) so i still think it should be completely separate. You can do whatever you like of course, since this is only in case of buggy external programs, so you're not chained to their possible faults.

I do agree that if you're going for the playlist dir, a subdirectory is neater, if you ever want to delete the views all at once or edit them manually as a user or something like that.

[sonninnos]

Heh, here comes the Ozone whack-a-mole:

1. When entering any Explore submenu the left side goes crazy:

2. When returning from the same submenu the left side recovers half way:

3. When entering entry from view the same invisible space is there:

https://user-images.githubusercontent.com/45124675/194444376-846d1edc-0105-458b-8a39-d7f2f8baadf7.mp4

XMB seems fine except the playlist icons are rather random after entering entries.

[schellingb]

@i30817 I would fully agree with all that if the /playlist/logs file and its .lrtl files weren't already a thing. But they exist so might as well add another subdirectory. As long as it doesn't create any directory or file for users who don't use the feature I'm fine honestly.

@sonninnos Man, that was very broken. Sorry about even committing that. I think I found a way to make Ozone cooperate without the need to add something silly like ozone->was_explore_list and I looked at all menus and they look good to me now. If you have some more time, can you give it another shot, please?

[sonninnos]

Tried my best to make it break, but was not able anymore. Still one more final thing seems to remain, which is the refreshing of downloaded thumbnails in views.

cb_task_pl_entry_thumbnail_refresh_menu() is where the current dodgy bit is which prevents the refresh callback from happening. I'll look into cleaning it up, but need a tiny break first.

How about simply like so:

diff --git a/tasks/task_pl_thumbnail_download.c b/tasks/task_pl_thumbnail_download.c
index 7a655d822d..96aa92ede8 100644
--- a/tasks/task_pl_thumbnail_download.c
+++ b/tasks/task_pl_thumbnail_download.c
@@ -578,21 +578,8 @@ static void cb_task_pl_entry_thumbnail_refresh_menu(
    else
 #endif
    {
-      char str[255];
-      menu_entries_get_label(str, sizeof(str));
-
-      /* Explore menu selection index does not match playlist index even in System list. */
-      if (string_is_equal(str, msg_hash_to_str(MENU_ENUM_LABEL_EXPLORE_TAB)))
-      {
-         if (!string_is_equal(pl_thumb->playlist_path,
-               playlist_get_conf_path(current_playlist)))
-         return;
-      }
-      else
-      if (((pl_thumb->list_index != menu_navigation_get_selection()) &&
-           (pl_thumb->list_index != menu->rpl_entry_selection_ptr)) ||
-            !string_is_equal(pl_thumb->playlist_path,
-               playlist_get_conf_path(current_playlist)))
+      if (!string_is_equal(pl_thumb->playlist_path,
+            playlist_get_conf_path(current_playlist)))
          return;
    }

[schellingb]

@sonninnos Thanks for the patch. I applied it and it makes thumbnails appear while browsing views so it seems to work as promised. I don't really know about the code modified there but I hope the removal of these two conditions doesn't somehow affect something else in a weird way. Was this maybe just some optimization to leave early before having to call string_is_equal?

-      if (((pl_thumb->list_index != menu_navigation_get_selection()) &&
-           (pl_thumb->list_index != menu->rpl_entry_selection_ptr)) ||

[sonninnos]

I wasn't able to get any noticeable benefit from allowing that early return to happen that way where it is supposed to be working (regular playlists), as in it won't catch any serious spam from late items when scrolling fast, so I guess those extra conditions aren't really important anymore..

And besides at that point some damage is already done, as in the image was downloaded, so not much is gained in aggressively preventing it from also displaying.

[LibretroAdmin]

OK, so I asked both @sonninnos and @schellingb and both agreed that this PR seems relatively safe now to merge. If there are any more outstanding issues that need addressing, let's make a new Github issue for it so it don't get lost.