Reset options, notify user about backup creation

[furszy]

Quick follow-up to first point of #602 (review)

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

[Rspigler]

make check

make[4]: Leaving directory '/home/user/gui/src'
Running tests: addrman_tests from test/addrman_tests.cpp
/bin/bash: line 2: test/test_bitcoin: Permission denied
make[3]: *** [Makefile:20843: test/addrman_tests.cpp.test] Error 1
make[3]: Leaving directory '/home/user/gui/src'
make[2]: *** [Makefile:18948: check-am] Error 2
make[2]: Leaving directory '/home/user/gui/src'
make[1]: *** [Makefile:18605: check-recursive] Error 1
make[1]: Leaving directory '/home/user/gui/src'
make: *** [Makefile:818: check-recursive] Error 1

[furszy]

@Rspigler issue is on your environment, this PR has no link to it.

"Permission denied" from bash usually means that the binary is not executable. Try cleaning your environment first (git clean -dfx) and recompile the sources (./autogen.sh && ./configure && make check). (plus, if doesn't work, could try with chmod +x src/test/test_bitcoin).

[Rspigler]

tACK 52372cb
It writes settings.json.bak, but then continues to overwrite the file rather than create settings1.json.bak, settings2...., etc.

Tests do pass

[shaavan]

ACK 52372cb

• I agree with the idea of this PR. Displaying where the settings are getting backup lets the user know where to access them when needed.
• I successfully compiled this PR on Ubuntu 22.04 with Qt version 5.15.3.

Screenshot:

Master	PR

Observations:

• Verified that the settings are correctly backed up on the displayed location.
• The latest custom settings overwrite the settings.json.bak file. Hence leading to backing up only the latest set of settings.

[furszy]

thanks for the reviews!

It writes settings.json.bak, but then continues to overwrite the file rather than create settings1.json.bak, settings2...., etc.

Yeah, I didn't implement those changes here. Only implemented the GUI changes. Those are coming in a following-up PR, test included :)

[jarolrod]

Please add translator comments to these translatable strings. I've written some documentation on crafting translator comments here: https://github.com/bitcoin-core/gui-qml/blob/main/src/qml/doc/translator-comments.md

I can suggest the comments if you'd like

[ryanofsky]

re: #617 (comment)

Please add translator comments to these translatable strings. I've written some documentation on crafting translator comments here: https://github.com/bitcoin-core/gui-qml/blob/main/src/qml/doc/translator-comments.md

I can suggest the comments if you'd like

This seems worth following up on. It's not obvious to me what translator comments would be helpful here so maybe suggesting specific comments would be helpful

[ryanofsky]

In commit "gui: reset options, notify user about the backup creation" (52372cb)

I think this is mentioning the wrong backup directory. Should be gArgs.GetDataDirNet() not GUIUtil::getDefaultDataDirectory()

[furszy]

yeah, good catch.
but one thing, will better provide client model access so we don't access gArgs from the widget (at least in my head, the ideal architecture would be widget -> model -> interfaces -> backend).

[furszy]

Updated per ryanofsky feedback.

Now the flow uses m_client_model->dataDir() instead of GUIUtil::getDefaultDataDirectory().

[ryanofsky]

Code review ACK ac4fb3b, just fixing displayed backup directory since last review

[ryanofsky]

re: #617 (comment)

Please add translator comments to these translatable strings. I've written some documentation on crafting translator comments here: https://github.com/bitcoin-core/gui-qml/blob/main/src/qml/doc/translator-comments.md

I can suggest the comments if you'd like

This seems worth following up on. It's not obvious to me what translator comments would be helpful here so maybe suggesting specific comments would be helpful

[jarolrod]

@ryanofsky

This seems worth following up on. It's not obvious to me what translator comments would be helpful here so maybe suggesting specific comments would be helpful

suggesting the following translator comments:

diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
index 462b923d6..3cf165004 100644
--- a/src/qt/optionsdialog.cpp
+++ b/src/qt/optionsdialog.cpp
@@ -286,9 +286,17 @@ void OptionsDialog::on_resetButton_clicked()
 {
     if (model) {
         // confirmation dialog
+        //: Window title text of pop-up window shown when the user has chosen to reset options.
         QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
+            /*: Text explaining that the settings the user changed will not come
+                into effect until the client is restarted. */
             tr("Client restart required to activate changes.") + "<br><br>" +
+            /*: Text explaining to the user that the client's current settings
+                will be backed up at a specific location. %1 is a stand-in
+                argument for the backup location's path. */
             tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>" +
+            /*: Text asking the user to confirm if they would like to proceed
+                with a client shutdown. */
             tr("Client will be shut down. Do you want to proceed?"),
             QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);

But, there is a problem with this. The translator comment for the first tr(), being "Client restart required to activate changes", will pick up the translator comment; but the other two tr() will not. This is because, while the tr() are on their own line, they are being appended together and the Qt Translator doesn't seem to like this in respect to adding the appropriate extracomment field to the translator file.

I would suggest to just create a variable that will hold the text and then pass that to the dialog, like so:

@@ -278,14 +284,23 @@ void OptionsDialog::setOkButtonState(bool fState)

 void OptionsDialog::on_resetButton_clicked()
 {
-    if(model)
-    {
+    if (model) {
         // confirmation dialog
+        /*: Text explaining that the settings the user changed will not come
+            into effect until the client is restarted. */
+        QString reset_window_text = tr("Client restart required to activate changes.") + "<br><br>";
+        /*: Text explaining to the user that the client's current settings
+            will be backed up at a specific location. %1 is a stand-in
+            argument for the backup location's path. */
+        reset_window_text.append(tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>");
+        /*: Text asking the user to confirm if they would like to proceed
+            with a client shutdown. */
+        reset_window_text.append(tr("Client will be shut down. Do you want to proceed?"));
+        //: Window title text of pop-up window shown when the user has chosen to reset options.
         QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
-            tr("Client restart required to activate changes.") + "<br><br>" + tr("Client will be shut down. Do you want to proceed?"),
-            QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);
+            reset_window_text,QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);

To not prevent this from moving along, this could just be picked up as a follow-up.

[ryanofsky]

suggesting the following translator comments:

This is great. Thanks!

[furszy]

Oh, sorry @jarolrod. I missed your comment there. Thanks for the suggestion 👌🏼 .

I'm fine either way, could add it here under your authorship or you could create a follow-up PR for it.

---

Have to say that mixing source code written in c++ with comments for translators isn't convincing me. There should be another way, some external file/s where we could map every text being translated to its own translator comment so we don't end up adding external stuff to the sources (if something like this doesn't exist then maybe we should create it).

[jarolrod]

@furszy

Have to say that mixing source code written in c++ with comments for translators isn't convincing me. There should be another way, some external file/s where we could map every text being translated to its own translator comment so we don't end up adding external stuff to the sources (if something like this doesn't exist then maybe we should create it).

this is nothing new, we just follow Qt's process for writing source code for translation. We've chosen to provide context to translators to help them with translating from english to a target language, and to do this we need to provide a translator comment. I am not aware of any other way to provide translator context and nicely integrate it with transifex. Custom tooling seems like an unnecessary effort.

Translator comments are part of the burden of developing an application that needs to be localized; it is a worthwhile burden.

[jarolrod]

tACK ac4fb3b

Will pick up translation fixup in a follow-up.

[furszy]

@furszy

Have to say that mixing source code written in c++ with comments for translators isn't convincing me. There should be another way, some external file/s where we could map every text being translated to its own translator comment so we don't end up adding external stuff to the sources (if something like this doesn't exist then maybe we should create it).

this is nothing new, we just follow Qt's process for writing source code for translation. We've chosen to provide context to translators to help them with translating from english to a target language, and to do this we need to provide a translator comment. I am not aware of any other way to provide translator context and nicely integrate it with transifex. Custom tooling seems like an unnecessary effort.

Translator comments are part of the burden of developing an application that needs to be localized; it is a worthwhile burden.

Yeah, I understood the rationale behind it (not the first time working with localized applications), I'm just not happy mixing code (which should be as clean as possible) with comments that should be properly crafted for non-technical people. We might end up having large cpp files full of non-technical comments that would make readability harder which is one of the main bug causes.

I'm not saying to not have them. Just check if there are other options, primarily to have them located elsewhere.

[jarolrod]

We might end up having large cpp files full of non-technical comments that would make readability harder which is one of the main bug causes.

Won't be an issue with the switch to qml :)