Add cancel button to configuration options popup #391

pull shaavan wants to merge 1 commits into bitcoin-core:master from shaavan:cancel-conf changing 1 files +16 −4
  1. shaavan commented at 7:42 pm on July 29, 2021: contributor

    This PR renames the OK button to Continue and adds a Cancel button to the configuration options pop-up.

    This feature will give the user an option to abort opening the configuration file if they want to. This is an essential helpful feature that was missing in the master branch.

    In some windows managers such as Windows I3. The exit button at the top right corner is missing. So this feature becomes crucial there. And even when the exit button is there, it doesn’t prevent the opening of the configuration file even when pressed.

    Additionally, it will always be possible to close using Keyboard Shortcut. This PR helps accessibility for those who need to use a mouse.

    Cancel-conf master(1)

    Screenshot from 2021-09-07 20-15-28

  2. shaavan renamed this:
    qt: Add cancel button to configuration options popup
    add cancel button to configuration options popup
    on Jul 29, 2021
  3. jarolrod added the label UX on Jul 29, 2021
  4. ghost commented at 8:41 pm on July 29, 2021: none
    Concept ACK. Can remove extra QMessageBox::Ok
  5. in src/qt/optionsdialog.cpp:303 in f9ff47c6ed outdated 
    301-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
302-           "Additionally, any command-line options will override this configuration file."));
303+    int ret = QMessageBox::information(this, tr("Configuration options"),
304+                                       tr("The configuration file is used to specify advanced user options which override GUI settings. "
305+                                          "Additionally, any command-line options will override this configuration file."),
306+                                       QMessageBox::Cancel | QMessageBox::Ok, QMessageBox::Ok);
    unknown commented at 8:41 pm on July 29, 2021: 
    0                                       QMessageBox::Cancel | QMessageBox::Ok);
    shaavan commented at 1:01 pm on July 31, 2021:
    That extra QMessageBox::Ok specifies selecting the Ok button by default, and I think it is better to keep it as it is.
  6. psancheti110 commented at 6:40 pm on August 2, 2021: contributor

    I agree with the concept proposed @ShaMan239 conceptually, which aborts the opening of the config file in case of keystroke by mistake on the tab.

    There are a few points I believe should be addressed here about this window.

    • The text that appears on the screen on clicking Open Configuration File is a message, meant for the user describing what a config file is. It is a pretty essential message for new users to Bitcoin and are trying to get hands-on experience with GUI. But because of the UI Symbol present in the dialogue, it appears more like a warning than message. This needs to be addressed.

    • I would like to put forward another approach that might enhance the feature. There are users/contributors who are very much familiar with Bitcoin GUI and hence are aware of what a configuration file is. Getting a message every time they try to open the configuration file can get irritating at some point.

      Adding a Do not show this message again option will resort to it. This will allow the user to allow the UI to show this message or NOT to show this message based on their preference. If a user checks this option and proceeds, they will not be shown this message ever again unless they change the setting back manually. I believe this will serve both the purpose of why this message was placed in the window in the first place (to provide an important message to new users) and providing existing experienced users with an option to turn it off forever.

    Please let me know your views on the suggested addition. If it looks good, we could work on it together :)

  7. jarolrod commented at 4:18 am on August 3, 2021: member

    Since the logic here is now of close | open, perhaps we could change the ok button into an open button. But the icon associated with the open button isn’t great.:

    alt

    Neither is the Close button icon as pointed out by @psancheti110:

    But because of the UI Symbol present in the dialogue, it appears more like a warning than message. This needs to be addressed.

    To address this let’s just not use any icons.

  8. psancheti110 commented at 4:21 am on August 4, 2021: contributor

    @jarolrod,

    To address this let’s just not use any icons.

    I like this idea better. That might clear the confusion and depict it like a message as it is supposed to be.

    Also would appreciate it if you could provide your views on the Do not show this message again I have mentioned above?

  9. in src/qt/optionsdialog.cpp:296 in f9ff47c6ed outdated 
    292@@ -293,12 +293,21 @@ void OptionsDialog::on_resetButton_clicked()
293     }
294 }
295 
296+
    hebasto commented at 8:28 am on August 4, 2021:
    Why this extra line?
    shaavan commented at 12:27 pm on August 11, 2021:

    Why this extra line?

    Sorry, that was a blunder on my part. Fixed in 25bee9251374fa78d19077f2fb5fa0bfc5d651a2

  10. in src/qt/optionsdialog.cpp:300 in f9ff47c6ed outdated 
    298 {
299     /* explain the purpose of the config file */
300-    QMessageBox::information(this, tr("Configuration options"),
301-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
302-           "Additionally, any command-line options will override this configuration file."));
303+    int ret = QMessageBox::information(this, tr("Configuration options"),
    hebasto commented at 8:29 am on August 4, 2021:
    Could be const?
    shaavan commented at 12:29 pm on August 11, 2021:
    Changed the declaration style in 25bee9251374fa78d19077f2fb5fa0bfc5d651a2. Now instead of int ret now QMessageBox config_msgbox, is used.
  11. in src/qt/optionsdialog.cpp:301 in f9ff47c6ed outdated 
    299     /* explain the purpose of the config file */
300-    QMessageBox::information(this, tr("Configuration options"),
301-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
302-           "Additionally, any command-line options will override this configuration file."));
303+    int ret = QMessageBox::information(this, tr("Configuration options"),
304+                                       tr("The configuration file is used to specify advanced user options which override GUI settings. "
    hebasto commented at 8:30 am on August 4, 2021:
    While these lines are touched, maybe add a commit with translator comments?
    shaavan commented at 12:29 pm on August 11, 2021:
    Added translator comments in 25bee9251374fa78d19077f2fb5fa0bfc5d651a2
  12. in src/qt/optionsdialog.cpp:310 in f9ff47c6ed outdated 
    308+    switch (ret) {
309+    case QMessageBox::Cancel:
310+        return;
311+    case QMessageBox::Ok:
312+        break;
313+    }
    hebasto commented at 8:32 am on August 4, 2021:

    Is it simpler

    0    if (ret == QMessageBox::Cancel) return;

    ?

    hebasto commented at 9:01 am on August 4, 2021:

    Or even better:

    0    if (ret != QMessageBox::Ok) return;

    The latter variant fixes a bug which also is present in master: when the dialog is opened and the program is being shutdown (e.g., via the Exit action of the systray icon menu) a configuration file gets opened.

    shaavan commented at 12:31 pm on August 11, 2021:

    if (ret != QMessageBox::Ok) return;

    That’s a great suggestion. Updated switch/case to this format, in 25bee9251374fa78d19077f2fb5fa0bfc5d651a2

  13. hebasto commented at 8:41 am on August 4, 2021: member

    Approach ACK f9ff47c6ed77cb6502f42fc37c7aa71ca92af476

    This feature will give the user an option to abort opening the configuration file if they want to.

    Agree.

    In some windows managers such as Windows I3. The exit button at the top right corner is missing. So this feature becomes crucial there. And even when the exit button is there, it does not allow you to cancel opening the configuration file.

    Additionally, it will always be possible to close using Keyboard Shortcut. This PR helps accessibility for those who need to use a mouse.

    I didn’t quite get it. On master I have no means to abort a configuration file opening at all:

    • no Cancel button
    • closing window with mouse just continues the file opening
    • closing window with Esc just continues the file opening
  14. hebasto renamed this:
    add cancel button to configuration options popup
    Add cancel button to configuration options popup
    on Aug 4, 2021
  15. hebasto commented at 8:47 am on August 4, 2021: member

    In the PR description

    127555187-db217eb1-e4c0-43eb-91fd-1ef04cba3f9d

    there is a “Close” button. Is it a correct screenshot?

  16. shaavan force-pushed on Aug 11, 2021
  17. shaavan force-pushed on Aug 11, 2021
  18. shaavan commented at 12:25 pm on August 11, 2021: contributor

    Updated from f9ff47c6ed77cb6502f42fc37c7aa71ca92af476 to 25bee9251374fa78d19077f2fb5fa0bfc5d651a2 (pr391.01 -> pr391.03)

    Addressed comments of @jarolrod, @psancheti110, and @hebasto Changes made are:

    1. Removed Icons for Buttons
    2. Changed ordering of button to now match the sequence of open and cancel button of the parent option dialog box
    3. Fixed Clang formatting
  19. shaavan commented at 12:48 pm on August 11, 2021: contributor

    there is a “Close” button. Is it a correct screenshot?

    The PR description was erroneous and is now updated.

  20. hebasto commented at 1:47 pm on August 11, 2021: member 
    0qt/optionsdialog.cpp: In member function ‘void OptionsDialog::on_openBitcoinConfButton_clicked()’:
1qt/optionsdialog.cpp:308:18: error: unused variable ‘close_button’ [-Werror=unused-variable]
2     QPushButton* close_button = config_msgbox.addButton(tr("Close"), QMessageBox::RejectRole);
3                  ^~~~~~~~~~~~
  21. shaavan force-pushed on Aug 11, 2021
  22. in src/qt/optionsdialog.cpp:307 in 13930ea230 outdated 
    306+    /*: The instructions written in this file will overwrite settings of Graphical User Interface.
307+        This instructions will themselves be overwritten by instructions given through command-line.*/
308+    config_msgbox.setText(tr("The configuration file is used to specify advanced user options which override GUI settings. "
309+                             "Additionally, any command-line options will override this configuration file."));
310+
311+    QPushButton* open_button = config_msgbox.addButton(tr("Open"), QMessageBox::ActionRole); //setting ActionRole to fix ordering issue of the buttons
    jarolrod commented at 5:27 pm on August 11, 2021:

    no need for this comment

    0    QPushButton* open_button = config_msgbox.addButton(tr("Open"), QMessageBox::ActionRole);
    shaavan commented at 3:19 pm on September 7, 2021:
    Addressed in ddec692.
  23. shaavan commented at 5:32 pm on August 11, 2021: contributor

    Updated from 25bee92 to 13930ea (pr391.03 to pr391.04)

    Addressed this comment by @hebasto

    Changes made:

    • Didn’t declare the added Close button as a separate variable, which corrected the unused variable warning.
  24. in src/qt/optionsdialog.cpp:300 in 13930ea230 outdated 
    299-    QMessageBox::information(this, tr("Configuration options"),
300-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
301-           "Additionally, any command-line options will override this configuration file."));
302+    QMessageBox config_msgbox;
303+    config_msgbox.setIcon(QMessageBox::Information);
304+    //: Option file to give custom user configuration instructions.
    jarolrod commented at 5:36 pm on August 11, 2021: 
    0    //: Window title text of pop-up window which allows you to open up your configuration file.
    shaavan commented at 3:20 pm on September 7, 2021:
    Addressed in ddec692.
  25. in src/qt/optionsdialog.cpp:302 in 13930ea230 outdated 
    301-           "Additionally, any command-line options will override this configuration file."));
302+    QMessageBox config_msgbox;
303+    config_msgbox.setIcon(QMessageBox::Information);
304+    //: Option file to give custom user configuration instructions.
305+    config_msgbox.setWindowTitle(tr("Configuration options"));
306+    /*: The instructions written in this file will overwrite settings of Graphical User Interface.
    jarolrod commented at 5:55 am on August 20, 2021:

    The translator comment is not very effective in helping me translate the corresponding string. Maybe re-think how it could be better.

    A good exercise is to really put yourself in the shoes of someone tasked to translate this. What information/context would you need to be able to effectively translate this string. Then use that to craft your translator comment.

    shaavan commented at 3:20 pm on September 7, 2021:
    Addressed in ddec692.
  26. shaavan force-pushed on Aug 20, 2021
  27. shaavan force-pushed on Aug 20, 2021
  28. shaavan commented at 12:49 pm on August 20, 2021: contributor

    Updated from bb17cd8 to ddec692 (pr391.04 -> pr391.05) changes: addressed 1 and 2 suggestions by @jarolrod

    • Updated translator comments.
    • Removed unnecessary comment about action role.

    Thanks, @jarolrod, for the suggestions.

  29. shaavan force-pushed on Aug 20, 2021
  30. shaavan force-pushed on Aug 20, 2021
  31. shaavan commented at 3:27 pm on August 20, 2021: contributor
    Updated from ddec692 to 4a0b791 The first force commit was pushed by mistake, and the second one is a correction to it. For the reviewers: There are no changes made by the above two commits
  32. jarolrod commented at 4:51 pm on August 20, 2021: member

    ~tACK 4a0b791~

    This window looks good now without the confusing icons. PR functions as intended.

    Screenshot of current window: pr391

    retracted ACK, see here: #391#pullrequestreview-747473854

  33. hebasto commented at 9:23 pm on August 23, 2021: member
    FWIW, the error in fuzzing job on bitcoinbuilds.org is related to https://github.com/bitcoin/bitcoin/pull/22517.
  34. hebasto commented at 9:35 pm on August 23, 2021: member

    @GBKS @bosch-0

    Could you look into this PR, please?

    What, in your designer’s opinion, is the best text on:

    1. the button which continues the procedure and actually opens the bitcoin.conf file
    2. the button which aborts the procedure of opening the bitcoin.conf, and returns the GUI to the previous state

    ?

  35. GBKS commented at 9:35 am on September 6, 2021: none

    How about “Continue” and “Cancel”?

    Just as a side note, on my Mac, the pop-up does not have a title, so I don’t see the text “Configuration options” shown in the screenshots above.

  36. in src/qt/optionsdialog.cpp:300 in 4a0b79170f outdated 
    299-    QMessageBox::information(this, tr("Configuration options"),
300-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
301-           "Additionally, any command-line options will override this configuration file."));
302+    QMessageBox config_msgbox;
303+    config_msgbox.setIcon(QMessageBox::Information);
304+    //: Window title text of pop-up box, which allows opening up configuration file.
    jarolrod commented at 1:44 am on September 7, 2021: 
    0    //: Window title text of pop-up window that allows you to open up your configuration file.
    shaavan commented at 3:21 pm on September 7, 2021:
    Addressed in 52aec5d.
  37. in src/qt/optionsdialog.cpp:302 in 4a0b79170f outdated 
    301-           "Additionally, any command-line options will override this configuration file."));
302+    QMessageBox config_msgbox;
303+    config_msgbox.setIcon(QMessageBox::Information);
304+    //: Window title text of pop-up box, which allows opening up configuration file.
305+    config_msgbox.setWindowTitle(tr("Configuration options"));
306+    /*: Self explanatory text about the priority order of instructions considered by client.
    jarolrod commented at 2:01 am on September 7, 2021: 
    0    /*: Explanatory text about the priority order of instructions considered by client.
    shaavan commented at 3:21 pm on September 7, 2021:
    Addressed in 52aec5d.
  38. in src/qt/optionsdialog.cpp:303 in 4a0b79170f outdated 
    302+    QMessageBox config_msgbox;
303+    config_msgbox.setIcon(QMessageBox::Information);
304+    //: Window title text of pop-up box, which allows opening up configuration file.
305+    config_msgbox.setWindowTitle(tr("Configuration options"));
306+    /*: Self explanatory text about the priority order of instructions considered by client.
307+        the order from high to low being: command-line -> configuration file -> GUI settings. */
    jarolrod commented at 2:01 am on September 7, 2021:

    You cannot use the arrow in the translator comments; they show up as something else, see the output of make -C src translate:

    0<extracomment>Self explanatory text about the priority order of instructions considered by client. the order from high to low being: command-line -&gt; configuration file -&gt; GUI settings.</extracomment>

    as you can see, the arrow’s show up as -&gt;

    0        The order from high to low being: command-line, configuration file, GUI settings. */
    shaavan commented at 3:21 pm on September 7, 2021:

    Addressed in 52aec5d.

    as you can see, the arrow’s show up as -&gt;

    Thanks for pointing that out. I had no idea about such behavior of the -> in translator comments.

    jarolrod commented at 3:39 pm on September 7, 2021:
    It is not specific to the combination of characters that to us mean an arrow: ->. What’s happening is that the > symbol gets converted to &gt.
  39. jarolrod commented at 2:06 am on September 7, 2021: member

    @ShaMan239 we should adopt @GBKS suggestion as a designer, so I retract my ack. Please update the button text according to his suggestions. Below are some other suggestions related to the translator comments. @GBKS thanks for taking the time to review a GUI PR. What you’ve noticed about the missing window title of a QMessageBox on macOS is the expected behavior, see: QMessageBox::setWindowTitle

    Sets the title of the message box to title. On macOS, the window title is ignored (as required by the macOS Guidelines).

  40. shaavan force-pushed on Sep 7, 2021
  41. shaavan commented at 2:50 pm on September 7, 2021: contributor

    Updated from 4a0b791 to 52aec5dd2338a3af627a6897dd5efe59a928bd10 (pr391.05 -> pr391.06) Addressed @GBKS and @jarolrod comments. Thanks, @GBKS, for taking the time to review this PR and giving your valuable suggestions. Thanks, @jarolrod, for suggesting the appropriate changes in the translator’s comments that I had missed earlier.

    Changes:

    • Updated buttons names from “Open” and “Close” to “Continue” and “Cancel.”
    • Updated translator comments as per @jarolrod suggestions.
    • Updated PR description accordingly
  42. unknown approved
  44. in src/qt/optionsdialog.cpp:298 in 52aec5dd23 outdated 
    294@@ -295,10 +295,22 @@ void OptionsDialog::on_resetButton_clicked()
295 
296 void OptionsDialog::on_openBitcoinConfButton_clicked()
297 {
298-    /* explain the purpose of the config file */
299-    QMessageBox::information(this, tr("Configuration options"),
300-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
301-           "Additionally, any command-line options will override this configuration file."));
302+    QMessageBox config_msgbox;
    hebasto commented at 3:39 pm on September 9, 2021:

    To preserve behavior

    0    QMessageBox config_msgbox(this);

    https://doc.qt.io/qt-5/qdialog.html:

    A dialog is always a top-level widget, but if it has a parent, its default location is centered on top of the parent’s top-level widget (if it is not top-level itself).

    shaavan commented at 1:38 pm on September 10, 2021:
    Addressed in 0b869df
  45. in src/qt/optionsdialog.cpp:300 in 52aec5dd23 outdated 
    299-    QMessageBox::information(this, tr("Configuration options"),
300-        tr("The configuration file is used to specify advanced user options which override GUI settings. "
301-           "Additionally, any command-line options will override this configuration file."));
302+    QMessageBox config_msgbox;
303+    config_msgbox.setIcon(QMessageBox::Information);
304+    //: Window title text of pop-up box that allows you to open up your configuration file.
    hebasto commented at 3:41 pm on September 9, 2021:

    nit:

    Translator comments are kind of documentation. Could they be more formal, without “you” and “your”?

    shaavan commented at 1:39 pm on September 10, 2021:
    Addressed in 0b869df
  46. in src/qt/optionsdialog.cpp:307 in 52aec5dd23 outdated 
    306+    /*: Explanatory text about the priority order of instructions considered by client.
307+        The order from high to low being: command-line, configuration file, GUI settings. */
308+    config_msgbox.setText(tr("The configuration file is used to specify advanced user options which override GUI settings. "
309+                             "Additionally, any command-line options will override this configuration file."));
310+
311+    QPushButton* open_button = config_msgbox.addButton(tr("Continue"), QMessageBox::ActionRole);
    hebasto commented at 3:43 pm on September 9, 2021:

    Not sure if QMessageBox::ActionRole is a good choice.

    QMessageBox::ActionRole:

    Clicking the button causes changes to the elements within the dialog.

    Maybe QMessageBox::AcceptRole or QMessageBox::YesRole?

    In case of QMessageBox::AcceptRole the button positions are evaluated according to the system-dependant styles. And s/open_button->setDefault(true);/config_msgbox.setDefaultButton(open_button);/ is required.

    shaavan commented at 1:26 pm on September 10, 2021:
    Both QMessageBox::AcceptRole and QMessageBox::YesRole are leading to changing the ordering of continue and cancel buttons. I am trying to keep ordering buttons in the Configuration Options window and Options window same to maintain consistency but both AcceptRole and YesRole mess that up. Screenshot from 2021-09-10 18-47-46 I guess there must be some way of using one of these roles without disrupting the ordering, but I cannot find it yet.
  47. qt: Add cancel button to configuration options popup 
    This adds a cancel buttion to the configuration options window
    0b869df1c9
  48. shaavan force-pushed on Sep 10, 2021
  49. shaavan commented at 1:38 pm on September 10, 2021: contributor

    Updated from 52aec5d to 0b869df1c913839855148d728514c76ba7664092 (pr391.06 -> pr391.07) Addressed 1 and 2 comments by @hebasto

    Changes made:

    • Rewritten Translators’ comment to now be not addressed to a second person
    • QMessageBox config_msgbox -> QMessageBox config_msgbox(this) to preserve behavior.
  50. hebasto approved
  51. hebasto commented at 3:28 pm on September 12, 2021: member

    ACK 0b869df1c913839855148d728514c76ba7664092, tested on Linux Mint 20.2 (Qt 5.12.8):

    Screenshot from 2021-09-12 18-27-00

  52. unknown approved
  53. unknown commented at 4:18 pm on September 12, 2021: none

    tACK https://github.com/bitcoin-core/gui/pull/391/commits/0b869df1c913839855148d728514c76ba7664092

    Tried on Windows 10

    image

    Continue and Cancel works as expected.

  54. hebasto merged this on Sep 12, 2021
  55. hebasto closed this on Sep 12, 2021
  56. shaavan commented at 4:42 pm on September 12, 2021: contributor
    Thank you very much to everyone who had given their valuable time to review this PR.
  57. sidhujag referenced this in commit 0548a8ef4d on Sep 13, 2021
  58. bitcoin-core locked this on Sep 12, 2022


shaavan ghost psancheti110 jarolrod hebasto GBKS

Labels
UX

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 07:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me