Skip to content

Prompt to reset settings when settings.json cannot be read #379

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.

Sign up for GitHub

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

Merged
hebasto merged 1 commit into from
Aug 6, 2021
+54 −3
Merged

Prompt to reset settings when settings.json cannot be read #379

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions src/qt/bitcoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,58 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans
QApplication::installTranslator(&translator);
}

static std::string JoinErrors(const std::vector<std::string>& errors)
{
return Join(errors, "\n", [](const std::string& error) { return "- " + error; });
}

static bool InitSettings()
{
if (!gArgs.GetSettingsPath()) {
return true; // Do nothing if settings file disabled.
}

std::vector<std::string> errors;
if (!gArgs.ReadSettingsFile(&errors)) {
bilingual_str error = _("Settings file could not be read");
InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));

QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
/*: Explanatory text shown on startup when the settings file cannot be read.
Prompts user to make a choice between resetting or aborting. */
messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));
Copy link
Copy Markdown
Contributor

@jarolrod jarolrod Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add a translator comment:

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index c72a5ecdd..bc9da65ab 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -161,6 +161,8 @@ static bool InitSettings()
         InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
 
         QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
+        /*: Explanatory text shown on startup when the settings file cannot be read.
+            Prompts user to make a choice between resetting or aborting. */
         messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));
         messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
         messagebox.setTextFormat(Qt::PlainText);

Copy link
Copy Markdown
Contributor Author

@ryanofsky ryanofsky Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #379 (comment)

could add a translator comment:

Added, thanks!

Copy link
Copy Markdown
Member

@luke-jr luke-jr Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the logic to actually reset the settings to defaults.

Copy link
Copy Markdown
Contributor Author

@ryanofsky ryanofsky Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the logic to actually reset the settings to defaults.

It happens implicitly because if reading the settings file fails, the file that is written on line 181 will be empty.

Copy link
Copy Markdown
Member

@luke-jr luke-jr Jul 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if it fails part way into reading the settings, won't the ones it already read remain?

Copy link
Copy Markdown
Contributor Author

@ryanofsky ryanofsky Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #379 (comment)

But if it fails part way into reading the settings, won't the ones it already read remain?

To my surprise, it is actually technically possible for this to happen! And it needs to be fixed outside the GUI code so I filed a bug in the main repo bitcoin/bitcoin#22638. Practically speaking, the scenario where this would happen is nearly impossible and if it did happen, the effect would just be settings kept instead of discarded. So I think there's no need for this PR to be held up by the bug, but it is a good catch!

messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
Copy link
Copy Markdown
Member

@hebasto hebasto Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, we use a detailed text to show untranslated error messages to users for convenient googling and referencing (see bitcoin/bitcoin#16224). Now it only contains a list of errors. Could an untranslated error message be added here as well?

Copy link
Copy Markdown
Contributor Author

@ryanofsky ryanofsky Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #379 (comment)

Usually, we use a detailed text to show untranslated error messages to users for convenient googling and referencing (see bitcoin/bitcoin#16224). Now it only contains a list of errors. Could an untranslated error message be added here as well?

Just to clarify what's being discussed, errors is untranslated, and the error messages are self contained, so this dialog is consistent with other dialogs in using translated text in the main section, and untranslated text in the details section. The inconsistency is just in the prefixing.

On the prefixing, I do think it would be reasonable for a followup PR to append BitcoinGUI::tr("Original message:") + "\n" + QString::fromStdString(error.original) if error.original != error.translated to be a consistent with ThreadSafeMessageBox but I think it would be better to do it in a separate PR because:

  • I think this logic should go in a common util function so it is not duplicated. Trying to do this and refactor the ThreadSafeMessageBox code would expand the scope of this PR and complicate it when I think this PR is already a strict improvement and it's been reviewed and tested by people.

  • In this specific place, just because of the fact that ReadSettings errors are fully self-describing, I think adding the extra text would actually make the UX slightly worse (more repetitive and confusing) so more discussion in a followup PR about how to have consistency and clarity in these dialogs at same time could be a good idea.

messagebox.setTextFormat(Qt::PlainText);
messagebox.setDefaultButton(QMessageBox::Reset);
switch (messagebox.exec()) {
case QMessageBox::Reset:
break;
case QMessageBox::Abort:
return false;
default:
assert(false);
}
}

errors.clear();
if (!gArgs.WriteSettingsFile(&errors)) {
bilingual_str error = _("Settings file could not be written");
InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));

QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok);
/*: Explanatory text shown on startup when the settings file could not be written.
Prompts user to check that we have the ability to write to the file.
Explains that the user has the option of running without a settings file.*/
messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings."));
Comment thread
ryanofsky marked this conversation as resolved.
Copy link
Copy Markdown
Member

@hebasto hebasto Aug 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: occured ==> occurred

Addressed in bitcoin/bitcoin#22653.

messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
messagebox.setTextFormat(Qt::PlainText);
messagebox.setDefaultButton(QMessageBox::Ok);
messagebox.exec();
return false;
}
return true;
}

/* qDebug() message handler --> debug.log */
void DebugMessageHandler(QtMsgType type, const QMessageLogContext& context, const QString &msg)
{
Expand Down Expand Up @@ -569,9 +621,8 @@ int GuiMain(int argc, char* argv[])
// Parse URIs on command line -- this can affect Params()
PaymentServer::ipcParseCommandLine(argc, argv);
#endif
if (!gArgs.InitSettings(error)) {
InitError(Untranslated(error));
QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error initializing settings: %1").arg(QString::fromStdString(error)));

if (!InitSettings()) {
return EXIT_FAILURE;
}

Expand Down