Do not require restart if overridden option is modified #440

1. 
   hebasto commented at 8:36 pm on September 29, 2021: member

   The options set via the GUI can be overridden by settings in the bitcoin.conf or by command-line options. If such an option is overridden, its modification in the GUI has no effect. Therefore, no need to require the client restart.
2. qt: Do not require restart if overridden option is modified 2f0e935cbb
3. qt, refactor: Reorder calls according to widget locations

   This is a move-only change.

   17cea05b4c
4. hebasto added the label UX on Sep 29, 2021
5. 
   jonatack commented at 8:43 pm on September 29, 2021: contributor

   Concept ACK.
6. 
   jarolrod commented at 1:34 am on September 30, 2021: member

   concept ack
7. 
   shaavan commented at 12:12 pm on September 30, 2021: contributor

   Concept ACK
8. shaavan approved
9. 
   shaavan commented at 1:38 pm on September 30, 2021: contributor

   tACK 17cea05b4c23606f3d7b2be9e1c01f9a8188dee9 Tested on Ubuntu 20.04 (Using Qt version 5.12.8)

   This PR allows not displaying restart warning for change in options, whose value is already overridden under the bitcoin.conf file.

   This is done by adding a conditional statement that displays the warning only when, setting’s name is not present in the bitcoin.conf file (edit: or in command-line options). I checked if there is a way to write only the setting’s name in bitcoin.conf file without specifying the value. There is none. So the logic of code is exemplary here.

   The formatting of the code is correct, and the ordering of settings correctly matches the order in which they are displayed. I was able to test this PR on Ubuntu 20.04 successfully. I was able to try all the individual settings successfully. Let me added an example screenshot pair.

   Screenshots (for prune option):

   Before updating the bitcoin.conf file	After Updating the bitcoin.conf file

   I agree with the changes suggested in this PR. The settings overridden by the bitcoin.conf file will not be affected by the change of setting in GUI, and hence they don’t need a restart warning for them.

10. 
    hebasto commented at 1:40 pm on September 30, 2021: member

    @shaavan

    … is not present in the bitcoin.conf file…

    Or in command-line options.

11. 
    luke-jr commented at 7:54 pm on October 3, 2021: member

    Wouldn’t it make more sense to instead have a warning message that restart is needed without the overriding option, to make it have effect?
12. 
    jarolrod commented at 5:28 am on October 4, 2021: member

    tACK 17cea05

    Tested on macOS 12. Conceptually, I agree with this PR. But, the current situation/UX around settings is not ideal. While this change is a step in the right direction, we should still do more.

    In regards to this change, we could follow up with some change that makes it clear to the user that a setting is currently overriden by the bitcoin.conf. It could use similar logic as this pr, utilizing: model->getOverriddenByCommandLine().contains("-prune").

13. 
    ryanofsky commented at 4:37 pm on October 4, 2021: contributor

    I think this PR seems a little dangerous and is is not really an improvement in it’s current form, but it could be a good change with some tweaks.

    The problem I see is the same one luke-jr and jarolrod pointed out, that this is not providing the user a clear enough warning that the change they just tried to make will be ignored. Even if the “This change would require a restart” message was not strictly accurate in all cases (because restarting could be necessary but not sufficient to apply the changes), at least the “This change would require a restart” message gave immediate feedback to the user that the changes they just made would not take effect. After this PR, there is no prominent feedback about the changes being ignored, except for the easy-to-miss “Options set in this dialog are overridden” blurb.

    I think if the goal of this PR is to remove ambiguity of the “This change would require a restart message”, instead of dropping it would be better to replace it with less ambiguous messages depending on the circumstance:

    • “Changes will not take effect until bitcoin is restarted” in the normal case.
    • “Changes will not take effect until bitcoin is restarted and command line or configuration file overrides are removed” in the special case addressed by this PR.

    In regards to this change, we could follow up with some change that makes it clear to the user that a setting is currently overriden by the bitcoin.conf. It could use similar logic as this pr, utilizing: model->getOverriddenByCommandLine().contains("-prune").

    This is really a broader question, and probably best to discuss in a new issue. (There is also a wiki page https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Settings-design-questions to explore tradeoffs, but it hasn’t really been filled out or updated.)

    Two things are worth noting though:

    • bitcoin/bitcoin#15936 changes behavior so that settings changed in the GUI will take priority over setting specified in bitcoin.conf, and will be used by bitcoind. I think this makes GUI settings dialog more useful and powerful and consistent and reduces complexity here.

    • bitcoin/bitcoin#15936 does not change the behavior of command line settings overriding GUI settings, which is a good thing, but still leaves open the question of how GUI should reflect the overrides. I don’t think anybody thinks the “Options set in this dialog are overridden” blurb is a great solution. Another solution could be to show the overridden settings as disabled. Another interesting solution is one Sjors implemented in bitcoin/bitcoin#12833 which is to drop the “Options set in this dialog are overridden” blurb entirely, show active command line values in the settings dialog, but treat the settings as temporary and do not persist them unless the user actually sets them.

14. 
    DrahtBot commented at 10:54 am on December 30, 2021: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #603 (Add settings.json prune-prev, proxy-prev, onion-prev settings by ryanofsky)
    • #602 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
    • #600 (refactor: Add OptionsModel getOption/setOption methods by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

15. DrahtBot added the label Needs rebase on May 22, 2022
16. 
    DrahtBot commented at 7:46 pm on May 22, 2022: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

17. hebasto closed this on May 23, 2022

    ---

18. bitcoin-core locked this on May 23, 2023

Contributors
hebasto jonatack jarolrod shaavan luke-jr ryanofsky DrahtBot

Labels
Needs rebase UX