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

[Refactor] Init wallet parameters interaction #1445

Merged
merged 4 commits into from
Apr 1, 2020

Conversation

furszy
Copy link

@furszy furszy commented Mar 22, 2020

This PR does two things:

  1. Move the pwalletMain object inside the wallet.h file.
  2. Move the wallet's parameters initialization to an static method ParameterInteraction in the wallet class.

This is coming (partially) from upstream 25340b7 .

@furszy furszy self-assigned this Mar 22, 2020
Partially coming from 25340b7
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Code ACK.
Just minor concerns about some code duplication between wallet and init. E.g the UIError/UIWarning static functions in wallet have almost the same implementation of InitError/InitWarning functions (also static) in init.cpp. Wouldn't it be better to export them from wallet and use them during init as well?
Actually, imo they don't even belong to the wallet file, really: should be in some "util" file, and imported from there both in wallet and init.

src/init.cpp Outdated Show resolved Hide resolved
@random-zebra random-zebra added Refactoring Startup Wallet startup changes and/or improvements Upstream labels Mar 25, 2020
@random-zebra random-zebra added this to the 4.1.0 milestone Mar 25, 2020
@furszy
Copy link
Author

furszy commented Mar 26, 2020

You know, I thought on that but then thought on the translation context for the AmountErrMsg method, which will need to receive the parent or make the util file depend on guiinterface (which is not good, util.h shouldn't have any GUI dependency).

Good catch on the InitError and InitWarning. I missed them completely.

@random-zebra
Copy link

which will need to receive the parent or make the util file depend on guiinterface

why? AmountErrMsg is only a string utility (takes a char pointer and a string to return a new string). It has no dependency with guiinterface (UIError and UIWarning do).

@furszy
Copy link
Author

furszy commented Mar 26, 2020

The _ before the parenthesis is a translation function call which is part of guiinterface.

@random-zebra
Copy link

Right.
Well, couldn't this be directly inlined in guiinterface then?

@furszy
Copy link
Author

furszy commented Mar 26, 2020

it could but not really the file's responsibility. That file should be clean as possible from utility functions.
We should have guiinterfaceutil or well, need to check if guiutil.h is suited for this or not.

@random-zebra
Copy link

I don't think guiutil.h is a good place either. Would introduce unneeded dependencies in other files.
Probably a separate util file (in src, not in src/qt) would be better.

@furszy furszy force-pushed the 2020_wallet_parameters_setup branch from d0a2ae3 to b7d1c41 Compare March 27, 2020 16:11
@furszy
Copy link
Author

furszy commented Mar 27, 2020

Updated.

@furszy furszy force-pushed the 2020_wallet_parameters_setup branch from b7d1c41 to d452a2d Compare March 27, 2020 23:56
random-zebra
random-zebra previously approved these changes Mar 28, 2020
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK d452a2d

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 831834c

@furszy furszy merged commit 9896879 into PIVX-Project:master Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Startup Wallet startup changes and/or improvements Upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants