New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Skyscraper as an alternate scraper #2497

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
4 participants
@cmitu

cmitu commented Oct 1, 2018

This PR is adding a re-worked version of the #2468 PR by @muldjord.

The scriptmodule is derived from the scraper.sh module for the GUI part, for users to get a similar interface for scraping outside Emulationstation.
Skyscraper GUI

I'd like to get some review for the choices I've added by default when configuring Skyscraper on the 1st install and on what should be present/not present in the Gui (especially from @muldjord).

  • When installing through the scriptmodule, the existing configuration folder is preserved; it's also linked to /opt/retropie/configs/all/skyscraper as for any other module. The config.ini is generated only if one is not already present, to preserve any user customizations.
  • Skyscraper has the concept of caching the resources downloaded online, then using the artwork.xml file to compose the image in the gamelist. Therefore, scraping will save some resources (configured in ~/.skyscraper/dbs) on disk and the Gui has an option to purge these cached resources and the current DB size is shown after each scrape.
  • I added an option to not cache any resources on-disk (the --nolocaldb option) and user can choose to scrape like this.
  • Gamelist generation
    • this is configurable by user, default is the normal ES location, but can be toggled through the GUI to the ROM folder.
    • depending on the choice of gamelist location, the media location is either ~/.emulationstation/downloaded_medi or (relative) in $ROMFOLDER/media.
  • The Scraping operation - I've monitored the Skyscraper topic in the RetroPie forums and I think @muldjord recommends scraping online and then a scrap from the local cached resources. I've implemented it like this (2 scrape invocations per system scraped), except when the user disabled caching. @muldjor - I would like your feedback on this.

The GUI configured the following parameters

  • -g/-o : location of the gamelist and generated artwork (Use rom folder for ...)
  • --forcefilename : either use ROM name retured by scraping or force file name as game name.
  • --nolocaldb: cache scraped resources or not
  • --nocovers/--nomarquees/--videos: by default marquees, screenshots and covers are downloaded (set in the .ini file), but videos are not. These options enable/disable downloading of those files.
  • -s <scraping_source>: allow the user to select the scraping source, from the ones supported by Skyscraper. All of them are present, except localdb. I don't know if it makes sens to add it here or not.
    Scrape source selection

Feedback welcome. Once this is added, I think we'll need to modify the Wiki to explain the usage and options available through the GUI.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 1, 2018

muldjord commented Oct 1, 2018

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 1, 2018

First of all, this looks really good! Great work so far. Here are my thoughts:

  • The design of the GUI should probably be designed around retaining the cache as much as possible. The cache is such a central part of Skyscraper in order to provide efficient rescrapes when adding new roms, and to save bandwidth for the sources (this is more important than people realize). I would hate for the users to disable it because they don't understand it. It would be better to put any "Purge" or "Cache disable" options under an "Advanced" menu.

'localdb' should definitely be a part of the sources menu. If users just want to adjust the 'artwork.xml' file and rescrape to get the new look, they would need to just scrape from the cache using '-s localdb'.

A quick general note: When installing, updating Skyscraper, it is very important that it follows the update_skyscraper.sh script from Skyscraper's github (or at least something that gives the exact same outcome). Otherwise people might end up with broken installations that I can't support. :S

Initially I feel like the following options would make sense:

  • Scrape from [source]
    • Set user key or id:passwd [key / id:passwd] ('-u key' or '-u id:passwd')
  • Gamelist / ROM names [SOURCE NAME / filename] (--forcefilename)
  • Bracket info [ENABLE / disable] (--nobrackets)
  • Force cache refresh [enable / DISABLE] (--refresh)
  • Scrape videos [enable / DISABLE] (--videos)
  • Artwork
    • Configure artwork (Start "nano -w ~/.skyscraper/artwork.xml" or something like that)
    • Reset 'artwork.xml' (copy in the default artwork.xml in case user has broken it)
  • Advanced
    • Edit 'config.ini' (Start "nano -w ~/.skyscraper/config.ini" or something like that)
    • Configure non-default paths
      • Set input rom folder [folder] (-i or configure in config.ini)
      • Set media export folder [folder] (-o or configure in config.ini)
      • Set gamelist export folder [folder] (-g or configure in config.ini)
    • Local cache
      • Disable screenshot caching [true / FALSE] (--noscreenshots)
      • Disable cover caching [true / FALSE] (--nocovers)
      • Disable wheel caching [true / FALSE] (--nowheels)
      • Disable marquee caching [true / FALSE] (--nomarquees)
      • Purge type [cover/screenshots/wheel/marquee/video]
      • Purge module [all/thegamesdb, etc...]
      • Purge selected type and module ("--purgedb t:type,m:module", if module is 'all' it should just use "--purgedb t:type")
      • Purge chosen platform, x MB (delete "~/.skyscraper/dbs/[platform]")
      • Purge all resources, x MB (delete "~/.skyscraper/dbs", should probably give a warning before doing this)
  • Scrape single system
  • Scrape all systems

Anything in CAPITALS (such as DISABLE) should probably be the default.

I realize that what I've put in is pretty elaborate. The purging could probably be fine with just the "Purge chosen platform" to begin with. I added the rest because it is an option in Skyscraper.

I will gladly help out with any help texts for each of these menu options.

I'm unsure how to implement the key / id:passwd part of the sources / modules. Some of the requires a key or id:passwd to be set to work at all. And some of them will benefit greatly from it. For instance "screenscraper" is restricted to 1 thread if no id:passwd is provided. But users can gain more threads by helping out with the database or buying more.

Let me know what you think. Of course everything I've put in is just suggestions based on how I feel like Skyscraper should be used. Thank you for giving this a shot!

muldjord commented Oct 1, 2018

First of all, this looks really good! Great work so far. Here are my thoughts:

  • The design of the GUI should probably be designed around retaining the cache as much as possible. The cache is such a central part of Skyscraper in order to provide efficient rescrapes when adding new roms, and to save bandwidth for the sources (this is more important than people realize). I would hate for the users to disable it because they don't understand it. It would be better to put any "Purge" or "Cache disable" options under an "Advanced" menu.

'localdb' should definitely be a part of the sources menu. If users just want to adjust the 'artwork.xml' file and rescrape to get the new look, they would need to just scrape from the cache using '-s localdb'.

A quick general note: When installing, updating Skyscraper, it is very important that it follows the update_skyscraper.sh script from Skyscraper's github (or at least something that gives the exact same outcome). Otherwise people might end up with broken installations that I can't support. :S

Initially I feel like the following options would make sense:

  • Scrape from [source]
    • Set user key or id:passwd [key / id:passwd] ('-u key' or '-u id:passwd')
  • Gamelist / ROM names [SOURCE NAME / filename] (--forcefilename)
  • Bracket info [ENABLE / disable] (--nobrackets)
  • Force cache refresh [enable / DISABLE] (--refresh)
  • Scrape videos [enable / DISABLE] (--videos)
  • Artwork
    • Configure artwork (Start "nano -w ~/.skyscraper/artwork.xml" or something like that)
    • Reset 'artwork.xml' (copy in the default artwork.xml in case user has broken it)
  • Advanced
    • Edit 'config.ini' (Start "nano -w ~/.skyscraper/config.ini" or something like that)
    • Configure non-default paths
      • Set input rom folder [folder] (-i or configure in config.ini)
      • Set media export folder [folder] (-o or configure in config.ini)
      • Set gamelist export folder [folder] (-g or configure in config.ini)
    • Local cache
      • Disable screenshot caching [true / FALSE] (--noscreenshots)
      • Disable cover caching [true / FALSE] (--nocovers)
      • Disable wheel caching [true / FALSE] (--nowheels)
      • Disable marquee caching [true / FALSE] (--nomarquees)
      • Purge type [cover/screenshots/wheel/marquee/video]
      • Purge module [all/thegamesdb, etc...]
      • Purge selected type and module ("--purgedb t:type,m:module", if module is 'all' it should just use "--purgedb t:type")
      • Purge chosen platform, x MB (delete "~/.skyscraper/dbs/[platform]")
      • Purge all resources, x MB (delete "~/.skyscraper/dbs", should probably give a warning before doing this)
  • Scrape single system
  • Scrape all systems

Anything in CAPITALS (such as DISABLE) should probably be the default.

I realize that what I've put in is pretty elaborate. The purging could probably be fine with just the "Purge chosen platform" to begin with. I added the rest because it is an option in Skyscraper.

I will gladly help out with any help texts for each of these menu options.

I'm unsure how to implement the key / id:passwd part of the sources / modules. Some of the requires a key or id:passwd to be set to work at all. And some of them will benefit greatly from it. For instance "screenscraper" is restricted to 1 thread if no id:passwd is provided. But users can gain more threads by helping out with the database or buying more.

Let me know what you think. Of course everything I've put in is just suggestions based on how I feel like Skyscraper should be used. Thank you for giving this a shot!

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 1, 2018

  • The design of the GUI should probably be designed around retaining the cache as much as possible. The cache is such a central part of Skyscraper in order to provide efficient rescrapes when adding new roms, and to save bandwidth for the sources (this is more important than people realize). I would hate for the users to disable it because they don't understand it. It would be better to put any "Purge" or "Cache disable" options under an "Advanced" menu.

I know that, that's why the caching is enabled by default and I think it should remain.

My rationale for putting the 'purge cache' as a first level entry is the way other scrapers (in RetroPie right now) work - they don't have any caching. The user simply invokes the scraper without any other worry and then just starts ES to see the result. Skyscraper is of course better in this regard, but if the user is unaware of such caching taking place, we might risk that using the scraper repeatedly - on long term - resources might accumulate on disk over time. Of course, Skyscraper caching will be documented, but I'm not putting much hope in users reading it and remembering this caching behavior.

Having said that, I'm ok putting the option under an advanced menu.

'localdb' should definitely be a part of the sources menu. If users just want to adjust the 'artwork.xml' file and rescrape to get the new look, they would need to just scrape from the cache using '-s localdb'.

I thought about this too and I was thinking of putting both Local sources (Import and Localdb) in a separate menu entry, but this would make it cumbersone to know which is actual source for scraping.
I think I'll add it to the main scraper source selection and show something like this on the main menu:

  • Scrape from ONLINE: <online_scraper>
    ...
  • Scrape from CACHE: Import
  • Scraper from CACHE: LocalCache

What do you think ?

A quick general note: When installing, updating Skyscraper, it is very important that it follows the update_skyscraper.sh script from Skyscraper's github (or at least something that gives the exact same outcome). Otherwise people might end up with broken installations that I can't support. :S

That's a good remark. There are 2 steps with installing Skyscraper (as with any RetroPie module)

  • Installation - this is done from the latest version available on Github, from scratch (i.e. I don't check the installed version to see if needs updating).
  • Configuration files - right now I've included int the $confdir whatever Skyscraper considers to add to ~/.skyscraper. You can see the list of files (and folders created) in the init_config_skyscraper function.

Configuration wise, the make install in your update script is copying the files listed in the qmake file (skyscraper.pro) - on the `unix:examples.files' line - to the install folder to make sure you have everything ok. Right now the scriptmodule is ok in this regard, but what I think you mean is that this list of files could change, so that the installation performed by the script will not copy all the necesary files.
Am I correct ? If so, we have 2 options here:

  1. Update the scriptmodule whenever the Skyscraper required files are changed.A bit cumbersome, but nevertheless very effective.
  2. Parse the unix:examples.files line from the qmake file and copy the required files in the $confdir during the configuration (which will automatically be called during an installation). This is a bit more foolproof, but has 1 disadvantage - we can't know which of the files can be user customized and we risk to overwrite one of them (artwork.xml comes to mind, without mentioning config.ini). This can be mitigated by splitting this files in 2 lists (config + resources) and copying the config set of file without overwriting them, while the resources set of files can be easily overwritten (since they're not meant to be edited by the user).

What do you think ?

OK, about the proposed GUI menu. The idea behind it is to let the user do the scraping with as little hassle as possible, being able to control it with simple toggleable options. It's not meant to be a full GUI for any option in Skyscraper - at least not in this first iteration :). This is also the concept behind the existing scraper module, which also has a lot more options available, but they're not exposed to the UI.
Advanced usage should be reserved to the command line and should not (all) be included in the GUI. This complicates the UI and having more screen levels in a - relatively - cramped UI might confuse some of them. The configuration files (artwork.xml and config.ini) are available via file shares and I'd incurage the beginner/advanced users to edit them that way (especially the artwork.xml).

Having said that, let's take a look at it one by one.

Initially I feel like the following options would make sense:

  • Scrape from [source]

Yep, should obviously be in.

  • Set user key or id:passwd [key / id:passwd] ('-u key' or '-u id:passwd')

I wouldn't put this here. Ideally, it would be next to the scrape source selection, but for now I would just leave it out and document how to set this in config.ini.

  • Gamelist / ROM names [SOURCE NAME / filename] (--forcefilename)

Yep, already there.

  • Bracket info [ENABLE / disable] (--nobrackets)

Yep, I thought about adding this one also.

  • Force cache refresh [enable / DISABLE] (--refresh)

I'm not sure about this one - right now online scraping is done with --refresh. You think we should remove this options from the scraping operation and have a separate refresh option ?

  • Scrape videos [enable / DISABLE] (--videos)

Yep, already there.

  • Artwork

    • Configure artwork (Start "nano -w ~/.skyscraper/artwork.xml" or something like that)
    • Reset 'artwork.xml' (copy in the default artwork.xml in case user has broken it)

I wouldn't add this at all. I consider the artwork.xml editing an advanced feature and should be carried out separately. The script doesn't overwrite the artwork.xml on install/update, but (as with config.ini) creates an default one in the config folder that can be used to overwrite a botched artwork.xml file.

  • Advanced

    • Edit 'config.ini' (Start "nano -w ~/.skyscraper/config.ini" or something like that)

Yes, that sounds right.

  • Configure non-default paths

    • Set input rom folder [folder] (-i or configure in config.ini)
    • Set media export folder [folder] (-o or configure in config.ini)
    • Set gamelist export folder [folder] (-g or configure in config.ini)

IMHO non-default paths for gamelist/media download should not be exposed. I understand they're supported by the Scraper, but ES doesn't read arbitrarily gamelist.xml files so we have 2 options here, and we make the user choose between them.
The same about the input ROM folder - it's configured to the default RetroPie path in the config.ini and I don't think we should let it be changed - at least not through the GUI.

Any advanced user can override these via CLI or through the -c to point to a different config file, but I think it's safer to make the right choices here.

  • Local cache

    • Disable screenshot caching [true / FALSE] (--noscreenshots)
    • Disable cover caching [true / FALSE] (--nocovers)
    • Disable wheel caching [true / FALSE] (--nowheels)
    • Disable marquee caching [true / FALSE] (--nomarquees)

Yep, some of them are already there, but I can add some of them. As a side note, you don't have the corresponding switched options. Usually when a program implements the --no[feature], also has the --feature implemented. But I digress :).

* Purge type [cover/screenshots/wheel/marquee/video]
* Purge module [all/thegamesdb, etc...]
* Purge selected type and module ("--purgedb t:type,m:module", if module is 'all' it should just use "--purgedb t:type")
* Purge chosen platform, x MB (delete "~/.skyscraper/dbs/[platform]")
* Purge all resources, x MB (delete "~/.skyscraper/dbs", should probably give a warning before doing this)

I see the options behind this, but for a 1st version, I think the Purge options should be limited to 2:

  • Purge all
  • Purge one system

Ok, maybe purge by type, since the user might want to purge all videos but refresh the other media, but can it be done without specifying a system ?

  • Scrape single system
  • Scrape all systems

Yep, already there, but I think they should remain on the first lines. This will enable the user just select the system and start scraping.

Thank you for your time and review !
I'll see how to add the advanced options and re-organise the GUI based on your feedback. I think I also found a bug w.r.t. gamelist generation, but I'll keep testing while modifying the scriptmodule.

cmitu commented Oct 1, 2018

  • The design of the GUI should probably be designed around retaining the cache as much as possible. The cache is such a central part of Skyscraper in order to provide efficient rescrapes when adding new roms, and to save bandwidth for the sources (this is more important than people realize). I would hate for the users to disable it because they don't understand it. It would be better to put any "Purge" or "Cache disable" options under an "Advanced" menu.

I know that, that's why the caching is enabled by default and I think it should remain.

My rationale for putting the 'purge cache' as a first level entry is the way other scrapers (in RetroPie right now) work - they don't have any caching. The user simply invokes the scraper without any other worry and then just starts ES to see the result. Skyscraper is of course better in this regard, but if the user is unaware of such caching taking place, we might risk that using the scraper repeatedly - on long term - resources might accumulate on disk over time. Of course, Skyscraper caching will be documented, but I'm not putting much hope in users reading it and remembering this caching behavior.

Having said that, I'm ok putting the option under an advanced menu.

'localdb' should definitely be a part of the sources menu. If users just want to adjust the 'artwork.xml' file and rescrape to get the new look, they would need to just scrape from the cache using '-s localdb'.

I thought about this too and I was thinking of putting both Local sources (Import and Localdb) in a separate menu entry, but this would make it cumbersone to know which is actual source for scraping.
I think I'll add it to the main scraper source selection and show something like this on the main menu:

  • Scrape from ONLINE: <online_scraper>
    ...
  • Scrape from CACHE: Import
  • Scraper from CACHE: LocalCache

What do you think ?

A quick general note: When installing, updating Skyscraper, it is very important that it follows the update_skyscraper.sh script from Skyscraper's github (or at least something that gives the exact same outcome). Otherwise people might end up with broken installations that I can't support. :S

That's a good remark. There are 2 steps with installing Skyscraper (as with any RetroPie module)

  • Installation - this is done from the latest version available on Github, from scratch (i.e. I don't check the installed version to see if needs updating).
  • Configuration files - right now I've included int the $confdir whatever Skyscraper considers to add to ~/.skyscraper. You can see the list of files (and folders created) in the init_config_skyscraper function.

Configuration wise, the make install in your update script is copying the files listed in the qmake file (skyscraper.pro) - on the `unix:examples.files' line - to the install folder to make sure you have everything ok. Right now the scriptmodule is ok in this regard, but what I think you mean is that this list of files could change, so that the installation performed by the script will not copy all the necesary files.
Am I correct ? If so, we have 2 options here:

  1. Update the scriptmodule whenever the Skyscraper required files are changed.A bit cumbersome, but nevertheless very effective.
  2. Parse the unix:examples.files line from the qmake file and copy the required files in the $confdir during the configuration (which will automatically be called during an installation). This is a bit more foolproof, but has 1 disadvantage - we can't know which of the files can be user customized and we risk to overwrite one of them (artwork.xml comes to mind, without mentioning config.ini). This can be mitigated by splitting this files in 2 lists (config + resources) and copying the config set of file without overwriting them, while the resources set of files can be easily overwritten (since they're not meant to be edited by the user).

What do you think ?

OK, about the proposed GUI menu. The idea behind it is to let the user do the scraping with as little hassle as possible, being able to control it with simple toggleable options. It's not meant to be a full GUI for any option in Skyscraper - at least not in this first iteration :). This is also the concept behind the existing scraper module, which also has a lot more options available, but they're not exposed to the UI.
Advanced usage should be reserved to the command line and should not (all) be included in the GUI. This complicates the UI and having more screen levels in a - relatively - cramped UI might confuse some of them. The configuration files (artwork.xml and config.ini) are available via file shares and I'd incurage the beginner/advanced users to edit them that way (especially the artwork.xml).

Having said that, let's take a look at it one by one.

Initially I feel like the following options would make sense:

  • Scrape from [source]

Yep, should obviously be in.

  • Set user key or id:passwd [key / id:passwd] ('-u key' or '-u id:passwd')

I wouldn't put this here. Ideally, it would be next to the scrape source selection, but for now I would just leave it out and document how to set this in config.ini.

  • Gamelist / ROM names [SOURCE NAME / filename] (--forcefilename)

Yep, already there.

  • Bracket info [ENABLE / disable] (--nobrackets)

Yep, I thought about adding this one also.

  • Force cache refresh [enable / DISABLE] (--refresh)

I'm not sure about this one - right now online scraping is done with --refresh. You think we should remove this options from the scraping operation and have a separate refresh option ?

  • Scrape videos [enable / DISABLE] (--videos)

Yep, already there.

  • Artwork

    • Configure artwork (Start "nano -w ~/.skyscraper/artwork.xml" or something like that)
    • Reset 'artwork.xml' (copy in the default artwork.xml in case user has broken it)

I wouldn't add this at all. I consider the artwork.xml editing an advanced feature and should be carried out separately. The script doesn't overwrite the artwork.xml on install/update, but (as with config.ini) creates an default one in the config folder that can be used to overwrite a botched artwork.xml file.

  • Advanced

    • Edit 'config.ini' (Start "nano -w ~/.skyscraper/config.ini" or something like that)

Yes, that sounds right.

  • Configure non-default paths

    • Set input rom folder [folder] (-i or configure in config.ini)
    • Set media export folder [folder] (-o or configure in config.ini)
    • Set gamelist export folder [folder] (-g or configure in config.ini)

IMHO non-default paths for gamelist/media download should not be exposed. I understand they're supported by the Scraper, but ES doesn't read arbitrarily gamelist.xml files so we have 2 options here, and we make the user choose between them.
The same about the input ROM folder - it's configured to the default RetroPie path in the config.ini and I don't think we should let it be changed - at least not through the GUI.

Any advanced user can override these via CLI or through the -c to point to a different config file, but I think it's safer to make the right choices here.

  • Local cache

    • Disable screenshot caching [true / FALSE] (--noscreenshots)
    • Disable cover caching [true / FALSE] (--nocovers)
    • Disable wheel caching [true / FALSE] (--nowheels)
    • Disable marquee caching [true / FALSE] (--nomarquees)

Yep, some of them are already there, but I can add some of them. As a side note, you don't have the corresponding switched options. Usually when a program implements the --no[feature], also has the --feature implemented. But I digress :).

* Purge type [cover/screenshots/wheel/marquee/video]
* Purge module [all/thegamesdb, etc...]
* Purge selected type and module ("--purgedb t:type,m:module", if module is 'all' it should just use "--purgedb t:type")
* Purge chosen platform, x MB (delete "~/.skyscraper/dbs/[platform]")
* Purge all resources, x MB (delete "~/.skyscraper/dbs", should probably give a warning before doing this)

I see the options behind this, but for a 1st version, I think the Purge options should be limited to 2:

  • Purge all
  • Purge one system

Ok, maybe purge by type, since the user might want to purge all videos but refresh the other media, but can it be done without specifying a system ?

  • Scrape single system
  • Scrape all systems

Yep, already there, but I think they should remain on the first lines. This will enable the user just select the system and start scraping.

Thank you for your time and review !
I'll see how to add the advanced options and re-organise the GUI based on your feedback. I think I also found a bug w.r.t. gamelist generation, but I'll keep testing while modifying the scriptmodule.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 1, 2018

Scrape from ONLINE: <online_scraper>
...
Scrape from CACHE: Import
Scraper from CACHE: LocalCache

What do you think ?

Yes, but it should probably be

  • Online source: [source]
  • Local source: [localdb/import]
    That's what I use in the Skyscraper documentation aswell.

...but what I think you mean is that this list of files could change, so that the installation performed by the script will not copy all the necesary files.
Am I correct ?

It is imperative that Skyscraper is installed with the "sudo make install" command. The files are copied to the /usr/local folders and I check those upon running Skyscraper, to make sure the example files and other files are up-to-date inside ~/.skyscraper. If it's done just by copying the files into ~/.skyscraper, my entire file checking routine breaks.

I'm not sure about this one - right now online scraping is done with --refresh. You think we should remove this options from the scraping operation and have a separate refresh option ?

I would prefer not to have '--refresh' added by default. The idea is that the user can rescrape their entire library whenever they want, and it will only contact the online sources for the new roms they have added. I feel a responsibility to make sure Skyscraper doesn't strain the sources. And I feel this GUI needs to reflect that. :) If the user don't use the advanced features anyways, they will get the same data from the cache and from the online source anyway. And with a helpful text on the entry, I think it will be useful as a bool.

Concerning the purging, I don't know man. It's up to you I guess. I kind of agree that the advanced usage can be left out for a first version. :)

I'm sorry if I at any point seem a bit on the fence about any of this. It bugs me a bit to "boil it down" to the bare minimum like this, as Skyscraper is "just another scraper" if the user doesn't take advantage of the artwork compositor, the config.ini possibilities, the user/id for the modules and of course the cache options. But I understand the need to, I just don't like it. :D

muldjord commented Oct 1, 2018

Scrape from ONLINE: <online_scraper>
...
Scrape from CACHE: Import
Scraper from CACHE: LocalCache

What do you think ?

Yes, but it should probably be

  • Online source: [source]
  • Local source: [localdb/import]
    That's what I use in the Skyscraper documentation aswell.

...but what I think you mean is that this list of files could change, so that the installation performed by the script will not copy all the necesary files.
Am I correct ?

It is imperative that Skyscraper is installed with the "sudo make install" command. The files are copied to the /usr/local folders and I check those upon running Skyscraper, to make sure the example files and other files are up-to-date inside ~/.skyscraper. If it's done just by copying the files into ~/.skyscraper, my entire file checking routine breaks.

I'm not sure about this one - right now online scraping is done with --refresh. You think we should remove this options from the scraping operation and have a separate refresh option ?

I would prefer not to have '--refresh' added by default. The idea is that the user can rescrape their entire library whenever they want, and it will only contact the online sources for the new roms they have added. I feel a responsibility to make sure Skyscraper doesn't strain the sources. And I feel this GUI needs to reflect that. :) If the user don't use the advanced features anyways, they will get the same data from the cache and from the online source anyway. And with a helpful text on the entry, I think it will be useful as a bool.

Concerning the purging, I don't know man. It's up to you I guess. I kind of agree that the advanced usage can be left out for a first version. :)

I'm sorry if I at any point seem a bit on the fence about any of this. It bugs me a bit to "boil it down" to the bare minimum like this, as Skyscraper is "just another scraper" if the user doesn't take advantage of the artwork compositor, the config.ini possibilities, the user/id for the modules and of course the cache options. But I understand the need to, I just don't like it. :D

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 1, 2018

Scrape from ONLINE: <online_scraper>
...
Scrape from CACHE: Import
Scraper from CACHE: LocalCache

What do you think ?

Yes, but it should probably be

  • Online source: [source]
  • Local source: [localdb/import]
    That's what I use in the Skyscraper documentation aswell.

OK, I'll try to find a way make it exclusive. Right now the source is just one selection and the user scrapes by using one of the

  • scrape all systems
  • scrape selected systems.

...but what I think you mean is that this list of files could change, so that the installation performed by the script will not copy all the necesary files.
Am I correct ?

It is imperative that Skyscraper is installed with the "sudo make install" command. The files are copied to the /usr/local folders and I check those upon running Skyscraper, to make sure the example files and other files are up-to-date inside ~/.skyscraper. If it's done just by copying the files into ~/.skyscraper, my entire file checking routine breaks.

The script does the installation and the update and makes sure these files are copied there - it's similar to a make install. I noticed this check (from /usr/local) - but how would this break the file checking routine ? Remember the scraper is run as an user, there's no write access outside the user's homepage and the

I'm not sure about this one - right now online scraping is done with --refresh. You think we should remove this options from the scraping operation and have a separate refresh option ?

I would prefer not to have '--refresh' added by default. The idea is that the user can rescrape their entire library whenever they want, and it will only contact the online sources for the new roms they have added. I feel a responsibility to make sure Skyscraper doesn't strain the sources. And I feel this GUI needs to reflect that. :) If the user don't use the advanced features anyways, they will get the same data from the cache and from the online source anyway. And with a helpful text on the entry, I think it will be useful as a bool.

You're right, I'll modify this accordingly.

Concerning the purging, I don't know man. It's up to you I guess. I kind of agree that the advanced usage can be left out for a first version. :)

I'm sorry if I at any point seem a bit on the fence about any of this. It bugs me a bit to "boil it down" to the bare minimum like this, as Skyscraper is "just another scraper" if the user doesn't take advantage of the artwork compositor, the config.ini possibilities, the user/id for the modules and of course the cache options. But I understand the need to, I just don't like it. :D

Skyscraper is not 'just another scraper' and users will still benefit from it, event without having access to advanced features from the GUI. You are already an advanced user and having all these features not available seems to you that we're hiding or obscuring these things for the user. Think of it this way - we can add more features later in the GUI as more users will get in touch with it and they discover the possibilities.

cmitu commented Oct 1, 2018

Scrape from ONLINE: <online_scraper>
...
Scrape from CACHE: Import
Scraper from CACHE: LocalCache

What do you think ?

Yes, but it should probably be

  • Online source: [source]
  • Local source: [localdb/import]
    That's what I use in the Skyscraper documentation aswell.

OK, I'll try to find a way make it exclusive. Right now the source is just one selection and the user scrapes by using one of the

  • scrape all systems
  • scrape selected systems.

...but what I think you mean is that this list of files could change, so that the installation performed by the script will not copy all the necesary files.
Am I correct ?

It is imperative that Skyscraper is installed with the "sudo make install" command. The files are copied to the /usr/local folders and I check those upon running Skyscraper, to make sure the example files and other files are up-to-date inside ~/.skyscraper. If it's done just by copying the files into ~/.skyscraper, my entire file checking routine breaks.

The script does the installation and the update and makes sure these files are copied there - it's similar to a make install. I noticed this check (from /usr/local) - but how would this break the file checking routine ? Remember the scraper is run as an user, there's no write access outside the user's homepage and the

I'm not sure about this one - right now online scraping is done with --refresh. You think we should remove this options from the scraping operation and have a separate refresh option ?

I would prefer not to have '--refresh' added by default. The idea is that the user can rescrape their entire library whenever they want, and it will only contact the online sources for the new roms they have added. I feel a responsibility to make sure Skyscraper doesn't strain the sources. And I feel this GUI needs to reflect that. :) If the user don't use the advanced features anyways, they will get the same data from the cache and from the online source anyway. And with a helpful text on the entry, I think it will be useful as a bool.

You're right, I'll modify this accordingly.

Concerning the purging, I don't know man. It's up to you I guess. I kind of agree that the advanced usage can be left out for a first version. :)

I'm sorry if I at any point seem a bit on the fence about any of this. It bugs me a bit to "boil it down" to the bare minimum like this, as Skyscraper is "just another scraper" if the user doesn't take advantage of the artwork compositor, the config.ini possibilities, the user/id for the modules and of course the cache options. But I understand the need to, I just don't like it. :D

Skyscraper is not 'just another scraper' and users will still benefit from it, event without having access to advanced features from the GUI. You are already an advanced user and having all these features not available seems to you that we're hiding or obscuring these things for the user. Think of it this way - we can add more features later in the GUI as more users will get in touch with it and they discover the possibilities.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 1, 2018

The script does the installation and the update and makes sure these files are copied there

Oh, so it copies it to the same folders in /usr/local? Then it should work as expected (I thought it just copied the files directly to "~/.skyscraper" which wouldn't work well). Then yes, the only problem would be if I change the installation script.

Think of it this way - we can add more features later in the GUI as more users will get in touch with it and they discover the possibilities.

Yes, I agree.

muldjord commented Oct 1, 2018

The script does the installation and the update and makes sure these files are copied there

Oh, so it copies it to the same folders in /usr/local? Then it should work as expected (I thought it just copied the files directly to "~/.skyscraper" which wouldn't work well). Then yes, the only problem would be if I change the installation script.

Think of it this way - we can add more features later in the GUI as more users will get in touch with it and they discover the possibilities.

Yes, I agree.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 1, 2018

The script does the installation and the update and makes sure these files are copied there

Oh, so it copies it to the same folders in /usr/local? Then it should work as expected (I thought it just copied the files directly to "~/.skyscraper" which wouldn't work well). Then yes, the only problem would be if I change the installation script.

Not exactly. What I mean is that your existing routine is:

  • make install copies the files in /usr/local/etc
  • when running, Skyscraper checks the files in /usr/local/etc and then copies them into ~/.skyscraper.

but the scriptmodule will copy those exact files into ~/.skyscraper directly, so the result is the same: the most recent files from your distribution will end up in the user's configuration folder. The only exceptions to this rule are config.ini and artwork.xml (those are copied/generated only on the 1st installation).

cmitu commented Oct 1, 2018

The script does the installation and the update and makes sure these files are copied there

Oh, so it copies it to the same folders in /usr/local? Then it should work as expected (I thought it just copied the files directly to "~/.skyscraper" which wouldn't work well). Then yes, the only problem would be if I change the installation script.

Not exactly. What I mean is that your existing routine is:

  • make install copies the files in /usr/local/etc
  • when running, Skyscraper checks the files in /usr/local/etc and then copies them into ~/.skyscraper.

but the scriptmodule will copy those exact files into ~/.skyscraper directly, so the result is the same: the most recent files from your distribution will end up in the user's configuration folder. The only exceptions to this rule are config.ini and artwork.xml (those are copied/generated only on the 1st installation).

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 1, 2018

It might be ok. I can't remember if there are issues. I spend a lot of time on those checking routines, but it's been a long since I did them, so I can't remember the details. Only way to test it properly would be to just install it from scratch using this script (without having any of the files in /usr/local) and see what happens.

muldjord commented Oct 1, 2018

It might be ok. I can't remember if there are issues. I spend a lot of time on those checking routines, but it's been a long since I did them, so I can't remember the details. Only way to test it properly would be to just install it from scratch using this script (without having any of the files in /usr/local) and see what happens.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 1, 2018

It might be ok. I can't remember if there are issues. I spend a lot of time on those checking routines, but it's been a long since I did them, so I can't remember the details. Only way to test it properly would be to just install it from scratch using this script (without having any of the files in /usr/local) and see what happens.

This is how I tested first, before starting any development of the module, and this is how I now test the GUI actions and their options. In both my test installations (Raspbian - PI3 and Ubuntu - PC), I didn't install anything in /usr/local.

cmitu commented Oct 1, 2018

It might be ok. I can't remember if there are issues. I spend a lot of time on those checking routines, but it's been a long since I did them, so I can't remember the details. Only way to test it properly would be to just install it from scratch using this script (without having any of the files in /usr/local) and see what happens.

This is how I tested first, before starting any development of the module, and this is how I now test the GUI actions and their options. In both my test installations (Raspbian - PI3 and Ubuntu - PC), I didn't install anything in /usr/local.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 3, 2018

Based on the feedback from 2 days ago, I've made some changes to the GUI. I've only pushed the changes in my fork repository, but not yet in this PR.

Main screen

Main GUI
I've added the options you suggested:

  • Force cache refresh for --refresh

  • Bracket info for --nobrackets

  • changed a bit the Scraper selection screen and merged the LocalDB and the Import module
    Scrape Selection

  • added an Advanced sub-dialog

The Advanced section

Advanced screen

The advanced screen is split in 3 sections. I moved here the stats for the Local Cache (from the main GUI)

  1. The Local Cache options you suggested to be moved here - caching of Wheels, Covers, Screenshots, Marquees
  2. The Purging options - either by platform or for all resources. When selecting the purging of All resources, a warning will be shown.

Erase all warning

  1. The - really - advanced options to edit the configuration files.

Let me know what you think.

Things that I know can be improved in this iteration (but I haven't got to it yet):

  • (Purging) when purging an individual system, show how much space should be freed in the system selection list.
  • (UI) Replace the Enabled/Disabled states with something like Enabled/Disabled vs Enabled/Disabled so it's clear it's a 2 state option.
  • (Install) instead of putting the list of necessary files in the script, parse the .pro file (used by qmake) and extract the list of files from there. Important files like config.ini and artwork.xml will be handled individually (i.e. making sure we're not overwriting them on upgrade).
  • (UI) add a Help button to document the option.

cmitu commented Oct 3, 2018

Based on the feedback from 2 days ago, I've made some changes to the GUI. I've only pushed the changes in my fork repository, but not yet in this PR.

Main screen

Main GUI
I've added the options you suggested:

  • Force cache refresh for --refresh

  • Bracket info for --nobrackets

  • changed a bit the Scraper selection screen and merged the LocalDB and the Import module
    Scrape Selection

  • added an Advanced sub-dialog

The Advanced section

Advanced screen

The advanced screen is split in 3 sections. I moved here the stats for the Local Cache (from the main GUI)

  1. The Local Cache options you suggested to be moved here - caching of Wheels, Covers, Screenshots, Marquees
  2. The Purging options - either by platform or for all resources. When selecting the purging of All resources, a warning will be shown.

Erase all warning

  1. The - really - advanced options to edit the configuration files.

Let me know what you think.

Things that I know can be improved in this iteration (but I haven't got to it yet):

  • (Purging) when purging an individual system, show how much space should be freed in the system selection list.
  • (UI) Replace the Enabled/Disabled states with something like Enabled/Disabled vs Enabled/Disabled so it's clear it's a 2 state option.
  • (Install) instead of putting the list of necessary files in the script, parse the .pro file (used by qmake) and extract the list of files from there. Important files like config.ini and artwork.xml will be handled individually (i.e. making sure we're not overwriting them on upgrade).
  • (UI) add a Help button to document the option.
@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Oct 4, 2018

Member

Thanks for your work on this. Let me know when you are happy with the functionality and I will do a review before merge etc. But looks good. Cheers

Member

joolswills commented Oct 4, 2018

Thanks for your work on this. Let me know when you are happy with the functionality and I will do a review before merge etc. But looks good. Cheers

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 4, 2018

@joolswills I'll buzz you for a review when I'm thinking it's good enough.

cmitu commented Oct 4, 2018

@joolswills I'll buzz you for a review when I'm thinking it's good enough.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 4, 2018

Great work, looks good. One comment: For the "Select scraping source" it should probably say "Local: LocalDb cache" and "Local: Import folder". So it's "Online" and "Local".

muldjord commented Oct 4, 2018

Great work, looks good. One comment: For the "Select scraping source" it should probably say "Local: LocalDb cache" and "Local: Import folder". So it's "Online" and "Local".

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 5, 2018

OK, I got the previous points implemented - except the UX part, because it doesn't look so good.

  • I added a size indicator when purging a system. The total size of the local DB is shown when entering the Advanced sub-menu
  • Installation now looks in the skyscraper.pro and gets all the files declared as unix:examples.files in order to copy them in the config folder.
  • I added a <Help> button that explains the options, with the corresponding CLI parameter that's associated with that option (where it applies).

@muldjord you can test the GUI and see the help messages by copying the skyscraper.sh file into $HOME/RetroPie-Setup/scriptmodules/supplementary, then running

cd $HOME/RetroPie-Setup/
sudo ./retropie_packages skyscraper gui

This will just show the GUI and you'll be able to change options, see how the Help looks like. When trying to scrape, it will probably give you an error unless you install the package first.

The package is in the optional section (we can move it to experimental at first); if installed, the GUI can be started from the main Configuration/Tools menu in RetroPie-Setup.

Let me know if you think we could add other options from the ones available (--unpack ?, --lang ?) in Skyscraper.

cmitu commented Oct 5, 2018

OK, I got the previous points implemented - except the UX part, because it doesn't look so good.

  • I added a size indicator when purging a system. The total size of the local DB is shown when entering the Advanced sub-menu
  • Installation now looks in the skyscraper.pro and gets all the files declared as unix:examples.files in order to copy them in the config folder.
  • I added a <Help> button that explains the options, with the corresponding CLI parameter that's associated with that option (where it applies).

@muldjord you can test the GUI and see the help messages by copying the skyscraper.sh file into $HOME/RetroPie-Setup/scriptmodules/supplementary, then running

cd $HOME/RetroPie-Setup/
sudo ./retropie_packages skyscraper gui

This will just show the GUI and you'll be able to change options, see how the Help looks like. When trying to scrape, it will probably give you an error unless you install the package first.

The package is in the optional section (we can move it to experimental at first); if installed, the GUI can be started from the main Configuration/Tools menu in RetroPie-Setup.

Let me know if you think we could add other options from the ones available (--unpack ?, --lang ?) in Skyscraper.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 6, 2018

Currently testing the ui:

  • When choosing "Cancel" in "Scrape chosen systems" it says "ROMS have been scraped" instead of just going back to the main Skyscraper menu.
  • In "Scrape from..." menu "Open Retro" should be "OpenRetro".
  • For "Remove bracket info", when it's "Disabled", the B in bracket suddenly becomes uppercase.

For help texts:

  • For "Scrape from..." help text it says "Open Retro", which should be "OpenRetro".
  • For "Scrape from..." help text it would probably make sense to add a small help text saying something like "Some sources need a or user:password to work, which can be set per source in config.ini" or similar. Otherwise I foresee a lot of support requests complaining about certain sources not working.
  • For "Purge system" is says "Skyscraper parameter: --purgedb ". This is not correct. The '--purgedb' option can't purge a system. It can only purge certain types from certain modules. The "Skyscraper parameter" line could probably just be removed. I assume it currently just removes the corresponding folders without actually instantiating Skyscraper right now.
  • For "Download videos" it should probably say "Toggle the download and caching of videos when scraping. This also toggles whether the videos will be included when generating the final game list." or similar.
  • For all of the "Cache " options the help should be "Toggle whether are cached locally when scraping".

I think that is all I have for now.

On whether more options should be supported:

  • I think we should leave '--unpack' out for now, as it is a bit of an experimental feature and requires "7z" to be installed.
  • '--region' would be useful, although it is only supported by the "ScreenScraper" source right now (and should probably say so in the help text). But I think it could make sense to add it with "eu" (default), "us" and "jp" regions. It will always try all regions regardless, but the one chose is the one it will prioritize highest. If '--region' is left out, "eu" is the region Skyscraper prioritizes highest by default, then the rest.
  • '--lang' could also be implemented, but I don't have a good grasp on how much people use this. Personally I always leave it out to just use the default "en", but some users might want "es", "de" or others. This relates mostly, if not only, to the texts. Maybe also screenshots, I can't remember.

muldjord commented Oct 6, 2018

Currently testing the ui:

  • When choosing "Cancel" in "Scrape chosen systems" it says "ROMS have been scraped" instead of just going back to the main Skyscraper menu.
  • In "Scrape from..." menu "Open Retro" should be "OpenRetro".
  • For "Remove bracket info", when it's "Disabled", the B in bracket suddenly becomes uppercase.

For help texts:

  • For "Scrape from..." help text it says "Open Retro", which should be "OpenRetro".
  • For "Scrape from..." help text it would probably make sense to add a small help text saying something like "Some sources need a or user:password to work, which can be set per source in config.ini" or similar. Otherwise I foresee a lot of support requests complaining about certain sources not working.
  • For "Purge system" is says "Skyscraper parameter: --purgedb ". This is not correct. The '--purgedb' option can't purge a system. It can only purge certain types from certain modules. The "Skyscraper parameter" line could probably just be removed. I assume it currently just removes the corresponding folders without actually instantiating Skyscraper right now.
  • For "Download videos" it should probably say "Toggle the download and caching of videos when scraping. This also toggles whether the videos will be included when generating the final game list." or similar.
  • For all of the "Cache " options the help should be "Toggle whether are cached locally when scraping".

I think that is all I have for now.

On whether more options should be supported:

  • I think we should leave '--unpack' out for now, as it is a bit of an experimental feature and requires "7z" to be installed.
  • '--region' would be useful, although it is only supported by the "ScreenScraper" source right now (and should probably say so in the help text). But I think it could make sense to add it with "eu" (default), "us" and "jp" regions. It will always try all regions regardless, but the one chose is the one it will prioritize highest. If '--region' is left out, "eu" is the region Skyscraper prioritizes highest by default, then the rest.
  • '--lang' could also be implemented, but I don't have a good grasp on how much people use this. Personally I always leave it out to just use the default "en", but some users might want "es", "de" or others. This relates mostly, if not only, to the texts. Maybe also screenshots, I can't remember.
@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 6, 2018

Currently testing the ui:

* When choosing "Cancel" in "Scrape chosen systems" it says "ROMS have been scraped" instead of just going back to the main Skyscraper menu.

Yeah, that's a reminder from the original (scraper) script. I think I can fix it.

* In "Scrape from..." menu "Open Retro" should be "OpenRetro".

OK, will fix.

* For "Remove bracket info", when it's "Disabled", the B in bracket suddenly becomes uppercase.

That's a leftover from the UI experiment, I'll fix it.

For help texts:

* For "Scrape from..." help text it says "Open Retro", which should be "OpenRetro".

OK, I'll change it.

* For "Scrape from..." help text it would probably make sense to add a small help text saying something like "Some sources need a  or user:password to work, which can be set per source in config.ini" or similar. Otherwise I foresee a lot of support requests complaining about certain sources not working.

Yes, that's a good idea, I'll add this.

* For "Purge system" is says "Skyscraper parameter: --purgedb ". This is not correct. The '--purgedb' option can't purge a system. It can only purge certain types from certain modules. The "Skyscraper parameter" line could probably just be removed. I assume it currently just removes the corresponding folders without actually instantiating Skyscraper right now.

OK, I'll fix it.

* For "Download videos" it should probably say "Toggle the download and caching of videos when scraping. This also toggles whether the videos will be included when generating the final game list." or similar.

* For all of the "Cache " options the help should be "Toggle whether  are cached locally when scraping".

OK, I'll fix it.

I think that is all I have for now.

On whether more options should be supported:

* I think we should leave '--unpack' out for now, as it is a bit of an experimental feature and requires "7z" to be installed.

* '--region' would be useful, although it is only supported by the "ScreenScraper" source right now (and should probably say so in the help text). But I think it could make sense to add it with "eu" (default), "us" and "jp" regions. It will always try all regions regardless, but the one chose is the one it will prioritize highest. If '--region' is left out, "eu" is the region Skyscraper prioritizes highest by default, then the rest.

* '--lang' could also be implemented, but I don't have a good grasp on how much people use this. Personally I always leave it out to just use the default "en", but some users might want "es", "de" or others. This relates mostly, if not only, to the texts. Maybe also screenshots, I can't remember.

OK, I'll leave those aside for now. The scriptmodule also installs p7zip, so users can use --unpack from the CLI.

Once the module is fine, the hard part will begin - writing the wiki documentation :). I'll get back after adding the corrections, thanks for the feedback!

cmitu commented Oct 6, 2018

Currently testing the ui:

* When choosing "Cancel" in "Scrape chosen systems" it says "ROMS have been scraped" instead of just going back to the main Skyscraper menu.

Yeah, that's a reminder from the original (scraper) script. I think I can fix it.

* In "Scrape from..." menu "Open Retro" should be "OpenRetro".

OK, will fix.

* For "Remove bracket info", when it's "Disabled", the B in bracket suddenly becomes uppercase.

That's a leftover from the UI experiment, I'll fix it.

For help texts:

* For "Scrape from..." help text it says "Open Retro", which should be "OpenRetro".

OK, I'll change it.

* For "Scrape from..." help text it would probably make sense to add a small help text saying something like "Some sources need a  or user:password to work, which can be set per source in config.ini" or similar. Otherwise I foresee a lot of support requests complaining about certain sources not working.

Yes, that's a good idea, I'll add this.

* For "Purge system" is says "Skyscraper parameter: --purgedb ". This is not correct. The '--purgedb' option can't purge a system. It can only purge certain types from certain modules. The "Skyscraper parameter" line could probably just be removed. I assume it currently just removes the corresponding folders without actually instantiating Skyscraper right now.

OK, I'll fix it.

* For "Download videos" it should probably say "Toggle the download and caching of videos when scraping. This also toggles whether the videos will be included when generating the final game list." or similar.

* For all of the "Cache " options the help should be "Toggle whether  are cached locally when scraping".

OK, I'll fix it.

I think that is all I have for now.

On whether more options should be supported:

* I think we should leave '--unpack' out for now, as it is a bit of an experimental feature and requires "7z" to be installed.

* '--region' would be useful, although it is only supported by the "ScreenScraper" source right now (and should probably say so in the help text). But I think it could make sense to add it with "eu" (default), "us" and "jp" regions. It will always try all regions regardless, but the one chose is the one it will prioritize highest. If '--region' is left out, "eu" is the region Skyscraper prioritizes highest by default, then the rest.

* '--lang' could also be implemented, but I don't have a good grasp on how much people use this. Personally I always leave it out to just use the default "en", but some users might want "es", "de" or others. This relates mostly, if not only, to the texts. Maybe also screenshots, I can't remember.

OK, I'll leave those aside for now. The scriptmodule also installs p7zip, so users can use --unpack from the CLI.

Once the module is fine, the hard part will begin - writing the wiki documentation :). I'll get back after adding the corrections, thanks for the feedback!

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 6, 2018

I just had a look at the command that's executed when it scrapes. It add both '--unattend' and '--unattendskip'. It should by default only use "/opt/retropie/supplementary/skyscraper/Skyscraper --unattend" and then the rest. '--unattendskip' and '--unattend' are mutually exclusive and '--unattend' should be default. I'll let you know if I stumble upon other stuff.

muldjord commented Oct 6, 2018

I just had a look at the command that's executed when it scrapes. It add both '--unattend' and '--unattendskip'. It should by default only use "/opt/retropie/supplementary/skyscraper/Skyscraper --unattend" and then the rest. '--unattendskip' and '--unattend' are mutually exclusive and '--unattend' should be default. I'll let you know if I stumble upon other stuff.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

OK, I've added the last modifications into the script

  • Removed --unattendSkip from the default run options.
  • Added an extra note in the Sources select help option for the user/password per source being needed.
  • Modified the help message for the video download toggle and cache toggles to make them uniform and added an extra info for the videos. Removed the erroneous Skyscraper parameter from the system purge menu option.
  • In the Scrape chosen systems dialogue, choosing Cancel will go right back to the main menu instead of showing the ROMs have been scraped message.
  • Fixed the bracket info toggle casing when disabled was chosen.

cmitu commented Oct 7, 2018

OK, I've added the last modifications into the script

  • Removed --unattendSkip from the default run options.
  • Added an extra note in the Sources select help option for the user/password per source being needed.
  • Modified the help message for the video download toggle and cache toggles to make them uniform and added an extra info for the videos. Removed the erroneous Skyscraper parameter from the system purge menu option.
  • In the Scrape chosen systems dialogue, choosing Cancel will go right back to the main menu instead of showing the ROMs have been scraped message.
  • Fixed the bracket info toggle casing when disabled was chosen.
@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 7, 2018

Just checked it out, looks good!

Also checked the installation, and it seems to just clone the "master" branch, is that correct? That needs to use the method I use in update_skyscraper.sh instead, so it gets the latest release instead. Otherwise I can't guarantee that it works. My master branch is, mostly, working. But it also contains experimental features that aren't well-tested yet.

Instead of cloning master, you can do:

LATEST=`wget -q -O - "https://api.github.com/repos/muldjord/skyscraper/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/'`
wget -N https://github.com/muldjord/skyscraper/archive/${LATEST}.tar.gz
[create directory to unpack to]
tar xvzf ${LATEST}.tar.gz --strip-components 1 --overwrite [path to unpack to I believe should be here]
... and so on.

That will grab the latest release and unpack it to where you need it to be.

UPDATE:
The install script stalled on my Pi 3b+ at:

= = = = = = = = = = = = = = = = = = = = =
Configuring 'skyscraper' : Scraper for EmulationStation by Lars Muldjord
= = = = = = = = = = = = = = = = = = = = =

Nothing happens after that. I've let it sit for 5 minutes now.

muldjord commented Oct 7, 2018

Just checked it out, looks good!

Also checked the installation, and it seems to just clone the "master" branch, is that correct? That needs to use the method I use in update_skyscraper.sh instead, so it gets the latest release instead. Otherwise I can't guarantee that it works. My master branch is, mostly, working. But it also contains experimental features that aren't well-tested yet.

Instead of cloning master, you can do:

LATEST=`wget -q -O - "https://api.github.com/repos/muldjord/skyscraper/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/'`
wget -N https://github.com/muldjord/skyscraper/archive/${LATEST}.tar.gz
[create directory to unpack to]
tar xvzf ${LATEST}.tar.gz --strip-components 1 --overwrite [path to unpack to I believe should be here]
... and so on.

That will grab the latest release and unpack it to where you need it to be.

UPDATE:
The install script stalled on my Pi 3b+ at:

= = = = = = = = = = = = = = = = = = = = =
Configuring 'skyscraper' : Scraper for EmulationStation by Lars Muldjord
= = = = = = = = = = = = = = = = = = = = =

Nothing happens after that. I've let it sit for 5 minutes now.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

OK, then.
@joolswills - can you also take a look to spot any problems ?

cmitu commented Oct 7, 2018

OK, then.
@joolswills - can you also take a look to spot any problems ?

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

[.]..]

Instead of cloning master, you can do:

LATEST=`wget -q -O - "https://api.github.com/repos/muldjord/skyscraper/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/'`
wget -N https://github.com/muldjord/skyscraper/archive/${LATEST}.tar.gz
[create directory to unpack to]
...
That will grab the latest release and unpack it to where you need it to be.

I see. We can do that. The script already has a latest_ver_skyscraper function that gets the latest release tag (using wget and doing something similar to what you described) and RetroPie's git functions can download a tagged release instead of using the master branch.

UPDATE:
The install script stalled on my Pi 3b+ at:

= = = = = = = = = = = = = = = = = = = = =
Configuring 'skyscraper' : Scraper for EmulationStation by Lars Muldjord
= = = = = = = = = = = = = = = = = = = = =

Nothing happens after that. I've let it sit for 5 minutes now.

I'll take a look at it, it could be because of the latest changes I did to inspect the skyscraper.pro file.

cmitu commented Oct 7, 2018

[.]..]

Instead of cloning master, you can do:

LATEST=`wget -q -O - "https://api.github.com/repos/muldjord/skyscraper/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/'`
wget -N https://github.com/muldjord/skyscraper/archive/${LATEST}.tar.gz
[create directory to unpack to]
...
That will grab the latest release and unpack it to where you need it to be.

I see. We can do that. The script already has a latest_ver_skyscraper function that gets the latest release tag (using wget and doing something similar to what you described) and RetroPie's git functions can download a tagged release instead of using the master branch.

UPDATE:
The install script stalled on my Pi 3b+ at:

= = = = = = = = = = = = = = = = = = = = =
Configuring 'skyscraper' : Scraper for EmulationStation by Lars Muldjord
= = = = = = = = = = = = = = = = = = = = =

Nothing happens after that. I've let it sit for 5 minutes now.

I'll take a look at it, it could be because of the latest changes I did to inspect the skyscraper.pro file.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

@muldjord I've modified the install to get the latest tagged release (which now it seems to be 2.7.5), but I cannot reproduce the problem you're seeing during configuration. Are you installing from scratch or over an existing installation ?
You can debug the configuration by running:

sudo __debug=1 ~/RetroPie-Setup/retropie_packages.sh skyscraper configure

cmitu commented Oct 7, 2018

@muldjord I've modified the install to get the latest tagged release (which now it seems to be 2.7.5), but I cannot reproduce the problem you're seeing during configuration. Are you installing from scratch or over an existing installation ?
You can debug the configuration by running:

sudo __debug=1 ~/RetroPie-Setup/retropie_packages.sh skyscraper configure
@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 7, 2018

I know why it stalls now. It stalls at:

+ cp -a /home/pi/.skyscraper/. /opt/retropie/configs/all/skyscraper/

Which is a problem. It tries to copy everything from .skyscraper to the configs dir. If users already have a substantial cache in there, it will copy it entirely. In my case, it was copying 14 GB of cached data. What is that step supposed to do? Can it be made to only copy needed files? Also, if any files needs to be in /opt/retropie/configs/all/skyscraper/ they should probably be symlinked instead of copied, so the authoritative ones are in .skyscraper, which is where Skyscraper looks for them. Thoughts on this?

muldjord commented Oct 7, 2018

I know why it stalls now. It stalls at:

+ cp -a /home/pi/.skyscraper/. /opt/retropie/configs/all/skyscraper/

Which is a problem. It tries to copy everything from .skyscraper to the configs dir. If users already have a substantial cache in there, it will copy it entirely. In my case, it was copying 14 GB of cached data. What is that step supposed to do? Can it be made to only copy needed files? Also, if any files needs to be in /opt/retropie/configs/all/skyscraper/ they should probably be symlinked instead of copied, so the authoritative ones are in .skyscraper, which is where Skyscraper looks for them. Thoughts on this?

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

Yeah, I was afraid of that. As I said at the beginning

When installing through the scriptmodule, the existing configuration folder is preserved; it's also linked to /opt/retropie/configs/all/skyscraper as for any other module. [..]

The scriptmodule symlinks the ~/.skyscraper to /opt/retropie/configs/all/skyscraper and preserves the contents. This happens also for ES (~/.emulationstation is a symlink also) or any other module that stores data under ~/ dot-folders / or ~/.config.
It's perfectly fine, it's just that for Skyscraper existing installations which have a lot of data in the Local DB this takes some time.

Hm, I could probably work around it by moving the DB out of the way and then moving it back (after the symlink is done). The only problem when this might fail is when /opt/ doesn't have enough space (i.e. mounted on a separate partition - but this would most likely happen for a PC install, not for a PI).

cmitu commented Oct 7, 2018

Yeah, I was afraid of that. As I said at the beginning

When installing through the scriptmodule, the existing configuration folder is preserved; it's also linked to /opt/retropie/configs/all/skyscraper as for any other module. [..]

The scriptmodule symlinks the ~/.skyscraper to /opt/retropie/configs/all/skyscraper and preserves the contents. This happens also for ES (~/.emulationstation is a symlink also) or any other module that stores data under ~/ dot-folders / or ~/.config.
It's perfectly fine, it's just that for Skyscraper existing installations which have a lot of data in the Local DB this takes some time.

Hm, I could probably work around it by moving the DB out of the way and then moving it back (after the symlink is done). The only problem when this might fail is when /opt/ doesn't have enough space (i.e. mounted on a separate partition - but this would most likely happen for a PC install, not for a PI).

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 7, 2018

But it doesn't seem to be symlinking, it's actually copying the data with "cp -a" which is a physical copy. A symlinking would be near-instant and doesn't take up more space. Can this be set to symlink instead?

muldjord commented Oct 7, 2018

But it doesn't seem to be symlinking, it's actually copying the data with "cp -a" which is a physical copy. A symlinking would be near-instant and doesn't take up more space. Can this be set to symlink instead?

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

But it doesn't seem to be symlinking, it's actually copying the data with "cp -a" which is a physical copy. A symlinking would be near-instant and doesn't take up more space.

No, actually the symlink is the other way around. It could probably be faster if using mv, but it probably tries to be safe and does the copying first.

cmitu commented Oct 7, 2018

But it doesn't seem to be symlinking, it's actually copying the data with "cp -a" which is a physical copy. A symlinking would be near-instant and doesn't take up more space.

No, actually the symlink is the other way around. It could probably be faster if using mv, but it probably tries to be safe and does the copying first.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 7, 2018

Oh... Then that will be an issue. I foresee plenty of current users running into trouble here. Not only will they potentially run out of disk space before the copy is done, but they will also be wondering why it got stuck without a message about what it's doing.

muldjord commented Oct 7, 2018

Oh... Then that will be an issue. I foresee plenty of current users running into trouble here. Not only will they potentially run out of disk space before the copy is done, but they will also be wondering why it got stuck without a message about what it's doing.

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 7, 2018

Can we perhaps check for a current ./skyscraper/dbs and ./skyscraper/import folders when installing it, and make it move the folders during the installation instead? Then it would go on to configuring later, and then the problematic folders are already moved. That would probably work.

muldjord commented Oct 7, 2018

Can we perhaps check for a current ./skyscraper/dbs and ./skyscraper/import folders when installing it, and make it move the folders during the installation instead? Then it would go on to configuring later, and then the problematic folders are already moved. That would probably work.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 7, 2018

Oh... Then that will be an issue.

Could be, depending on the size of the Local DB. I'm just using a standard RetroPie setup helper here.

Can we perhaps check for a current ./skyscraper/dbs folder when installing it, and make it move the folder during the installation instead?

Yes, that's what I was suggesting above in #2497 (comment). I'll think of a way to safely do that - space wouldn't be an issue on a standard RetroPie install, only on a PC, where the user might have /opt mounted separately than /home.

cmitu commented Oct 7, 2018

Oh... Then that will be an issue.

Could be, depending on the size of the Local DB. I'm just using a standard RetroPie setup helper here.

Can we perhaps check for a current ./skyscraper/dbs folder when installing it, and make it move the folder during the installation instead?

Yes, that's what I was suggesting above in #2497 (comment). I'll think of a way to safely do that - space wouldn't be an issue on a standard RetroPie install, only on a PC, where the user might have /opt mounted separately than /home.

@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Oct 9, 2018

Member

Think i've just used shellcheck in the past occasionally, but we don't follow all it's recommendations (and it's limited as you have noted).

Member

joolswills commented Oct 9, 2018

Think i've just used shellcheck in the past occasionally, but we don't follow all it's recommendations (and it's limited as you have noted).

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 10, 2018

@joolswills I've modified the script after the review, mainly

  • added local to the vars that had it missing.
  • renamed internal functions and prefixed them with _.
  • modified the function style to remove extra lines at the beginning/end
  • minimized the use of --colors on the GUI's front menus and left any formatting on the Help pages.

I've also changed the purge functions to take into account what @muldjord mentioned about the keeping the conf file when deleting a system's cache.

cmitu commented Oct 10, 2018

@joolswills I've modified the script after the review, mainly

  • added local to the vars that had it missing.
  • renamed internal functions and prefixed them with _.
  • modified the function style to remove extra lines at the beginning/end
  • minimized the use of --colors on the GUI's front menus and left any formatting on the Help pages.

I've also changed the purge functions to take into account what @muldjord mentioned about the keeping the conf file when deleting a system's cache.

@hhromic

This comment has been minimized.

Show comment
Hide comment
@hhromic

hhromic Oct 10, 2018

Contributor

Hi guys, sorry to drop-in :)
I noticed that the she-bang is set to #!/usr/bin/env bash -x, and out of curiosity I went to check what -x is for. Turns out is for enabling debug mode in bash (every day you learn something new).
But, probably it should be taken out for the final version of the scriptmodule? :)

Contributor

hhromic commented Oct 10, 2018

Hi guys, sorry to drop-in :)
I noticed that the she-bang is set to #!/usr/bin/env bash -x, and out of curiosity I went to check what -x is for. Turns out is for enabling debug mode in bash (every day you learn something new).
But, probably it should be taken out for the final version of the scriptmodule? :)

popd
}
# Purge by cached platform

This comment has been minimized.

@joolswills

joolswills Oct 10, 2018

Member

white space above in function.

no gap between functions.

@joolswills

joolswills Oct 10, 2018

Member

white space above in function.

no gap between functions.

This comment has been minimized.

@cmitu

cmitu Oct 10, 2018

OK, fixed.

@cmitu

cmitu Oct 10, 2018

OK, fixed.

if [[ "$md_mode" == "remove" ]]; then
_purge_skyscraper
return
fi

This comment has been minimized.

@joolswills

joolswills Oct 10, 2018

Member

the purge function should probably be a remove_ function

@joolswills

joolswills Oct 10, 2018

Member

the purge function should probably be a remove_ function

This comment has been minimized.

@cmitu

cmitu Oct 10, 2018

Initially it was, but I thought to spare an extra function definition and add it to the configure's remove call. But it's clearer if moved to a separate function - I added the remove_ back.

@cmitu

cmitu Oct 10, 2018

Initially it was, but I thought to spare an extra function definition and add it to the configure's remove call. But it's clearer if moved to a separate function - I added the remove_ back.

f_size=$(du --total -sm "$home/.skyscraper/dbs" "$home/.skyscraper/import" 2>/dev/null | grep total | cut -f 1 )
printMsgs "console" "INFO: Moving the Local DB and Import folders to new configuration folder (total: $f_size Mb)"
for folder in dbs import; do

This comment has been minimized.

@joolswills

joolswills Oct 10, 2018

Member

no local.

@joolswills

joolswills Oct 10, 2018

Member

no local.

@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Oct 10, 2018

Member

made a few more comments - please go over the whitespace in the file. eg I found a function with no gap from the one above. In some functions you have more lines than needed in places with 2 line gaps etc - looks inconsistent.

Member

joolswills commented Oct 10, 2018

made a few more comments - please go over the whitespace in the file. eg I found a function with no gap from the one above. In some functions you have more lines than needed in places with 2 line gaps etc - looks inconsistent.

rp_module_section="opt"
function depends_skyscraper() {
getDepends qt5-default p7zip-full

This comment has been minimized.

@joolswills

joolswills Oct 10, 2018

Member

What is p7zip-full needed for ?

@joolswills

joolswills Oct 10, 2018

Member

What is p7zip-full needed for ?

This comment has been minimized.

@cmitu

cmitu Oct 10, 2018

It's used by the --unpack parameter to unzip/unpack any 7z packed ROMs and hash them. See
https://github.com/muldjord/skyscraper/blob/24ed2abb1639864dc5343ca6065865f492210694/src/skyscraper.cpp#L133

@cmitu

cmitu Oct 10, 2018

It's used by the --unpack parameter to unzip/unpack any 7z packed ROMs and hash them. See
https://github.com/muldjord/skyscraper/blob/24ed2abb1639864dc5343ca6065865f492210694/src/skyscraper.cpp#L133

This comment has been minimized.

@joolswills

joolswills Oct 10, 2018

Member

ok thanks.

@joolswills

joolswills Oct 10, 2018

Member

ok thanks.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 10, 2018

made a few more comments - please go over the whitespace in the file. eg I found a function with no gap from the one above. In some functions you have more lines than needed in places with 2 line gaps etc - looks inconsistent.

I'll check over the whitespace and then commit the latest changes.

cmitu commented Oct 10, 2018

made a few more comments - please go over the whitespace in the file. eg I found a function with no gap from the one above. In some functions you have more lines than needed in places with 2 line gaps etc - looks inconsistent.

I'll check over the whitespace and then commit the latest changes.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 10, 2018

OK, did a whitespace (spurious newlines and a few 3 space indents) cleanup and seems ok.

cmitu commented Oct 10, 2018

OK, did a whitespace (spurious newlines and a few 3 space indents) cleanup and seems ok.

@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Oct 11, 2018

Member

Thanks for the changes. If you are happy, please can you squash the commits, and I will do a final quick check and merge. Appreciate the contribution - it's good to have another scraper implemented (especially with the detail you have given the interface).

Member

joolswills commented Oct 11, 2018

Thanks for the changes. If you are happy, please can you squash the commits, and I will do a final quick check and merge. Appreciate the contribution - it's good to have another scraper implemented (especially with the detail you have given the interface).

# If we don't have a previous config.ini file, copy the dist one
[[ ! -f "$md_conf_dir/config.ini" ]] && cp "$md_conf_dir/config.ini.rp-dist" "$md_conf_dir/config.ini"

This comment has been minimized.

@joolswills

joolswills Oct 11, 2018

Member

Might it be worth using copyDefaultConfig. It could be that this isn't exactly what you need, but with current retropie modules we only create a rp-dist if the standard config is changed from the default. This may be useful or not to you.

@joolswills

joolswills Oct 11, 2018

Member

Might it be worth using copyDefaultConfig. It could be that this isn't exactly what you need, but with current retropie modules we only create a rp-dist if the standard config is changed from the default. This may be useful or not to you.

This comment has been minimized.

@cmitu

cmitu Oct 11, 2018

The idea here was to generate a basic default for a new installation and to save it in case editing it will bungle the config. By default skyscraper has a config.ini.example but it can run without. I can change the suffix to keep the convention for .rp-dist.

@cmitu

cmitu Oct 11, 2018

The idea here was to generate a basic default for a new installation and to save it in case editing it will bungle the config. By default skyscraper has a config.ini.example but it can run without. I can change the suffix to keep the convention for .rp-dist.

This comment has been minimized.

@joolswills

joolswills Oct 11, 2018

Member

Happy to leave the decision to you. Just wanted to mention the function. Whatever you think is best in this case. Thanks

@joolswills

joolswills Oct 11, 2018

Member

Happy to leave the decision to you. Just wanted to mention the function. Whatever you think is best in this case. Thanks

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 11, 2018

I second that. Thank you very much for your work on this @cmitu, I look forward to seeing where this goes when people start using it.

muldjord commented Oct 11, 2018

I second that. Thank you very much for your work on this @cmitu, I look forward to seeing where this goes when people start using it.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 11, 2018

@joolswills added the modifications and squashed the commits. Module is in 'optional', should appear alongside the scraper module.

cmitu commented Oct 11, 2018

@joolswills added the modifications and squashed the commits. Module is in 'optional', should appear alongside the scraper module.

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 11, 2018

I second that. Thank you very much for your work on this @cmitu, I look forward to seeing where this goes when people start using it.

Yep, will be interesting. Unfortunately I won't have time until Sunday to do any additions to the wiki describing the Gui and usage, but once merged you should probably make a new topic in the forums for people to report problems for the module specifically - instead of cluttering the existing topic (which is already all time 4th most popular topic).

cmitu commented Oct 11, 2018

I second that. Thank you very much for your work on this @cmitu, I look forward to seeing where this goes when people start using it.

Yep, will be interesting. Unfortunately I won't have time until Sunday to do any additions to the wiki describing the Gui and usage, but once merged you should probably make a new topic in the forums for people to report problems for the module specifically - instead of cluttering the existing topic (which is already all time 4th most popular topic).

@muldjord

This comment has been minimized.

Show comment
Hide comment
@muldjord

muldjord Oct 11, 2018

Yes, you are right, it's probably fitting with a new topic. I will do that once it's merged. I also plan to announce it on reddit's RetroPie sub once the wiki page has been updated with usage information. Proper credit will be given both places of course. :)

muldjord commented Oct 11, 2018

Yes, you are right, it's probably fitting with a new topic. I will do that once it's merged. I also plan to announce it on reddit's RetroPie sub once the wiki page has been updated with usage information. Proper credit will be given both places of course. :)

@cmitu

This comment has been minimized.

Show comment
Hide comment
@cmitu

cmitu Oct 11, 2018

Proper credit will be given both places of course. :)

Eh [with Canadian accent], don't feel you need to do that (and accidentally closing the PR).

cmitu commented Oct 11, 2018

Proper credit will be given both places of course. :)

Eh [with Canadian accent], don't feel you need to do that (and accidentally closing the PR).

@cmitu cmitu closed this Oct 11, 2018

@cmitu cmitu reopened this Oct 11, 2018

@joolswills

This comment has been minimized.

Show comment
Hide comment
@joolswills

joolswills Oct 12, 2018

Member

Thanks again for this and going through all the tweaks. Appreciated.

Member

joolswills commented Oct 12, 2018

Thanks again for this and going through all the tweaks. Appreciated.

@joolswills joolswills merged commit d7b14e5 into RetroPie:master Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment