-
Notifications
You must be signed in to change notification settings - Fork 50
Introduce OptionsModel backend, Wire up Storage Amount settings to backend #207
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
Conversation
src/qml/bitcoin.cpp
Outdated
@@ -185,6 +186,9 @@ int QmlGuiMain(int argc, char* argv[]) | |||
|
|||
engine.rootContext()->setContextProperty("nodeModel", &node_model); | |||
|
|||
OptionsQmlModel options_model{*node}; | |||
engine.rootContext()->setContextProperty("options", &options_model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want a more specfic variable name here since its registered to the root to prevent possible conflicts with other property names. "optionsModel" maybe.
Also, I can't get my head around how this is working now. When hebasto did the previous PR setting a rootContext like this wasn't accessible in any of the sub components but this one is working. All I can think of is that it has something to do with how the StackView in main.qml is implemented now. Possibly due to the Onboarding components being statically defined instead of loading in the files with the Wizard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In debug.log, I can observe a few TypeErrors.
% cat debug.log | grep GUI
2022-12-20T13:45:16Z GUI: qrc:/qml/components/StorageSettings.qml:24: TypeError: Cannot read property 'pruneSizeGB' of null
2022-12-20T13:45:16Z GUI: qrc:/qml/components/StorageSettings.qml:16: TypeError: Cannot read property 'prune' of null
The reason for this TypeError is that the OptionsQmlModel
is initialized after the engine.
And since objects are destroyed in reverse order of how they are initialized, the OptionsQmlModel
is getting destroyed before the QML engine.
The same TypeErrors existed in #184, as pointed out by @johnny9 here.
So you have to initialize the OptionsQmlModel
before the engine to solve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will address both of these soon
Rebase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one bug in the code due to which the OptionsButton and Setting are not correctly connected.
Steps to reproduce:
- Start GUI.
- Select the "no prune" option.
- Complete onboarding.
- Restart GUI.
Observation:
- The OptionsButton has reset back to the "prune" option, but the prune option is deselected in the detailed setting.
Recording:
Recording.mp4
@shaavan technically that can be addressed, but the issue here really is that we just haven't introduced detection of whether or not it's the first time that the user is running the gui. If there is a settings.json in the data directory we're looking in we wouldn't be showing the onboarding process. |
12cbdc9
to
d72ac4c
Compare
I've re-run CI. |
src/qml/bitcoin.cpp
Outdated
@@ -185,6 +186,9 @@ int QmlGuiMain(int argc, char* argv[]) | |||
|
|||
engine.rootContext()->setContextProperty("nodeModel", &node_model); | |||
|
|||
OptionsQmlModel options_model{*node}; | |||
engine.rootContext()->setContextProperty("options", &options_model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
engine.rootContext()->setContextProperty("options", &options_model); | |
engine.rootContext()->setContextProperty("optionsModel", &options_model); |
@@ -19,11 +19,18 @@ ColumnLayout { | |||
description: qsTr("Uses about 2GB. For simple wallet use.") | |||
recommended: true | |||
checked: true | |||
onClicked: { | |||
options.prune = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.prune = true | |
optionsModel.prune = true |
@@ -19,11 +19,18 @@ ColumnLayout { | |||
description: qsTr("Uses about 2GB. For simple wallet use.") | |||
recommended: true | |||
checked: true | |||
onClicked: { | |||
options.prune = true | |||
options.pruneSizeGB = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.pruneSizeGB = 2 | |
optionsModel.pruneSizeGB = 2 |
d72ac4c
to
cc96bd1
Compare
updated from d72ac4c to cc96bd1, compare Changes:
|
Please rebase :) |
Wires both the easy and advanced configuration options related to storage amount to the options model backend. Additionally, this removes an unused and unwanted storage location setting from the advanced storage amount settings table.
cc96bd1
to
63162b0
Compare
Updated from cc96bd1 to 63162b0 Changes: rebased over main A note on implications of setting the default prune values when the page is loaded and in relation to #219: If we have an approach where we are only loading one page at a time, that means if a user goes back or forwards, the page gets loaded again. This means that if the user had configured a prune target of 8gb as a custom setting, continued on to the next page then decided to go back and change the prune target, the prune target would be reset to 2gb. This can be mitigated with detection of settings already set, which is needed anyways. Alternatively, the logic here can be changed into a form, where when the user finally finishes onboarding, then the settings get written. This is probably preferable anyways, for several cases such as quiting the application during onboarding; if we write settings, we wouldn't show onboarding again, but if we only write settings when onboarding is finished, the onboarding wizard will show on subsequent gui startups until the user completes onboarding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 63162b0
Built and tested. Changes look good.
Github-Pull: bitcoin-core#207 Rebased-From: 4c6562e
Github-Pull: bitcoin-core#207 Rebased-From: 93f78f8
Wires both the easy and advanced configuration options related to storage amount to the options model backend. Additionally, this removes an unused and unwanted storage location setting from the advanced storage amount settings table. Github-Pull: bitcoin-core#207 Rebased-From: 63162b0
Github-Pull: bitcoin-core#207 Rebased-From: 4c6562e
Github-Pull: bitcoin-core#207 Rebased-From: 93f78f8
Wires both the easy and advanced configuration options related to storage amount to the options model backend. Additionally, this removes an unused and unwanted storage location setting from the advanced storage amount settings table. Github-Pull: bitcoin-core#207 Rebased-From: 63162b0
Github-Pull: bitcoin-core#207 Rebased-From: 4c6562e
Github-Pull: bitcoin-core#207 Rebased-From: 93f78f8
Wires both the easy and advanced configuration options related to storage amount to the options model backend. Additionally, this removes an unused and unwanted storage location setting from the advanced storage amount settings table. Github-Pull: bitcoin-core#207 Rebased-From: 63162b0
This re-uses #184, with changes, to introduce an OptionsModel backend, while also wiring the Storage Amount settings; as the first settings to be wired to the options backend.
Both the "easy configuration" and "advanced configuration" methods for configuring the storage amount are wired to the backend. While on the fifth view of the onboarding wizard, the view with the header "Storage", if you click on the "Reduce Storage" button, your
Settings.json
file will show:and if you click on the "Store all data" button, your
Settings.json
file will show:Within "Detailed Settings", you can examine that the prune target specified within the
Settings.json
file will only be non-zero if a non-zero value is set in storage limit and if the "Store recent blocks only" switch is enabled, as it should be.The values you configure while onboarding will then be visible within the NodeSettings view when you click on "Storage".
Note: This doesn't complete the entire feature set around the setting, enabling, or disabling of this feature. There is still work to be done, this is the initial introduction and wiring.
The screenshots below highlight the functionality of this PR.