Skip to content

Introduce OptionsQmlModel class to bind node settings with GUI #184

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

Closed
wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Oct 26, 2022

This PR adds only a basic implementation with a support for the -prune setting only.

@hebasto
Copy link
Member Author

hebasto commented Oct 26, 2022

cc @jarolrod

@johnny9
Copy link
Collaborator

johnny9 commented Oct 27, 2022

I see

2022-10-27T18:02:45Z GUI: qrc:/qml/components/StorageSettings.qml:24: TypeError: Cannot read property 'pruneSizeGB' of null
2022-10-27T18:02:45Z GUI: qrc:/qml/components/StorageSettings.qml:16: TypeError: Cannot read property 'prune' of null

in debug.log

@johnny9
Copy link
Collaborator

johnny9 commented Oct 27, 2022

I believe you need to use qmlRegisterSingletonType and then import the registered module in order to access the object in the StorageSettings component. It seems only the root component has access to the object when using setContextProperty.

Edit: Here's a potential patch
https://gist.github.com/johnny9/0534358700749a8b5cb2008606d27cbc

A caveat with this, as Jarol mentioned in a previous PR, is that this way to register objects to the QML engine is outdated as of 5.15 but we might need some improvements to the qmake or cmake build to support the new registration macros. https://www.qt.io/blog/qml-type-registration-in-qt-5.15

Edit2: With the above patch applied, I see "prune" property being set in settings.json and 2022-10-27T20:27:34Z Setting file arg: prune = 4768 in debug.log but I also see 2022-10-27T20:27:35Z Setting NODE_NETWORK on non-prune mode. I'm not familiar enough with the details of setting pruning to know if that means it worked.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK

The implementation looks standard. But as @johnny9 suggested, we can experiment using the qmlRegisterSingletonType.

@hebasto
I know you are currently occupied with finishing the CMake project, so I would like to share the burden of the work with you.
I want to work forward on this branch if that's okay with you. Thank you.

@jarolrod
Copy link
Member

@shaavan @johnny9 registering with the qml macro makes qml responsible for the lifecycle of the optionsmodel, we wouldn't want that. The way around this would be to subclass the qml engine, but there's no strong use-case for that right now.

@johnny9
Copy link
Collaborator

johnny9 commented Nov 14, 2022

Is that something unique to using the macro? Is there not an equivalent of qmlRegisterSingletonInstance using the macros to allow you to manage the lifecycle yourself?

@johnny9
Copy link
Collaborator

johnny9 commented Nov 14, 2022

Ok I found what you were referring to here https://doc-snapshots.qt.io/qt5-5.15/qqmlengine.html#QML_SINGLETON. Sounds like we should continue to use qmlRegisterSingletonInstance to override ownership of the object if we decide to use the macros.

@jarolrod
Copy link
Member

@johnny9 seems that you are correct in suggesting we use qmlRegisterSingletonInstance for many reasons including the performance hit of context properties and the non-constant lookup time. The ownership of the object can stay with c++ and not with the qml engine by specifying the QQmlEngine::CppOwnership flag.

@hebasto hebasto closed this Nov 16, 2022
hebasto added a commit that referenced this pull request Jan 17, 2023
…ttings to backend

63162b0 qml: Wire Storage amount options to OptionsModel backend (jarolrod)
93f78f8 qml: Add "-prune" settings to `OptionsQmlModel` class (Hennadii Stepanov)
4c6562e qml, build: Add `OptionsQmlModel` class with basic implementation (Hennadii Stepanov)

Pull request description:

  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:

  ```
  {
      "prune": 1907
  }
  ```

  and if you click on the "Store all data" button, your `Settings.json` file will show:

  ```
  {
      "prune": 0
  }
  ```

  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.
  | configuring while onboarding | correct values available within the node settings window |
  | ---------------------------- | -------------------------------------------------------- |
  | <img width="752" alt="Screen Shot 2022-12-14 at 4 32 48 AM" src="https://user-images.githubusercontent.com/23396902/207559204-76eeac23-8d6d-4234-8f97-122dc58b2bd5.png"> |  <img width="752" alt="Screen Shot 2022-12-14 at 4 32 57 AM" src="https://user-images.githubusercontent.com/23396902/207559250-6d77b672-7d28-4743-9d08-138dcc152c8d.png"> |

  [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/207)
  [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/207)
  [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/207)
  [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/207)

ACKs for top commit:
  johnny9:
    ACK 63162b0

Tree-SHA512: 2c7559434b8db1bd659fbf91ebfe8c99a709d9e2f438d00c5eff3d9d483c0087a31022f0e20dc4942e25fcdfef65e545714d380e645df50c3c2d5d834db61b38
@hebasto hebasto deleted the 221026-options branch January 17, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants