bitcoin-core / gui-qml

Bitcoin GUI (experimental QML-based fork)
MIT License
106 stars 40 forks source link

Wiring for Custom Datadir #408

Open D33r-Gee opened 3 weeks ago

D33r-Gee commented 3 weeks ago

This pull request builds upon previous issues (#390, #392, and #397) by re-introducing the functionality for users to specify a custom data directory (datadir) during the onboarding process.

The majority of changes in qml/bitcoin.cpp are mainly splitting the onboarding and node creation so that the user can set their preferred datatdir it's inspired by the qt/bitcoin.cpp however it uses the anonymous namespace instead of using a class. The intention came from feedback on https://github.com/bitcoin-core/gui-qml/pull/390

In the QML pages, the node_model context property is no longer been initialized during onboarding. We're now using options_model instead. This means the code has been updated to use the options_model for settings that were previously set with the node_model context property.

Support for the Android version is being added to this PR.

D33r-Gee commented 3 weeks ago

Thanks @johnny9 for testing

Android builds are failing. Will need that fixed.

addressed it for now with the latest push 679818a

My testing on desktop seemed to work well. I had one odd thing happening. After onboarding, I tried again with using -> > resetguisettings and the onboarding page had a strange Custom path as the initial selection. I imagine this is maybe just resetguisettings not clearing out the datadir in QSettings but haven't dug in yet.

Addressed the above for now... although there are a few issues when using the -resetguisettings flag, so a question I have is is it crucial to address them here or can they be addressed in later PRs that specifically tackle them?

D33r-Gee commented 3 weeks ago

679818a

Thanks testing and reviewing @MarnixCroes

  • during onboarding, when clicking Custom datadir it gives this output in terminal
Model size of -106 is less than 0
Model size of -6 is less than 0

Hmmm... not sure what that is... will look into it

  • startNode and startOnboarding have a lot of duplicate code. maybe consider extracting common functionality to avoid duplication?

Yes some refinement can be done there... will look into how best approach it

D33r-Gee commented 3 weeks ago

Currently tackling a strange beahvior involving the custom datadir and reading the settings files:

when going through first setup it up it works great and the settings.json file is created in the the proper place...

However on restart it seems like it doesn't read the settings.json file from the new datadir and only looks in the default directory.

Right now trying to see how that can be addressed... will update soon with what I find

D33r-Gee commented 3 weeks ago

with aebfc89 addressed the settings.json file not being read on restart by moving the gArgs.ReadSettingsFile right before node creation in qml/bitcoin.cpp

Now will tackle the -resetguisettings not updating the datadir location right away

D33r-Gee commented 2 weeks ago

with the latest update 5ef5d19 addressed the -resetguisettings discrepencies.

It now resets to defaults and allows for changing the datadir

Next will tackle code optimization...

D33r-Gee commented 2 weeks ago

This update 6d4c111 is the first pass at optimizing the qml/bitcoin.cpp code.

There are no major functionality updates, mainly code consolidation.

D33r-Gee commented 1 week ago
  • during onboarding, when clicking Custom datadir it gives this output in terminal
Model size of -106 is less than 0
Model size of -6 is less than 0

@MarnixCroes are you still seeing the above pop-up with 6d4c111?

MarnixCroes commented 1 week ago
  • during onboarding, when clicking Custom datadir it gives this output in terminal
Model size of -106 is less than 0
Model size of -6 is less than 0

@MarnixCroes are you still seeing the above pop-up with 6d4c111?

I do not

D33r-Gee commented 1 week ago

Thanks @pablomartin4btc for testing and for your questions.

what's the intention of the changes in the protocol of bitcoin.cpp (re-structuring it for clarity?

The majority of changes in qml/bitcoin.cpp are mainly splitting the onboarding and node creation so that the user can set their preferred datatdir it's inspired by the qt/bitcoin.cpp however it uses the anonymous namespace instead of using a class. The intention came from feedback on #390

what's the motivation behind making node a pointer instead of a reference?

I'm assuming you are referencing: https://github.com/bitcoin-core/gui-qml/blob/f3b8c5a625a64ab490f9ba271c5310f988c5a0ae/src/qml/models/options_model.cpp#L27

if so the logic there came from having to initiate the options_model in qml/bitcoin.cpp with the node as a nullptr. And this was a way for it to work with the least amount of changes.

If you check the code in core/ qt it's always defined as &. Is there an issue you are trying to solve or just syntax or semantic?).

Yes in qt/bitcoin.cpp the OptionsModel is initiated through a reference because node creation doesn't happen until the splashscreen/intro have gone through and no settings are being set with the OptionsModel.

On the other hand in qml/bitcoin.cpp the options_model is needed for onboarding so initiating it with a nullptr is allowing the QML context property to be set without Node initiation.

D33r-Gee commented 1 week ago

Thanks @MarnixCroes for testing and @hebasto for the comments.

Will update once more with cleaner and clearer commits...

Also working on the Android version... Found a workaround for QSettings not working with Android, will post Android related commits soon...

D33r-Gee commented 1 week ago

This update 2cf64dd introduces Android functionality by creating qml/androidcustomdatadir files. This allows the app to retrieve the custom data directory path, if previously set by the user.