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

Updater QT Integration #2997

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@j-selby
Copy link
Contributor

j-selby commented Oct 8, 2017

Blocked by citra-emu/ext-windows-bin#11, #2966. Related to citra-emu/citra-web#56.

Original implementation by @jroweboy. Based on https://github.com/Skycoder42/QtAutoUpdater.

This implements support for Qt integration into the main UI. The installer binary is searched for in the parent directory, labelled "maintenancetool[.exe]".

Autoupdating can be configured via Citra's configuration menu, as well as Help->Open Updater/Check for Updates. These settings/options are hidden when the installer binary does not exist.

Two separate updating mechanisms are implemented:

  • Check on startup (default). Popup is shown at startup which launches the main updater UI in updating mode if Yes is selected.
  • Silent updating. Updater binary is launched at shutdown, which silently updates Citra (if required).

Relatively untested on Linux, and untested on Macs.

UI Screenshots:

image
image
image (if menubar option manually clicked)
image


This change is Reviewable

return;
}

if (found) {

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

It's possible to unindent much of this by comparing the no update case first via

if (!found) {
    LOG_INFO(Frontend, "No updates found");
    return;
}

...
backend->tool_path = UpdaterPrivate::toSystemExe(maintenance_tool_path);
}

Updater::~Updater() {}

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member
Updater::~Updater() = default;
}
}

const QString UpdaterPrivate::toSystemExe(QString basePath) {

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

const isn't really necessary here. Also base_path

bool UpdaterPrivate::StartUpdateCheck() {
if (running || !HasUpdater()) {
return false;
} else {

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

This else can be omitted and the indentation reduced, since the if statement body contains a return.

}

void UpdaterPrivate::StopUpdateCheck(int delay, bool async) {
if (main_process && main_process->state() != QProcess::NotRunning) {

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member
if (!main_process || main_process->state() == QProcess::NotRunning) {
    return;
}

can be used to unindent the rest of the code.

Updater::Updater(QObject* parent) : Updater(DEFAULT_TOOL_PATH, parent) {}

Updater::Updater(const QString& maintenance_tool_path, QObject* parent)
: QObject(parent), backend(new UpdaterPrivate(this)) {

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

make_unique?

backend->LaunchUIOnExit();
}

Updater::UpdateInfo::UpdateInfo() : name(), version(), size(0ull) {}

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

You can just make the struct body like so to avoid the need for initializing these directly in the initializer list:

//! Provides information about updates for components
struct UpdateInfo {
    //! The name of the component that has an update
    QString name;
    //! The new version for that compontent
    QVersionNumber version;
    //! The update download size (in Bytes)
    quint64 size = 0;

    //! Default Constructor
    UpdateInfo();
    //! Copy Constructor
    UpdateInfo(const UpdateInfo& other);
    //! Constructor that takes name, version and size
    UpdateInfo(QString name, QVersionNumber version, quint64 size);
};

This can then be defaulted as:

Updater::UpdateInfo::UpdateInfo() = default;
}

bool UpdaterPrivate::HasUpdater() {
QFileInfo toolInfo(QCoreApplication::applicationDirPath(), tool_path);

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

tool_info

void SilentlyUpdate();

//! Checks to see if a updater application is available
bool HasUpdater();

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

As far as I'm aware, this can be a const member function.


static const QString toSystemExe(QString basePath);

bool HasUpdater();

This comment has been minimized.

@lioncash

lioncash Oct 8, 2017

Member

As far as I'm aware, this can be a const member function.

@B3n30
Copy link
Contributor

B3n30 left a comment

Mostly just coding style things. And the PR isn't as awful as promised :-P

</layout>
</widget>
</item>

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

Usually a ui file doesn't have blank lines like this

@@ -149,8 +178,7 @@
</widget>
</item>
<item>
<widget class="QComboBox" name="theme_combobox">
</widget>
<widget class="QComboBox" name="theme_combobox"/>

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

(Unintentional change?, but the change is fine)

@@ -345,6 +368,46 @@ void GMainWindow::OnDisplayTitleBars(bool show) {
}
}

void GMainWindow::OnCheckForUpdates() {
this->CheckForUpdates();

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

is this-> necessary here?

LOG_INFO(Frontend, "Update found!");
auto result = QMessageBox::question(
this, tr("Update available!"),
tr("A update has been found for Citra. Do you wish to install it now?<br /><br />"

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

Nit: "A update for citra is available. Do you wish to install it now?"
I somehow don't like to use found here

This comment has been minimized.

@Dragios

Dragios Oct 8, 2017

Contributor

It should be "An update".

@@ -45,7 +45,7 @@
<x>0</x>
<y>0</y>
<width>1081</width>
<height>19</height>
<height>21</height>

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

Nit: Unintentional change?

return XMLParseResult::Success;
}

void UpdaterPrivate::UpdaterReady(int exitCode, QProcess::ExitStatus exitStatus) {

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

exit_code, exit_status

normal_exit = true;
last_error_code = exitCode;
last_error_log = main_process->readAllStandardError();
const auto updateOut = main_process->readAllStandardOutput();

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

update_out

}

if (exitStatus != QProcess::NormalExit) {
UpdaterError(QProcess::Crashed);

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

return; ?


QList<Updater::UpdateInfo> update_info;
auto err = ParseResult(updateOut, update_info);
bool hasError = false;

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

has_error


class UpdaterPrivate;

//! The main updater. Can check for updates and run the maintenancetool as updater

This comment has been minimized.

@B3n30

B3n30 Oct 8, 2017

Contributor

//! isn't the comment style we use here: /// or //<

@B3n30

B3n30 approved these changes Oct 8, 2017

Copy link
Contributor

B3n30 left a comment

Ok, besides my little wish to add a popup for no available update if you "Check for Updates". LGT and all the dependencies MERGED...

Release the kraken!!! ahh... Canary!!!

@j-selby

This comment has been minimized.

Copy link
Contributor

j-selby commented Oct 8, 2017

From Discord:

wwylele / 白疾風 - Today at 8:31 PM
> a normal user isn't going to have loaded a game
cci/cxi file format is linked to citra on my machine, and I can double click the file to start it up in citra immediately
CitraBotBOT - Today at 8:32 PM
<neobrain> you're just adding UI verbosity to the update dialog for a scenario that could easily be avoided (e.g. by not allowing games to be started until update is ran through, by aborting the update automatically [because let's be honest is it going to be critical enough to annoy the user about it], etc)
Selby - Today at 8:33 PM
yeah, it could be suppressed, true
@RavenHome1

This comment has been minimized.

Copy link

RavenHome1 commented Oct 8, 2017

i am more supportive of keeping it simple

user opens citra -> citra checks for update -> if new update available then notify the user and ask the user if they want to update, else do nothing -> if user selects yes then citra updates and restarts and do what ever it needs to do.

if user selected no then continue running citra normally -> user can select check for updates from the help drop down menu and citra will check and ask the user to update.

if no update was found when the user selected the check for update option then citra notifies the user that there is no new update (basically @B3n30 idea above)

if emulation is already running then grey out the check for updates button and prevent any update operations until the user brings emulation to a full stop (basically check that no emulation is happening)

adding the mentioned open updater and silent run mode just creates superfluous and confusing options. (better leave the program to do most of the work than having a confused user)

also perhaps look into hiding maintenancetool[.exe] so the user won't run it out of curiosity and cause some problems (if there is any chance of that happening)

@B3n30

This comment has been minimized.

Copy link
Contributor

B3n30 commented Oct 8, 2017

@RavenHome1

  • Your suggestions are mainly what this PR does (except for my tiny popup wish and the greyed out menu during emulation).
  • But I still think a silent update mode is necessary and not to confusing.
  • Hiding the maintenancetool isn't necessary since it is basically the deinstaller and provides the option to install additional stuff. So it is fine if the user can see it/ use it. Thats to tool that gets opened when you select "Open Updater", which is now renamed to "Modify Citra Install". From here you can select if you want to use canary or nightly and might also be able to install custom system archives, ...
@RavenHome1

This comment has been minimized.

Copy link

RavenHome1 commented Oct 8, 2017

@B3n30

  • i guess i was not paying much attention, thanks for clearing that up

  • i still don't find a need for the silent updater, giving the user the option at citra startup is quite enough.

  • "the modify citra install" now seems much better when you cleared it up and named it a better name but will it still be needed in the future. will canary and nightly remain forever or will they be merged together at some point in a more stable single release?.

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Oct 8, 2017

  • we didn't choose the name of the maintenence tool. its part of the library for installing and updating
  • obtrusive popups on almost every launch is very annoying. Most "modern" applications like chrome/firefox have moved to a background update on close because its a better user experience.
@@ -207,4 +223,5 @@
</action>
</widget>
<resources/>
<connections/>

This comment has been minimized.

@B3n30

B3n30 Oct 28, 2017

Contributor

NIT: unintentional change, but I think it can stay

//
// * Redistributions in binary form must reproduce the above copyright notice,
// this list of conditions and the following disclaimer in the documentation
// and/or other materials provided with the distribution.

This comment has been minimized.

@B3n30

B3n30 Oct 28, 2017

Contributor

I don't think we have this anywhere in our license file, but we should

This comment has been minimized.

@j-selby

j-selby Oct 31, 2017

Contributor

Not sure how you think we should handle this.

/// Destroys the updater and kills the update check (if running)
~Updater();

/// Returns `true`, if the updater exited normally

This comment has been minimized.

@B3n30

B3n30 Oct 28, 2017

Contributor

The docstrings for functions in citra are usually

/**
 * returns true if the updater exited normally 
 */

Some for the other public functions

/// returns the error output (stderr) of the last update
QByteArray ErrorLog() const;

/// readAcFn{Updater::running}

This comment has been minimized.

@B3n30

B3n30 Oct 28, 2017

Contributor

What is readAcFn? Same for LatestUpdateInfo, RunningChanged and UpdateInfoChanged

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 1, 2017

As reported, this fails the frozen dependencies build because of the usage of Qt 5.4 overload of QTimer::singleShot (and probably more). Our frozen dependencies uses Qt 5.2 (= current minimal requirement to compile citra), so please consider using functions in this version, otherwise we need to bump the version requirement and document it.

@j-selby

This comment has been minimized.

Copy link
Contributor

j-selby commented Nov 1, 2017

Fair enough. I don't have the skills to do that kind of backporting (nor the time to fuss over the million other things wrong with this), so I will likely close this PR shortly.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 1, 2017

@j-selby hey I am not rejecting this. I just skimmed over the code, and found that it is pretty easy to fix QTimer::singleShot issue: you just need to make what is a lambda now an actual slot/member function and use the old version of singleShot to call it.

@jroweboy

This comment has been minimized.

Copy link
Member

jroweboy commented Nov 4, 2017

I fixed the qtimer issue and conflicts and manually pushed to master

@jroweboy jroweboy closed this Nov 4, 2017

@j-selby j-selby deleted the j-selby:updater branch Nov 25, 2017

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