Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Contributor

@maflcko maflcko commented Aug 26, 2020

Motivation: (copied from bitcoin/bitcoin#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. Net processing: move ProcessMessage() to PeerLogicValidation bitcoin/bitcoin#19704 (comment) 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.

@maflcko
Copy link
Contributor Author

maflcko commented Aug 26, 2020

requested by @ryanofsky in #35 (comment)

@maflcko maflcko changed the title qt: Pass argsmanager reference to intro instead of relying on global Pass argsmanager reference to intro instead of relying on global Aug 26, 2020
@hebasto
Copy link
Member

hebasto commented Aug 28, 2020

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa1b478, tested on Linux Mint 20 (x86_64) with the -choosedatadir command-line option.

src/qt/intro.h Show resolved Hide resolved
@maflcko
Copy link
Contributor Author

maflcko commented Aug 31, 2020

@ryanofsky Would be good to have your eyes over this as a sanity check 👀

@maflcko maflcko closed this Aug 31, 2020
@maflcko maflcko reopened this Aug 31, 2020
@ryanofsky
Copy link
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.

@maflcko
Copy link
Contributor Author

maflcko commented Sep 9, 2020

Ok, thanks for looking.

@maflcko maflcko closed this Sep 9, 2020
@maflcko maflcko deleted the 2008-qtArgsIntro branch September 9, 2020 18:58
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants