From c4ec6bc0f552fe7af693d52826c82f1e7db908de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Mr=C3=A1zek?= Date: Sat, 7 Jan 2017 06:52:09 +0100 Subject: [PATCH] NOISSUE polish the java setup wizard page * Added a button to check why Java failed * It will now avoid automatically scanning binaries that do not have 'java' in their filename * Fixed some crashes related to running too many Java checks (it only does one at a time now) * It can now distinguish between more Java failure states (not there at all, crashing, returning nonsense) * Changed '...' button to Browse button to match the wizard page subtitle * Changing minimum and maximum memory will no longer trigger a java check twice --- api/logic/java/JavaChecker.cpp | 7 +- api/logic/java/JavaChecker.h | 8 +- api/logic/java/JavaInstallList.cpp | 2 +- api/logic/java/launch/CheckJava.cpp | 46 ++++-- application/JavaCommon.cpp | 35 ++-- application/JavaCommon.h | 12 +- application/setupwizard/JavaWizardPage.cpp | 176 +++++++++++++++++---- application/setupwizard/JavaWizardPage.h | 18 ++- 8 files changed, 228 insertions(+), 76 deletions(-) diff --git a/api/logic/java/JavaChecker.cpp b/api/logic/java/JavaChecker.cpp index 54d552a9..daf375ad 100644 --- a/api/logic/java/JavaChecker.cpp +++ b/api/logic/java/JavaChecker.cpp @@ -80,13 +80,14 @@ void JavaChecker::finished(int exitcode, QProcess::ExitStatus status) result.id = m_id; } result.errorLog = m_stderr; + result.outLog = m_stdout; qDebug() << "STDOUT" << m_stdout; qWarning() << "STDERR" << m_stderr; qDebug() << "Java checker finished with status " << status << " exit code " << exitcode; if (status == QProcess::CrashExit || exitcode == 1) { - qDebug() << "Java checker failed!"; + result.validity = JavaCheckResult::Validity::Errored; emit checkFinished(result); return; } @@ -112,7 +113,7 @@ void JavaChecker::finished(int exitcode, QProcess::ExitStatus status) if(!results.contains("os.arch") || !results.contains("java.version") || !success) { - qDebug() << "Java checker failed - couldn't extract required information."; + result.validity = JavaCheckResult::Validity::ReturnedInvalidData; emit checkFinished(result); return; } @@ -122,7 +123,7 @@ void JavaChecker::finished(int exitcode, QProcess::ExitStatus status) bool is_64 = os_arch == "x86_64" || os_arch == "amd64"; - result.valid = true; + result.validity = JavaCheckResult::Validity::Valid; result.is_64bit = is_64; result.mojangPlatform = is_64 ? "64" : "32"; result.realPlatform = os_arch; diff --git a/api/logic/java/JavaChecker.h b/api/logic/java/JavaChecker.h index 650e7ce3..c6bd697c 100644 --- a/api/logic/java/JavaChecker.h +++ b/api/logic/java/JavaChecker.h @@ -15,10 +15,16 @@ struct MULTIMC_LOGIC_EXPORT JavaCheckResult QString mojangPlatform; QString realPlatform; JavaVersion javaVersion; + QString outLog; QString errorLog; - bool valid = false; bool is_64bit = false; int id; + enum class Validity + { + Errored, + ReturnedInvalidData, + Valid + } validity = Validity::Errored; }; typedef std::shared_ptr QProcessPtr; diff --git a/api/logic/java/JavaInstallList.cpp b/api/logic/java/JavaInstallList.cpp index c0729227..d9204ba3 100644 --- a/api/logic/java/JavaInstallList.cpp +++ b/api/logic/java/JavaInstallList.cpp @@ -156,7 +156,7 @@ void JavaListLoadTask::javaCheckerFinished(QList results) qDebug() << "Found the following valid Java installations:"; for(JavaCheckResult result : results) { - if(result.valid) + if(result.validity == JavaCheckResult::Validity::Valid) { JavaInstallPtr javaVersion(new JavaInstall()); diff --git a/api/logic/java/launch/CheckJava.cpp b/api/logic/java/launch/CheckJava.cpp index 717fad49..0dc10822 100644 --- a/api/logic/java/launch/CheckJava.cpp +++ b/api/logic/java/launch/CheckJava.cpp @@ -78,23 +78,37 @@ void CheckJava::executeTask() void CheckJava::checkJavaFinished(JavaCheckResult result) { - if (!result.valid) + switch (result.validity) { - // Error message displayed if java can't start - emit logLine(tr("Could not start java:"), MessageLevel::Error); - emit logLines(result.errorLog.split('\n'), MessageLevel::Error); - emit logLine("\nCheck your MultiMC Java settings.", MessageLevel::MultiMC); - printSystemInfo(false, false); - emitFailed(tr("Could not start java!")); - } - else - { - auto instance = m_parent->instance(); - printJavaInfo(result.javaVersion.toString(), result.mojangPlatform); - instance->settings()->set("JavaVersion", result.javaVersion.toString()); - instance->settings()->set("JavaArchitecture", result.mojangPlatform); - instance->settings()->set("JavaTimestamp", m_javaUnixTime); - emitSucceeded(); + case JavaCheckResult::Validity::Errored: + { + // Error message displayed if java can't start + emit logLine(tr("Could not start java:"), MessageLevel::Error); + emit logLines(result.errorLog.split('\n'), MessageLevel::Error); + emit logLine("\nCheck your MultiMC Java settings.", MessageLevel::MultiMC); + printSystemInfo(false, false); + emitFailed(tr("Could not start java!")); + return; + } + case JavaCheckResult::Validity::ReturnedInvalidData: + { + emit logLine(tr("Java checker returned some invalid data MultiMC doesn't understand:"), MessageLevel::Error); + emit logLines(result.outLog.split('\n'), MessageLevel::Warning); + emit logLine("\nMinecraft might not start properly.", MessageLevel::MultiMC); + printSystemInfo(false, false); + emitSucceeded(); + return; + } + case JavaCheckResult::Validity::Valid: + { + auto instance = m_parent->instance(); + printJavaInfo(result.javaVersion.toString(), result.mojangPlatform); + instance->settings()->set("JavaVersion", result.javaVersion.toString()); + instance->settings()->set("JavaArchitecture", result.mojangPlatform); + instance->settings()->set("JavaTimestamp", m_javaUnixTime); + emitSucceeded(); + return; + } } } diff --git a/application/JavaCommon.cpp b/application/JavaCommon.cpp index a8561f6d..bb7246f7 100644 --- a/application/JavaCommon.cpp +++ b/application/JavaCommon.cpp @@ -22,40 +22,37 @@ bool JavaCommon::checkJVMArgs(QString jvmargs, QWidget *parent) return true; } -void JavaCommon::TestCheck::javaWasOk(JavaCheckResult result) +void JavaCommon::javaWasOk(QWidget *parent, JavaCheckResult result) { QString text; - text += tr("Java test succeeded!
Platform reported: %1
Java version " - "reported: %2
").arg(result.realPlatform, result.javaVersion.toString()); + text += QObject::tr("Java test succeeded!
Platform reported: %1
Java version " + "reported: %2
").arg(result.realPlatform, result.javaVersion.toString()); if (result.errorLog.size()) { auto htmlError = result.errorLog; htmlError.replace('\n', "
"); - text += tr("
Warnings:
%1").arg(htmlError); + text += QObject::tr("
Warnings:
%1").arg(htmlError); } - CustomMessageBox::selectable(m_parent, tr("Java test success"), text, - QMessageBox::Information)->show(); + CustomMessageBox::selectable(parent, QObject::tr("Java test success"), text, QMessageBox::Information)->show(); } -void JavaCommon::TestCheck::javaArgsWereBad(JavaCheckResult result) +void JavaCommon::javaArgsWereBad(QWidget *parent, JavaCheckResult result) { auto htmlError = result.errorLog; QString text; htmlError.replace('\n', "
"); - text += tr("The specified java binary didn't work with the arguments you provided:
"); + text += QObject::tr("The specified java binary didn't work with the arguments you provided:
"); text += QString("%1").arg(htmlError); - CustomMessageBox::selectable(m_parent, tr("Java test failure"), text, QMessageBox::Warning) - ->show(); + CustomMessageBox::selectable(parent, QObject::tr("Java test failure"), text, QMessageBox::Warning)->show(); } -void JavaCommon::TestCheck::javaBinaryWasBad(JavaCheckResult result) +void JavaCommon::javaBinaryWasBad(QWidget *parent, JavaCheckResult result) { QString text; - text += tr( + text += QObject::tr( "The specified java binary didn't work.
You should use the auto-detect feature, " "or set the path to the java executable.
"); - CustomMessageBox::selectable(m_parent, tr("Java test failure"), text, QMessageBox::Warning) - ->show(); + CustomMessageBox::selectable(parent, QObject::tr("Java test failure"), text, QMessageBox::Warning)->show(); } void JavaCommon::TestCheck::run() @@ -74,9 +71,9 @@ void JavaCommon::TestCheck::run() void JavaCommon::TestCheck::checkFinished(JavaCheckResult result) { - if (!result.valid) + if (result.validity != JavaCheckResult::Validity::Valid) { - javaBinaryWasBad(result); + javaBinaryWasBad(m_parent, result); emit finished(); return; } @@ -96,12 +93,12 @@ void JavaCommon::TestCheck::checkFinished(JavaCheckResult result) void JavaCommon::TestCheck::checkFinishedWithArgs(JavaCheckResult result) { - if (result.valid) + if (result.validity == JavaCheckResult::Validity::Valid) { - javaWasOk(result); + javaWasOk(m_parent, result); emit finished(); return; } - javaArgsWereBad(result); + javaArgsWereBad(m_parent, result); emit finished(); } diff --git a/application/JavaCommon.h b/application/JavaCommon.h index b100c213..4e4cd633 100644 --- a/application/JavaCommon.h +++ b/application/JavaCommon.h @@ -10,6 +10,13 @@ namespace JavaCommon { bool checkJVMArgs(QString args, QWidget *parent); + // Show a dialog saying that the Java binary was not usable + void javaBinaryWasBad(QWidget *parent, JavaCheckResult result); + // Show a dialog saying that the Java binary was not usable because of bad options + void javaArgsWereBad(QWidget *parent, JavaCheckResult result); + // Show a dialog saying that the Java binary was usable + void javaWasOk(QWidget *parent, JavaCheckResult result); + class TestCheck : public QObject { Q_OBJECT @@ -25,11 +32,6 @@ namespace JavaCommon signals: void finished(); - private: - void javaBinaryWasBad(JavaCheckResult result); - void javaArgsWereBad(JavaCheckResult result); - void javaWasOk(JavaCheckResult result); - private slots: void checkFinished(JavaCheckResult result); void checkFinishedWithArgs(JavaCheckResult result); diff --git a/application/setupwizard/JavaWizardPage.cpp b/application/setupwizard/JavaWizardPage.cpp index 31b8ff71..a56fdffc 100644 --- a/application/setupwizard/JavaWizardPage.cpp +++ b/application/setupwizard/JavaWizardPage.cpp @@ -7,8 +7,8 @@ #include #include #include +#include #include -#include #include #include @@ -16,6 +16,7 @@ #include #include #include +#include JavaWizardPage::JavaWizardPage(QWidget *parent) @@ -33,7 +34,8 @@ JavaWizardPage::JavaWizardPage(QWidget *parent) connect(m_permGenSpinBox, SIGNAL(valueChanged(int)), this, SLOT(memoryValueChanged(int))); connect(m_versionWidget, &VersionSelectWidget::selectedVersionChanged, this, &JavaWizardPage::javaVersionSelected); connect(m_javaBrowseBtn, &QPushButton::clicked, this, &JavaWizardPage::on_javaBrowseBtn_clicked); - connect(m_javaPathTextBox, &QLineEdit::textEdited, this, &JavaWizardPage::checkJavaPath); + connect(m_javaPathTextBox, &QLineEdit::textEdited, this, &JavaWizardPage::javaPathEdited); + connect(m_javaStatusBtn, &QToolButton::clicked, this, &JavaWizardPage::on_javaStatusBtn_clicked); } void JavaWizardPage::setupUi() @@ -63,11 +65,11 @@ void JavaWizardPage::setupUi() m_javaBrowseBtn->setSizePolicy(sizePolicy2); m_javaBrowseBtn->setMaximumSize(QSize(28, 16777215)); */ - m_javaBrowseBtn->setText(QStringLiteral("...")); m_horizontalLayout->addWidget(m_javaBrowseBtn); - m_javaStatusLabel = new IconLabel(this, badIcon, QSize(16, 16)); - m_horizontalLayout->addWidget(m_javaStatusLabel); + m_javaStatusBtn = new QToolButton(this); + m_javaStatusBtn->setIcon(yellowIcon); + m_horizontalLayout->addWidget(m_javaStatusBtn); m_verticalLayout->addLayout(m_horizontalLayout); @@ -86,7 +88,7 @@ void JavaWizardPage::setupUi() m_minMemSpinBox->setMinimum(256); m_minMemSpinBox->setMaximum(m_availableMemory); m_minMemSpinBox->setSingleStep(128); - m_minMemSpinBox->setValue(256); + m_labelMinMem->setBuddy(m_minMemSpinBox); m_gridLayout_2->addWidget(m_minMemSpinBox, 0, 1, 1, 1); m_labelMaxMem = new QLabel(m_memoryGroupBox); @@ -99,7 +101,7 @@ void JavaWizardPage::setupUi() m_maxMemSpinBox->setMinimum(512); m_maxMemSpinBox->setMaximum(m_availableMemory); m_maxMemSpinBox->setSingleStep(128); - m_maxMemSpinBox->setValue(1024); + m_labelMaxMem->setBuddy(m_maxMemSpinBox); m_gridLayout_2->addWidget(m_maxMemSpinBox, 1, 1, 1, 1); m_labelPermGen = new QLabel(m_memoryGroupBox); @@ -114,7 +116,6 @@ void JavaWizardPage::setupUi() m_permGenSpinBox->setMinimum(64); m_permGenSpinBox->setMaximum(m_availableMemory); m_permGenSpinBox->setSingleStep(8); - m_permGenSpinBox->setValue(128); m_gridLayout_2->addWidget(m_permGenSpinBox, 2, 1, 1, 1); m_permGenSpinBox->setVisible(false); @@ -133,9 +134,12 @@ void JavaWizardPage::initializePage() m_versionWidget->initialize(); auto s = MMC->settings(); // Memory - m_minMemSpinBox->setValue(s->get("MinMemAlloc").toInt()); - m_maxMemSpinBox->setValue(s->get("MaxMemAlloc").toInt()); - m_permGenSpinBox->setValue(s->get("PermGen").toInt()); + observedMinMemory = s->get("MinMemAlloc").toInt(); + observedMaxMemory = s->get("MaxMemAlloc").toInt(); + observedPermGenMemory = s->get("PermGen").toInt(); + m_minMemSpinBox->setValue(observedMinMemory); + m_maxMemSpinBox->setValue(observedMaxMemory); + m_permGenSpinBox->setValue(observedPermGenMemory); } bool JavaWizardPage::validatePage() @@ -144,7 +148,10 @@ bool JavaWizardPage::validatePage() auto path = m_javaPathTextBox->text(); switch(javaStatus) { - case JavaStatus::Bad: + case JavaStatus::NotSet: + case JavaStatus::DoesNotExist: + case JavaStatus::DoesNotStart: + case JavaStatus::ReturnedInvalidData: { int button = CustomMessageBox::selectable( this, @@ -215,24 +222,40 @@ bool JavaWizardPage::wantsRefreshButton() void JavaWizardPage::memoryValueChanged(int) { + bool actuallyChanged = false; int min = m_minMemSpinBox->value(); int max = m_maxMemSpinBox->value(); + int permgen = m_permGenSpinBox->value(); QObject *obj = sender(); - if (obj == m_minMemSpinBox) + if (obj == m_minMemSpinBox && min != observedMinMemory) { + observedMinMemory = min; + actuallyChanged = true; if (min > max) { + observedMaxMemory = min; m_maxMemSpinBox->setValue(min); } } - else if (obj == m_maxMemSpinBox) + else if (obj == m_maxMemSpinBox && max != observedMaxMemory) { + observedMaxMemory = max; + actuallyChanged = true; if (min > max) { + observedMinMemory = max; m_minMemSpinBox->setValue(max); } } - checkJavaPath(m_javaPathTextBox->text()); + else if (obj == m_permGenSpinBox && permgen != observedPermGenMemory) + { + observedPermGenMemory = permgen; + actuallyChanged = true; + } + if(actuallyChanged) + { + checkJavaPath(m_javaPathTextBox->text()); + } } void JavaWizardPage::javaVersionSelected(BaseVersionPtr version) @@ -251,7 +274,13 @@ void JavaWizardPage::javaVersionSelected(BaseVersionPtr version) void JavaWizardPage::on_javaBrowseBtn_clicked() { - QString raw_path = QFileDialog::getOpenFileName(this, tr("Find Java executable")); + QString filter; +#if defined Q_OS_WIN32 + filter = "Java (javaw.exe)"; +#else + filter = "Java (java)"; +#endif + QString raw_path = QFileDialog::getOpenFileName(this, tr("Find Java executable"), QString(), filter); if(raw_path.isNull()) { return; @@ -261,30 +290,106 @@ void JavaWizardPage::on_javaBrowseBtn_clicked() checkJavaPath(cooked_path); } +void JavaWizardPage::on_javaStatusBtn_clicked() +{ + QString text; + bool failed = false; + switch(javaStatus) + { + case JavaStatus::NotSet: + checkJavaPath(m_javaPathTextBox->text()); + return; + case JavaStatus::DoesNotExist: + text += QObject::tr("The specified file either doesn't exist or is not a proper executable."); + failed = true; + break; + case JavaStatus::DoesNotStart: + { + text += QObject::tr("The specified java binary didn't start properly.
"); + auto htmlError = m_result.errorLog; + if(!htmlError.isEmpty()) + { + htmlError.replace('\n', "
"); + text += QString("%1").arg(htmlError); + } + failed = true; + break; + } + case JavaStatus::ReturnedInvalidData: + { + text += QObject::tr("The specified java binary returned unexpected results:
"); + auto htmlOut = m_result.outLog; + if(!htmlOut.isEmpty()) + { + htmlOut.replace('\n', "
"); + text += QString("%1").arg(htmlOut); + } + failed = true; + break; + } + case JavaStatus::Good: + text += QObject::tr("Java test succeeded!
Platform reported: %1
Java version " + "reported: %2
").arg(m_result.realPlatform, m_result.javaVersion.toString()); + break; + case JavaStatus::Pending: + // TODO: abort here? + return; + } + CustomMessageBox::selectable( + this, + failed ? QObject::tr("Java test success") : QObject::tr("Java test failure"), + text, + failed ? QMessageBox::Critical : QMessageBox::Information + )->show(); +} + void JavaWizardPage::setJavaStatus(JavaWizardPage::JavaStatus status) { javaStatus = status; switch(javaStatus) { case JavaStatus::Good: - m_javaStatusLabel->setIcon(goodIcon); + m_javaStatusBtn->setIcon(goodIcon); break; + case JavaStatus::NotSet: case JavaStatus::Pending: - m_javaStatusLabel->setIcon(yellowIcon); + m_javaStatusBtn->setIcon(yellowIcon); break; default: - case JavaStatus::Bad: - m_javaStatusLabel->setIcon(badIcon); + m_javaStatusBtn->setIcon(badIcon); break; } } +void JavaWizardPage::javaPathEdited(const QString& path) +{ + // only autocheck + auto realPath = FS::ResolveExecutable(path); + QFileInfo pathInfo(realPath); + if (pathInfo.baseName().toLower().contains("java")) + { + checkJavaPath(path); + } + else + { + if(!m_checker) + { + setJavaStatus(JavaStatus::NotSet); + } + } +} + void JavaWizardPage::checkJavaPath(const QString &path) { + if(m_checker) + { + queuedCheck = path; + return; + } auto realPath = FS::ResolveExecutable(path); if(realPath.isNull()) { - setJavaStatus(JavaStatus::Bad); + setJavaStatus(JavaStatus::DoesNotExist); return; } setJavaStatus(JavaStatus::Pending); @@ -302,15 +407,31 @@ void JavaWizardPage::checkJavaPath(const QString &path) void JavaWizardPage::checkFinished(JavaCheckResult result) { - if(result.valid) + m_result = result; + switch(result.validity) { - setJavaStatus(JavaStatus::Good); - } - else - { - setJavaStatus(JavaStatus::Bad); + case JavaCheckResult::Validity::Valid: + { + setJavaStatus(JavaStatus::Good); + break; + } + case JavaCheckResult::Validity::ReturnedInvalidData: + { + setJavaStatus(JavaStatus::ReturnedInvalidData); + break; + } + case JavaCheckResult::Validity::Errored: + { + setJavaStatus(JavaStatus::DoesNotStart); + break; + } } m_checker.reset(); + if(!queuedCheck.isNull()) + { + checkJavaPath(queuedCheck); + queuedCheck.clear(); + } } void JavaWizardPage::retranslate() @@ -324,4 +445,5 @@ void JavaWizardPage::retranslate() m_labelMaxMem->setText(QApplication::translate("JavaPage", "Maximum memory allocation:", Q_NULLPTR)); m_minMemSpinBox->setToolTip(QApplication::translate("JavaPage", "The amount of memory Minecraft is started with.", Q_NULLPTR)); m_permGenSpinBox->setToolTip(QApplication::translate("JavaPage", "The amount of memory available to store loaded Java classes.", Q_NULLPTR)); + m_javaBrowseBtn->setText(QApplication::translate("JavaPage", "Browse", Q_NULLPTR)); } diff --git a/application/setupwizard/JavaWizardPage.h b/application/setupwizard/JavaWizardPage.h index 5a61ff02..56a40453 100644 --- a/application/setupwizard/JavaWizardPage.h +++ b/application/setupwizard/JavaWizardPage.h @@ -15,7 +15,7 @@ class QHBoxLayout; class QGroupBox; class QGridLayout; class QLabel; -class IconLabel; +class QToolButton; class JavaWizardPage : public BaseWizardPage { @@ -35,15 +35,20 @@ public: enum class JavaStatus { + NotSet, Pending, Good, - Bad - } javaStatus; + DoesNotExist, + DoesNotStart, + ReturnedInvalidData + } javaStatus = JavaStatus::NotSet; protected slots: void memoryValueChanged(int); + void javaPathEdited(const QString &path); void javaVersionSelected(BaseVersionPtr version); void on_javaBrowseBtn_clicked(); + void on_javaStatusBtn_clicked(); void checkFinished(JavaCheckResult result); protected: /* methods */ @@ -58,7 +63,7 @@ private: /* data */ QLineEdit * m_javaPathTextBox = nullptr; QPushButton * m_javaBrowseBtn = nullptr; - IconLabel * m_javaStatusLabel = nullptr; + QToolButton * m_javaStatusBtn = nullptr; QHBoxLayout *m_horizontalLayout = nullptr; QGroupBox *m_memoryGroupBox = nullptr; @@ -73,7 +78,12 @@ private: /* data */ QIcon yellowIcon; QIcon badIcon; + int observedMinMemory = 0; + int observedMaxMemory = 0; + int observedPermGenMemory = 0; + QString queuedCheck; uint64_t m_availableMemory = 0ull; shared_qobject_ptr m_checker; + JavaCheckResult m_result; };