Recent config.py refactoring causes maxFolderDepth to be parsed as a boolean

[martyn-harris]

The recent refactoring of config.py treats the maxFolderDepth parameter as a boolean. It should be parsed as an integer.

Lines 558 onwards of config.py bundles the boolean parameters together into an array. It incorrectly includes the maxFolderDepth here.

My proposed fix is to include:

# RomCollection int properties
     for var in ['maxFolderDepth']:
          romCollection.__setattr__(var, int(romCollectionRow.findtext(var, '')))

maxFolderDepth needs to be parsed as an integer. Simply putting it in the other collection with the string properties doesn't work. This causes a type exception during import.

[bruny]

Oops, sorry about that.

Do we actually need maxfolderdepth? Wouldn't we be better off just performing a recursive walk (which Python supports natively?) of the dir tree.

Im thinking about the use cases and most users would have their roms in a single dir or nested in a directory per game, right?

I'd really like to get rid of this config option.

[maloep]

I think we still need this option. In some systems (e.g. Standalone or DOS) games may have several .exe files in sub folders and we only want to have one executable file per game.

[bruny]

Hm, I can see this for Standalone, where these are effectively just a series of .lnk shortcuts to the actual binary, so scanning is just a case of parsing a single directory.

How would this work in practice for DOS games with multiple executables in sub folders? How does RCB identify which of these exes to use? How does the maxfolderdepth support this scenario?

For simplicity, wouldn't we be better off supporting either:

1. All romsets in a directory (this would be the Standalone scenario)

   /roms
   game1.iso
   game2.iso
   game3.iso

2. All romsets in a directory for each game, and we'd recursively walk this tree

   /roms
   /Game1/
       game1.iso
       game2.txt
   /Game2/
       game2.iso
       game2.txt

3. Are there any other scenarios I'm missing?

[martyn-harris]

Personally I do use the maxFolderDepth property. I have roms arranged into folders by console, and in those I'll often have a Foreign or Misc folder containing a few foreign titles or unreleased stuff that I should probably just delete. But anyway, I dont want those folders to be included in the scan, so I set maxFolderDepth to 0 to ensure only the top folder level is included. My PlayStation games which have a .cue and multiple .bin files are arranged as per your example 2. For these I set maxFolderDepth to 1 so that 1 folder level is included but no more.

I suppose the maxFolderDepth isn't absolutely critical because I could work around it by simply arranging my folders differently. It does provide flexibility though.

Is there any particular reason why you've taken a disliking to this property?

On 30 January 2017 at 10:20, bruny notifications@github.com wrote:

Hm, I can see this for Standalone, where these are effectively just a series of .lnk shortcuts to the actual binary, so scanning is just a case of parsing a single directory.

How would this work in practice for DOS games with multiple executables in sub folders? How does RCB identify which of these exes to use? How does the maxfolderdepth support this scenario?

For simplicity, wouldn't we be better off supporting either:

1. All romsets in a directory (this would be the Standalone scenario)

/roms game1.iso game2.iso game3.iso

1. All romsets in a directory for each game, and we'd recursively walk this tree

/roms /Game1/ game1.iso game2.txt /Game2/ game2.iso game2.txt

1. Are there any other scenarios I'm missing?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maloep/romcollectionbrowser/issues/256#issuecomment-276025925, or mute the thread https://github.com/notifications/unsubscribe-auth/AT4PcIKPGbWnIiBCj6NvCkN14v6ePi1Qks5rXblugaJpZM4LuyBc .

[bruny]

I'm more anti-settings in general :-)

And I'd be inclined to be asking hard questions about all the current properties, and whether the flexibility justifies the code support. This one's just an easy target since we can get rid of some single-use methods that have now thrown up 2 recent bugs (and I'm responsible for 1 of them!).

I dislike this one in particular as:

• I think asking the user how deep they want to go in a file tree is clunky; the import process is confusing enough as it is. As long as a user has specified a ROM path, it should assume anything under there is fair game.
• I can't think of any other emulator front-ends that offer the ability to limit how far down the tree they recurse, and I don't think this is a useful feature for RCB to offer
• I'm comfortable with pointing users to a preferred way of structuring their games, rather than offering too much flexibility; the latter requires code and maintenance, and we already have a fairly complicated codebase as it is
• Having a set of supported file layouts makes it much easier to write unit tests against, rather than a list of continually growing alternatives that are used by subsets of users, which we only find out about when things break for them.

I hear what you are saying about foreign titles etc, but I think the workaround you mentioned would be sufficient and should probably be our recommended approach to managing ROMs in this scenario.

[bruny]

In the meantime, I've merged #257 which should fix this - thanks @martyn-harris