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

SDL CIA Installation #3113

Merged
merged 8 commits into from Nov 24, 2017

Conversation

Projects
None yet
8 participants
@shinyquagsire23
Copy link
Contributor

shinyquagsire23 commented Nov 14, 2017

This PR can be reviewed commit-by-commit.

This PR adds command-line CIA installs with the -i or --install= argument. Installation is done using the CIAContainer and CIAFile classes, CIAContainer to initially check if a CIA is valid, and CIAFile to handle the actual installation.

Example CIA installation command:
citra -i scummvm.cia


This change is Reviewable

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Nov 15, 2017

Just skimmed it, awesome! Wanted to bring up that a lot of that logic for CIA install in the SDL frontend looks like it should be something thats in core somewhere since its basically going to be duplicated for every frontend that wants to install a CIA file. Does anyone have any thoughts on this?

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Nov 15, 2017

Agreed, the Install function itself should be in core

#include "core/loader/loader.h"
#include "core/settings.h"

static void PrintHelp(const char* argv0) {
std::cout << "Usage: " << argv0
<< " [options] <filename>\n"
"-g, --gdbport=NUMBER Enable gdb stub on port NUMBER\n"
"-i, --install=FILE Installs a specified CIA file\n"

This comment has been minimized.

@mailwl

mailwl Nov 15, 2017

Contributor

there is no "=" it TS...

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Nov 15, 2017

Contributor

It seems to accept either --install scummvm.cia or --install=scummvm.cia, I think --something=somethingelse is pretty standard (was just going off --gdbport though, to be honest).

@BreadFish64

This comment has been minimized.

Copy link
Contributor

BreadFish64 commented Nov 15, 2017

can confirm it works

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:sdl-cia-install branch from e38e69e to 49cb6fa Nov 15, 2017

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:sdl-cia-install branch from 49cb6fa to 7bb1409 Nov 15, 2017


/**
* Installs a CIA file from a specified file path.
* @param path file path of the CIA file to install

This comment has been minimized.

@Subv

Subv Nov 15, 2017

Member

Missing parameter docs for the callback

* @returns bool whether the install was successful
*/
bool InstallCIA(const std::string& path,
const std::function<void(size_t, size_t)>&& update_callback = {});

This comment has been minimized.

@Subv

Subv Nov 15, 2017

Member

remove the const. Use nullptr instead of {}

Alias void(size_t, size_t) to something more meaningful like using ProgressCallback = void(size_t, size_t);

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:sdl-cia-install branch from 7bb1409 to 002e7a2 Nov 15, 2017


bool CIAFile::Close() const {
bool complete = true;
for (int i = 0; i < container.GetTitleMetadata().GetContentCount(); i++) {

This comment has been minimized.

@lioncash

lioncash Nov 15, 2017

Member

This is a signed/unsigned mismatch, GetContentCount() returns a size_t

@@ -89,6 +98,12 @@ int main(int argc, char** argv) {
exit(1);
}
break;
case 'i':
if (!Service::AM::InstallCIA(std::string(optarg), CIAProgress))

This comment has been minimized.

@MerryMage

MerryMage Nov 15, 2017

Member

Personally I prefer:

        case 'i': {
            const auto cia_progress = [](size_t written, size_t total) {
                LOG_INFO(Frontend, "%02zu%%", (written * 100 / total));
            };
            if (!Service::AM::InstallCIA(std::string(optarg), cia_progress))
                errno = EINVAL;
            if (errno != 0)
                exit(1);
            break;
        }

to limit the scope of CIAProgress.

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Nov 15, 2017

Contributor

Yeah that's way cleaner, thanks.

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:sdl-cia-install branch from 002e7a2 to 553ca2b Nov 15, 2017


bool SetSize(u64 size) const override {
bool InstallCIA(const std::string& path, std::function<ProgressCallback>&& update_callback) {

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 16, 2017

Contributor

Would it be possible to return a more detailed result than true/false so that GUI frontends can give the user a more specific error?

This comment has been minimized.

@shinyquagsire23
@Subv

Subv approved these changes Nov 24, 2017

@jroweboy jroweboy merged commit b7cf793 into citra-emu:master Nov 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment