Merge of manual and autoscan

[zoltanvb]

Note: Removed the draft flag, from my side it can go forward.

Scanning options are unified for a single operation, which can be customized to cover previous autoscan, manual scan, DAT scan, and a lot of combinations in-between.

Description

The existing Import Content menu is reduced to a single entry, Content Scan. By leaving the settings on default values, the operation is identical to the previous auto scan: all files that may be handled by some core are matched against all databases, and results are sorted to playlists named after the matching database.

Changing the method to Custom, new options appear:

• Database Check (use_db, db_usage in the code) controls the match logic: required to match database (Strict), checked but will be added to playlist even if it does not match (Loose), no database check at all (None), and DAT file use.
• Database to Match (db_selection in the code) can be used to restrict matching, speeding up the scan process
• Target Playlist (earlier system name), options are Content dir (i.e. playlist will be named after the directory being scanned), Custom, an existing system, or Automatic. Automatic will use the matched database name if there is one, and falls back to content directory if not.
• DAT file, core selection work the same way as previously in manual scan. Strict DAT = earlier DAT scan with "Arcade DAT filter" enabled.
• Overwrite, validate options appear if the settings guarantee there will be only one playlist affected. Recursive and archive handling can be combined with all scan options.
• "Skip DB reference" will cause all entries to have the playlist name in the db_name field of the playlist even if a database was matched, for people who construct their own thumbnail packs.
• "Scan single file" option (appears after recursive scan is turned off) is a bit clumsy but operational way to add one file to a playlist. Can be combined with any scanning methods.

As for improvements over the previous state, maybe these use cases stand out:

• adding all supported content for a system with a single scan operation, matching files will have canonical names, but non-matching files will appear in the same playlist (if target playlist is chosen correctly)
• adding non-matching content from a large archive file - earlier manual scan only offered one file from inside the archive
• limiting the match to one database will significantly speed up scanning

Testing was done for a fair bit of combinations, to mention a few:

• several combinations of strict and loose scans, directed to one or more playlists
• DAT scan check for regression
• archived and unarchived content
• triggering from all 4 directions (menu, refresh playlist, cli scan, scan button from file browser)
• ensure that recent M3U filtering still works as expected
• also without HAVE_LIBRETRODB (which seems to be only PSP and PS3)

Some coding aspects were postponed to later PRs, to keep the changes manageable:

• get rid of the now unnecessary menu layer
• certain functions have now unnecessary parameters
• align the naming in task_database.c and manual_content_scan.c with current usage
• pushing the scan button twice in a normal file browser will cause UI freeze, but this was the same with the previous scan code, I doubt this is very frequently used as I do not remember any issue opened about it

Related Issues

Helps #16031 (is not implemented as described there, but improves the situation)
Fixes #13077 / #13140 / #13367 / #14792 (autoscan was already doing it, but now all scan options do so)
Fixes #9011 (now it is possible to add those extra file extensions and have it still matched against the database)
Fixes #5440 (scan archives option is obeyed for all scan options)
Fixes #10952

Related Pull Requests

#18621
#18565
#18458

Reviewers

Eyes are welcome. There are also some IOS-specific file picker conditions in the code that I could not verify.

[hizzlekizzle]

This is exciting stuff 👀

[i30817]

Eh.... Does this fix the unintended "feature" caused by retroarch not zeroing strings in these menu?

If you don't know, if you use the manual scanner, there is no way to edit a playlist parameters and then rerun the scanner...

Or so it seems. There is a option in playlist management to "refresh playlist" (one, not all which is another missing feature).

Refresh playlist reruns a scan with the same properties as the playlist by lifting the original values from the playlist itself (@jdgleaver made it so they are recorded when he designed the manual scanner).

Then due to retroarch not cleaning strings, you can go into the manual scanner and all values will be filled, ready to edit that one setting you wanted different and rescanning.

It's a pretty interesting "hidden" feature which is probably going to get "removed" when and if RA gets serious about cstring hygiene.

It would be better ofc if it didn't require a extra rescan just to fill the values and it IS a bug, but it's useful if you use the manual scanner a lot (since the manual scanner is muuuuch faster than the automatic one).

[i30817]

As for my other concern, you know it already, broken thumbnails in custom named or renamed playlists.

It's for that reason I think the directory name option should not exist (edit: see posts below for a alternative and a more cautious change in the last paragraph) and instead the UI should force the user to chose, (a bit like password web forms with a insufficient password) and certainly not by default.

Custom is also dangerous but it's the only way (edit: except views now) from the UI to end with multiple playlists for the same system so I give it some slack even if it DOES break thumbnails. If the explore/view filtering was expanded to positive filtering paths I would support it going away too (to for example, the user placing all scummvm AGS games apart from the rest with a view by simply filtering the paths. Or even better core specific properties to filter, in scummvm case, engine obviously)

It's so-o-o easy for a new user to end up broken thumbnails in the manual scanner (just name the directories scanned "wrong" and don't change the default system name option) it's no wonder most avoid it it, even if it's much faster, and has more features (now). Also it allows hacks to not be recognised as the "original game" if you dont want it (if you do you keep the same name, which is useful for translations/simple hacks to get the same thumbnails as the original).

But if you're doing this to make a 1-to-1 equivalent as a start I understand. I just think "start designs" in retroarch end up being "forever designs" very often when the original feature coder retires.

I understand this refactoring helps coders by DRY various parts of the scanner.

But I also generally think all options to simplify the retroarch UI should at least be considered if not strongly encouraged in new features and redesigns, even at the cost of the user being required to "choose a value" to make sure they're paying attention when the alternative is a possibility broken feature like the wrong system name.

[i30817]

A really really radical alternative would be getting rid of the "system name" setting altogether and it's ALWAYS directory name. Only it would only accept directory names corresponding to system name (databases or another proxy for it in retroarch private data)

This would be a great simplification because the complicated setting would disappear and it would force those not so gifted users to think about where they place their Roms which would hopefully minimise the cases of scanning the same directory but expecting different system playlists without bogus other system files in the manual scanner, and new users would be funneled to the automatic scanner anyway by default in a unified design like this (that can scan any directory).

Power users would still run fast.

However, obviously it would be a radical redesign that would get rid of custom. I wouldn't care (views have potential to replace the one use case I care about), and I doubt 99% would.

The users which are Devs testing new cores without database files could surely put in a empty database file with the name they want for the system to test it out and make the check allow the scan (or a similar workaround).

Yes I think I like this idea even more than just removing Directory name and Custom. The setting disappears and can't be wrong to progress and it has to be chosen naturally because the scan directory is obligatory. Put in a list of the allowed names in the message when it fails, the reasons for it (don't mix multiple system roms, link the correct thumbnails), or a link to it if it's too long and everyone will understand what they have to do.

It would cause a lot of churn to some users, but frankly I think It would be worth it, and the manual scan users that don't already name the directories "correctly" are a minority of a minority probably (the thumbnails being broken with the default if they're wrong kind of ensures it).

If you want to be boring, still keeping system name option but only allowing directories named one of the system names retroarch accepts if directory name is the system name also works and allows naive users to use other directory names if they choose a system name other than the default, but they still can't break thumbnails (at least if they chose a dir without multiple systems). I'd recommend getting rid of custom in this case though. That would often break thumbnails and Devs could add system names to list by whatever mechanism is used to add them for testing new cores.

[i30817]

Anyway, the other user case you should ensure works with your redesigns are portable
playlists.

I think they'll work because that setting only affects paths that match a prefix in playlists iirc, so there is no reason for it to be broken in a UI redesign if the playlist format doesn't change. But if it does, like the proposed removal of the system name option (which is one of the memoized options for rescans I mentioned before, even if not a path) it's worth testing.

[zoltanvb]

you can go into the manual scanner and all values will be filled

This works the same with the new changes. Values are set to default when RetroArch starts, but not saved to config file (with the exception of scan without core match and scan check CRC for duplicates, which were already normal config items). I consider it more of a feature than anything else, it helps adding items from multiple places to the same playlist.

broken thumbnails in custom named or renamed playlists

This will be slightly easier to manage. At least content, that matches the database, will be associated with that, no matter what the playlist is called. (Unless it is turned off with the "skip DB reference" option, or whole DB match is disabled.)

a radical redesign that would get rid of custom

From my side I'm not planning to do any radical changes for multiple reasons.

portable playlists

Fair point, I did not change anything there intentionally, but I'll run a few tests.

[i30817]

I'm begging you, you don't HAVE to do the "radical change". It's a bit too much automagic anyway, I got too excited about it.

"Just" remove custom (which is useless if the cores without databases get entry in the system name list )

And make directory name only accept system database names (and not be default/be the last option)

This is because after not using the dump sets libretro recommends, these options are the major reason why users have no thumbnails or a broken explore tab using manual scanning.

They scan a directory, leave the system name to the default of "directory name" (because they don't understand it) and get those 2 features broken because the directory name wasn't a database name.

Imagine the workflow if the new user had to select one of the options

1. The user would go to scan, and a info popup would say "select system type"
2. They would go to the drop-down and would see 'directory name' at the bottom and a bunch of names they recognize first.
3. Assuming they were going to chose directory name after this and they had the "wrong name" and press scan, a popup would say "the system name as directory name needs to be named one of the recognized systems'

No chance to screw up and no broken expectations

Custom is completely useless if the cores without databases get a entry anyway, except maybe for Devs making a new core. Devs making a new core can use the CMD line to load games instead

[i30817]

Edit: removed the other post talking about this bug, it's better it's a bug report

If you really want custom to stay at least this bug that causes ROMs being loaded by the wrong core should be fixed:

#18665

[zoltanvb]

Checked portable playlists, no changes there due to this PR, works as expected.

[zoltanvb]

Marking it as ready, from my side it could be merged. Documentation update is prepared, will be sent after this is merged.

[RobLoach]

This is a great step forwards in easing the user experience around adding content to the playlists. My only thoughts are that it looks kind of strange to have only one item in the Import Content menu, though I understand the technical reason why it's like that...

Would it make sense for it to be in the Main Menu as "Import Content"? ie...

1. Move the Content Scan menu item to the Main Menu
2. Rename it to "Import Content.

Before

After

[sonninnos]

Import Content location is already optional, and it was defaulted back to sidebar/tabs on request, as in I put it in Main Menu at first when I unified the option for all menu drivers: #17953 & #17517

[zoltanvb]

I do plan to remove the unnecessary layer (while ofc keeping the current configurability), it is one of the postponed changes.