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

QT CIA installation #3144

Merged
merged 1 commit into from Dec 3, 2017

Conversation

Projects
None yet
5 participants
@BreadFish64
Copy link
Contributor

BreadFish64 commented Nov 24, 2017

Adds QT CIA installation
image
image
image


This change is Reviewable


// update progress
connect(this, &GMainWindow::UpdateProgress, this, &GMainWindow::OnUpdateProgress);
watcher = new QFutureWatcher<Service::AM::InstallStatus>;

This comment has been minimized.

@MerryMage

MerryMage Nov 24, 2017

Member

Memory leak.

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 24, 2017

Contributor

fixed (I think)

ui.action_Install_CIA->setEnabled(false);

// update progress
connect(this, &GMainWindow::UpdateProgress, this, &GMainWindow::OnUpdateProgress);

This comment has been minimized.

@MerryMage

MerryMage Nov 24, 2017

Member

On repeated invocations of OnMenuInstallCIA, connect will be called multiple times, resulting in multiple calls to OnUpdateProgress on each signal emission.

On Qt 4.6 the lazy fix is to use Qt::UniqueConnection. The proper fix is to put this in the constructor.

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 24, 2017

Contributor

moved to ConnectWidgetEvents

}

void GMainWindow::OnCIAInstallFinished() {
this->statusBar()->removeWidget(progress_bar);

This comment has been minimized.

@MerryMage

MerryMage Nov 24, 2017

Member

Memory leak of progress_bar.

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 24, 2017

Contributor

fixed

@BreadFish64 BreadFish64 force-pushed the BreadFish64:CIAInstallUI branch from dc0b699 to fca9beb Nov 24, 2017

@B3n30

This comment has been minimized.

Copy link
Contributor

B3n30 commented Nov 25, 2017

This will most likely conflict with #3073. So be aware of that before adding it to canary or merging it

@B3n30
Copy link
Contributor

B3n30 left a comment

Overall looks fine, I'd like to test on my system before i give my approval, but expect this to be done by today

"<a href='https://community.citra-emu.org/t/how-to-upload-the-log-file/296'>How to "
"Upload the Log File</a>.<br/><br/>Would you like to quit back to the game list? "
"Continuing emulation may result in crashes, corrupted save data, or other bugs."),
"<a href='https://community.citra-emu.org/t/how-to-upload-the-log-file/296'>How "

This comment has been minimized.

@B3n30

B3n30 Nov 25, 2017

Contributor

Why is this changed?

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 25, 2017

Contributor

Good question... I suspect clang has something to do with it

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 25, 2017

Contributor

fixed

@@ -5,9 +5,12 @@
#pragma once

#include <memory>
#include <QFutureWatcher>

This comment has been minimized.

@B3n30

B3n30 Nov 25, 2017

Contributor

Can be forward declared. Same for QProgressBar ~~~and Service::AM::InstallStatus~~~

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 25, 2017

Contributor

fixed


// Status bar elements

This comment has been minimized.

@B3n30

B3n30 Nov 25, 2017

Contributor

remove the new line

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 25, 2017

Contributor

fixed

@@ -696,6 +708,61 @@ void GMainWindow::OnMenuSelectGameListRoot() {
}
}

void GMainWindow::OnMenuInstallCIA() {
QString filepath = QFileDialog::getOpenFileName(

This comment has been minimized.

@B3n30

B3n30 Nov 25, 2017

Contributor

It would be nice to have getOpenFileNames and be able to install multiple files at once. But I know it is a bit more difficult for error handling and the progress bar...
So maybe in the future(tm)?

This comment has been minimized.

@BreadFish64

BreadFish64 Nov 25, 2017

Contributor

Yeah, I specifically disable the button while installing the CIA to avoid that nightmare

@B3n30

B3n30 approved these changes Nov 25, 2017

class Config;
class QProgressBar;

This comment has been minimized.

@B3n30

B3n30 Nov 27, 2017

Contributor

Since the sorting here is alphabetical this should be below class QFutureWatcher

class GPUCommandListWidget;
class GPUCommandStreamWidget;

This comment has been minimized.

@B3n30

B3n30 Nov 27, 2017

Contributor

Why did you change this?

#include "ui_main.h"

class AboutDialog;

This comment has been minimized.

@B3n30

B3n30 Nov 27, 2017

Contributor

Unintentional Change? But it's fine

@Subv

Subv approved these changes Dec 2, 2017

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Dec 2, 2017

Please squash this

@BreadFish64 BreadFish64 force-pushed the BreadFish64:CIAInstallUI branch from 03e3728 to 7f8219f Dec 2, 2017

@BreadFish64 BreadFish64 force-pushed the BreadFish64:CIAInstallUI branch from 7f8219f to 80852f9 Dec 2, 2017

@wwylele wwylele merged commit 466bec2 into citra-emu:master Dec 3, 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