Reset options, notify user about backup creation

furszy commented at 3:08 pm on June 12, 2022: contributor

Quick follow-up to first point of #602#pullrequestreview-1002780997

hebasto renamed this:
~~gui: reset options, notify user about backup creation~~
Reset options, notify user about backup creation
on Jun 12, 2022

DrahtBot commented at 9:43 pm on June 12, 2022: contributor

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

Conflicts

No conflicts as of last run.

Rspigler commented at 3:53 am on June 13, 2022: contributor

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 commented at 1:01 pm on June 13, 2022: contributor

@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).

jarolrod added the label UX on Jun 13, 2022

Rspigler commented at 10:03 pm on June 13, 2022: contributor

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

Tests do pass

shaavan approved

shaavan commented at 12:40 pm on June 14, 2022: contributor

ACK 52372cbfdb4d2cffd83f238cccab84fdc4cc6632

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 commented at 1:32 pm on June 14, 2022: contributor

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

in src/qt/optionsdialog.cpp:292 in 52372cbfdb outdated

284         // confirmation dialog
285         QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
286-            tr("Client restart required to activate changes.") + "<br><br>" + tr("Client will be shut down. Do you want to proceed?"),
287+            tr("Client restart required to activate changes.") + "<br><br>" +
288+            tr("Current settings will be backed up at \"%1\".").arg(GUIUtil::getDefaultDataDirectory()) + "<br><br>" +
289+            tr("Client will be shut down. Do you want to proceed?"),

jarolrod commented at 9:06 am on June 23, 2022:

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 commented at 7:23 pm on June 28, 2022:

re: #617 (review)

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

in src/qt/optionsdialog.cpp:285 in 52372cbfdb outdated

283+    if (model) {
284         // confirmation dialog
285         QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
286-            tr("Client restart required to activate changes.") + "<br><br>" + tr("Client will be shut down. Do you want to proceed?"),
287+            tr("Client restart required to activate changes.") + "<br><br>" +
288+            tr("Current settings will be backed up at \"%1\".").arg(GUIUtil::getDefaultDataDirectory()) + "<br><br>" +

ryanofsky commented at 7:18 pm on June 23, 2022:

In commit “gui: reset options, notify user about the backup creation” (52372cbfdb4d2cffd83f238cccab84fdc4cc6632)

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

furszy commented at 1:33 pm on June 28, 2022:

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

gui: reset options, notify user about the backup creation ac4fb3bbbe

furszy force-pushed on Jun 28, 2022

furszy commented at 1:35 pm on June 28, 2022: contributor

Updated per ryanofsky feedback.

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

ryanofsky approved

ryanofsky commented at 7:24 pm on June 28, 2022: contributor

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

jarolrod commented at 7:31 pm on June 28, 2022: member

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

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

 0@@ -278,14 +284,23 @@ void OptionsDialog::setOkButtonState(bool fState)
 1
 2 void OptionsDialog::on_resetButton_clicked()
 3 {
 4-    if(model)
 5-    {
 6+    if (model) {
 7         // confirmation dialog
 8+        /*: Text explaining that the settings the user changed will not come
 9+            into effect until the client is restarted. */
10+        QString reset_window_text = tr("Client restart required to activate changes.") + "<br><br>";
11+        /*: Text explaining to the user that the client's current settings
12+            will be backed up at a specific location. %1 is a stand-in
13+            argument for the backup location's path. */
14+        reset_window_text.append(tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>");
15+        /*: Text asking the user to confirm if they would like to proceed
16+            with a client shutdown. */
17+        reset_window_text.append(tr("Client will be shut down. Do you want to proceed?"));
18+        //: Window title text of pop-up window shown when the user has chosen to reset options.
19         QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
20-            tr("Client restart required to activate changes.") + "<br><br>" + tr("Client will be shut down. Do you want to proceed?"),
21-            QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);
22+            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 commented at 8:18 pm on June 28, 2022: contributor

suggesting the following translator comments:

This is great. Thanks!

furszy commented at 10:07 pm on June 28, 2022: contributor

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 commented at 10:21 pm on June 28, 2022: member

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

jarolrod commented at 10:23 pm on June 28, 2022: member

tACK ac4fb3bbbe207d0744201a9df8a971af06db5476

Will pick up translation fixup in a follow-up.

hebasto merged this on Jun 28, 2022

hebasto closed this on Jun 28, 2022

furszy commented at 10:36 pm on June 28, 2022: contributor

@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 commented at 10:39 pm on June 28, 2022: member

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

sidhujag referenced this in commit e02df2a15b on Jun 29, 2022

jarolrod referenced this in commit d5c141f221 on Jun 30, 2022

hebasto referenced this in commit 27a4dd055b on Jul 12, 2022

bitcoin-core locked this on Jun 28, 2023

Reset options, notify user about backup creation #617

Conflicts