Pass argsmanager reference to intro instead of relying on global #75

1. 
   MarcoFalke commented at 3:11 pm on August 26, 2020: contributor

   Motivation: (copied from https://github.com/bitcoin/bitcoin/pull/19779)

   The gArgs global has several issues:

   • gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, …), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value.
   • Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. https://github.com/bitcoin/bitcoin/pull/19704#issuecomment-678259092 or #19511

   The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs to use a passed-in reference instead.

2. qt: Pass argsmanager reference to intro instead of relying on global fa1b478ddb
3. 
   MarcoFalke commented at 3:12 pm on August 26, 2020: contributor

   requested by @ryanofsky in #35 (review)
4. MarcoFalke renamed this:
   qt: Pass argsmanager reference to intro instead of relying on global
   Pass argsmanager reference to intro instead of relying on global
   on Aug 26, 2020
5. MarcoFalke added the label Refactoring on Aug 26, 2020
6. 
   hebasto commented at 6:17 pm on August 28, 2020: member

   Concept ACK.
7. in src/qt/intro.h:50 in fa1b478ddb

   46@@ -47,7 +47,7 @@ class Intro : public QDialog
   47      * @note do NOT call global GetDataDir() before calling this function, this
   48      * will cause the wrong path to be cached.
   49      */
   50-    static bool showIfNeeded(bool& did_show_intro, bool& prune);
   51+    static bool showIfNeeded(ArgsManager& args, bool& did_show_intro, bool& prune);
   

   ---

   ---

   

   hebasto commented at 6:32 pm on August 28, 2020:

   As args is an in-out parameter, maybe place it at the end of the parameter list?

   ---

   

   MarcoFalke commented at 6:44 pm on August 28, 2020:

   every param in this list is an in-out param

   ---

   

   hebasto commented at 7:25 pm on August 28, 2020:

   Indeed.
8. hebasto approved
9. 
   hebasto commented at 6:35 pm on August 28, 2020: member

   ACK fa1b478ddb7eccea2ae33246212a36ec503cdbca, tested on Linux Mint 20 (x86_64) with the -choosedatadir command-line option.
10. 
    MarcoFalke commented at 8:16 am on August 31, 2020: contributor

    @ryanofsky Would be good to have your eyes over this as a sanity check :eyes:
11. MarcoFalke closed this on Aug 31, 2020

    ---

12. MarcoFalke reopened this on Aug 31, 2020

    ---

13. 
    ryanofsky commented at 4:29 pm on September 9, 2020: contributor

    I don’t think this change is beneficial in it’s current form. Before this change there are 46 references to the gArgs global in src/qt/, and after this change there are 40 references to gArgs. I think if you really want to get rid of the gArgs global in the GUI you could do it in a single PR instead of having gArgs and args variables used different places unpredictably.

    Also I don’t think there is a necessarily a reason to eliminate the gArgs in the GUI. It’s understandable to want to get rid of global variables in node and wallet code to allow more code reuse and help testing, but the GUI already relies on a global QApplication instance, a global event loop, global settings state and probably other global state, so getting rid of gArgs there doesn’t seem to accomplish much. I think removing gArgs from node and wallet code and then making gArgs a GUI-only variable would satisfy goals stated in the PR description while keeping GUI code less verbose, and requiring fewer changes.

14. 
    MarcoFalke commented at 6:58 pm on September 9, 2020: contributor

    Ok, thanks for looking.
15. MarcoFalke closed this on Sep 9, 2020

    ---

16. MarcoFalke deleted the branch on Sep 9, 2020
17. bitcoin-core locked this on Feb 15, 2022

Contributors
MarcoFalke hebasto ryanofsky

Labels
Refactoring