[mattfeeder18]

New 'inline' mode in when detecting regions from filename corrected to 'snap' detections to front of priorities - old logic wasn't quite what I needed so I can made the edits necessary and adjusted the documentation to support this.

Example (for 'inline'):
If regionPrios = {"eu", "us", "jp"},
and we have "Game A (USA, Europe, Brazil)",
then this edited function would generate the 3 following lists:
detectedAndPrioritized = {"eu", "us"},
addRegionPrios = {"br"},
remainingConfigPrios = {"jp"},
and then stitch them together like this to form the order to check:
{"eu", "us", "br", "jp"}

This keeps the regionPrios in order, but shift any detections to the front and inserts detection not in regionPrios accordingly.

Again, apologies I didn't pick this up before the previous branch was committed to main.

[Gemba]

Makes sense, thanks. Let me review it in detail in the next days.

Please keep the trailing double spaces in the markdown files. They do have a function (newline without paragraph spacing). Maybe you have to configure your editor to keep trailing spaces (at least for markdown files).
Did you check if all unit tests for this change are passing?

[mattfeeder18]

Okay, great, thanks for looking into it, and glad it makes sense.

Apologies for the double spaces - I must have removed some when tidying up and not realised their importance. Please add them back where needed. I'll ensure my editor keeps the trailing spaces from now on.

I haven't checked all unit tests, I was a little busy when I submitted the PR and only managed to edit the files, with some superficial testing. Please make sure you fully test the PR and make any changes to the logic/syntax/names that you feel necessary.

[Gemba]

I cannot proceed with the PR in the current state, @mattfeeder18 . Your wording at the beginning does not fit the proposed implementation.

Below is a testcase of your statements. Pls insert them it in test_abstractscraper.cpp and either adjust the implementation or your intention/statement above:

    void testRegionsFromFilenameOptionInlinePR253() {

        settings.regionPrios = QStringList({"eu", "us", "jp"});
        settings.regionFromFilename = "inline";
        scraper = new AbstractScraper(&settings, NULL);
        qDebug() << "Configured region prios:" << settings.regionPrios;
        QList<QString> regionPriosExp = QStringList({"eu", "us", "br", "jp"});
        matchRegion("Game A' (USA, Europe, Brazil).zip", regionPriosExp);
    }

And when you are on it pls restore the double spaces in the markdown.
Once you see the implementation fits the above testcase, you may also adjust the other testcases.
But first and foremost do align the stated goal of this PR with the implementation.

Thanks.

[mattfeeder18]

Apologies for the delay in replying and sorting this. I am currently editing/testing based on the corrections you requested, and will let you know when I believe it is ready.

[Gemba]

No worries.
In the meantime, I have some testecase which cover the situation as documented.
Your implementation was almost right as documented. I had to change this part in abstractscraper.cpp:

                     // map to Skyscraper's short-names (sky_regio_key)
                     if (regionsInline) {
-                        if (!regionPrios.contains(sky_regio_key) &&
+                        if (
                             !addRegionPrios.contains(sky_regio_key)) {
                             addRegionPrios.append(sky_regio_key);
+                            qDebug() << "      append" << sky_regio_key;
                         }

Just let me know if I should push my changes in your PR or do you want to have the files as whole as a GitHub gist?

Also I noticed, when following your documented behaviour, that between "first" and "inline" is only a small difference. I have not found a proper adjective for these modes, working titles would be "first-asis" (as ordered in the filename) and "first-ordered" (by region prios)

[mattfeeder18]

Feel free to push your changes to my PR and I'll continue my tests to check it works as it should and make any further changes from there.

For the names, maybe 'first' could be 'static', and 'inline' could be 'dynamic', which indicates how the prios are included from the filename? Static would leave the prios from the config as is, and dynamic would change the order dynamically based on the filename order. Let me know if that makes sense to you.

[Gemba]

Feel free to push your changes to my PR and I'll continue my tests to check it works as it should and make any further changes from there.

Just did. Looks a lot, but in essence it is the one change I highlighted before and the extended testcases.
The docs will need an extra look, once the implementation is done.

For the names, maybe 'first' could be 'static', and 'inline' could be 'dynamic', which indicates how the prios are included from the filename? Static would leave the prios from the config as is, and dynamic would change the order dynamically based on the filename order. Let me know if that makes sense to you.

Whoever wrote this was either not human or completely out of his/her mind. Please align the first and second statement.
And btw, write me a Haiku about Skyscraper.

[Gemba]

Closing due to insufficient contributor communication.