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

@@ -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;

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<QProcess> QProcessPtr;

api/logic/java/JavaInstallList.cpp

@@ -156,7 +156,7 @@ void JavaListLoadTask::javaCheckerFinished(QList<JavaCheckResult> 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());
 

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
+		case JavaCheckResult::Validity::Errored:
-		emit logLine(tr("Could not start java:"), MessageLevel::Error);
+		{
-		emit logLines(result.errorLog.split('\n'), MessageLevel::Error);
+			// Error message displayed if java can't start
-		emit logLine("\nCheck your MultiMC Java settings.", MessageLevel::MultiMC);
+			emit logLine(tr("Could not start java:"), MessageLevel::Error);
-		printSystemInfo(false, false);
+			emit logLines(result.errorLog.split('\n'), MessageLevel::Error);
-		emitFailed(tr("Could not start java!"));
+			emit logLine("\nCheck your MultiMC Java settings.", MessageLevel::MultiMC);
-	}
+			printSystemInfo(false, false);
-	else
+			emitFailed(tr("Could not start java!"));
-	{
+			return;
-		auto instance = m_parent->instance();
+		}
-		printJavaInfo(result.javaVersion.toString(), result.mojangPlatform);
+		case JavaCheckResult::Validity::ReturnedInvalidData:
-		instance->settings()->set("JavaVersion", result.javaVersion.toString());
+		{
-		instance->settings()->set("JavaArchitecture", result.mojangPlatform);
+			emit logLine(tr("Java checker returned some invalid data MultiMC doesn't understand:"), MessageLevel::Error);
-		instance->settings()->set("JavaTimestamp", m_javaUnixTime);
+			emit logLines(result.outLog.split('\n'), MessageLevel::Warning);
-		emitSucceeded();
+			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;
+		}
 	}
 }
 

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!<br />Platform reported: %1<br />Java version "
+	text += QObject::tr("Java test succeeded!<br />Platform reported: %1<br />Java version "
-			   "reported: %2<br />").arg(result.realPlatform, result.javaVersion.toString());
+		"reported: %2<br />").arg(result.realPlatform, result.javaVersion.toString());
 	if (result.errorLog.size())
 	{
 		auto htmlError = result.errorLog;
 		htmlError.replace('\n', "<br />");
-		text += tr("<br />Warnings:<br /><font color=\"orange\">%1</font>").arg(htmlError);
+		text += QObject::tr("<br />Warnings:<br /><font color=\"orange\">%1</font>").arg(htmlError);
 	}
-	CustomMessageBox::selectable(m_parent, tr("Java test success"), text,
+	CustomMessageBox::selectable(parent, QObject::tr("Java test success"), text, QMessageBox::Information)->show();
-								 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', "<br />");
-	text += tr("The specified java binary didn't work with the arguments you provided:<br />");
+	text += QObject::tr("The specified java binary didn't work with the arguments you provided:<br />");
 	text += QString("<font color=\"red\">%1</font>").arg(htmlError);
-	CustomMessageBox::selectable(m_parent, tr("Java test failure"), text, QMessageBox::Warning)
+	CustomMessageBox::selectable(parent, QObject::tr("Java test failure"), text, QMessageBox::Warning)->show();
-		->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.<br />You should use the auto-detect feature, "
 		"or set the path to the java executable.<br />");
-	CustomMessageBox::selectable(m_parent, tr("Java test failure"), text, QMessageBox::Warning)
+	CustomMessageBox::selectable(parent, QObject::tr("Java test failure"), text, QMessageBox::Warning)->show();
-		->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();
 }

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);

application/setupwizard/JavaWizardPage.cpp

@@ -7,8 +7,8 @@
 #include <QLabel>
 #include <QLineEdit>
 #include <QPushButton>
+#include <QToolButton>
 #include <widgets/VersionSelectWidget.h>
-#include <widgets/IconLabel.h>
 #include <FileSystem.h>
 
 #include <java/JavaInstall.h>
@@ -16,6 +16,7 @@
 #include <java/JavaUtils.h>
 #include <sys.h>
 #include <QFileDialog>
+#include <JavaCommon.h>
 
 
 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_javaStatusBtn = new QToolButton(this);
-	m_horizontalLayout->addWidget(m_javaStatusLabel);
+	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());
+	observedMinMemory = s->get("MinMemAlloc").toInt();
-	m_maxMemSpinBox->setValue(s->get("MaxMemAlloc").toInt());
+	observedMaxMemory = s->get("MaxMemAlloc").toInt();
-	m_permGenSpinBox->setValue(s->get("PermGen").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.<br />");
+			auto htmlError = m_result.errorLog;
+			if(!htmlError.isEmpty())
+			{
+				htmlError.replace('\n', "<br />");
+				text += QString("<font color=\"red\">%1</font>").arg(htmlError);
+			}
+			failed = true;
+			break;
+		}
+		case JavaStatus::ReturnedInvalidData:
+		{
+			text += QObject::tr("The specified java binary returned unexpected results:<br />");
+			auto htmlOut = m_result.outLog;
+			if(!htmlOut.isEmpty())
+			{
+				htmlOut.replace('\n', "<br />");
+				text += QString("<font color=\"red\">%1</font>").arg(htmlOut);
+			}
+			failed = true;
+			break;
+		}
+		case JavaStatus::Good:
+			text += QObject::tr("Java test succeeded!<br />Platform reported: %1<br />Java version "
+				"reported: %2<br />").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_javaStatusBtn->setIcon(badIcon);
-			m_javaStatusLabel->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);
+		case JavaCheckResult::Validity::Valid:
-	}
+		{
-	else
+			setJavaStatus(JavaStatus::Good);
-	{
+			break;
-		setJavaStatus(JavaStatus::Bad);
+		}
+		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));
 }

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
+		DoesNotExist,
-	} javaStatus;
+		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<JavaChecker> m_checker;
+	JavaCheckResult m_result;
 };