Skip to content

Instantiate NodeModel in qml#38

Closed
promag wants to merge 5 commits into
bitcoin-core:mainfrom
promag:2021-09-qml-nodemodel
Closed

Instantiate NodeModel in qml#38
promag wants to merge 5 commits into
bitcoin-core:mainfrom
promag:2021-09-qml-nodemodel

Conversation

@promag
Copy link
Copy Markdown
Contributor

@promag promag commented Sep 28, 2021

Based on #37, this change moves NodeModel instantiation to QML. It is instanced only when InitExecutor is ready.

The QML engine is specialized in order to provide access to interfaces::Node instance and possible others.

A nice follow-up is to improve how InitExecutor is exposed in QML.

@promag promag mentioned this pull request Sep 28, 2021
@promag promag changed the title qml: Instantiate NodeModel in qml Instantiate NodeModel in qml Sep 28, 2021
@promag promag force-pushed the 2021-09-qml-nodemodel branch from 3d58d92 to 54924fc Compare September 28, 2021 12:07
Copy link
Copy Markdown
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.

I think the "qml: Update block tip in the gui thread" commit (54924fc) is valuable by its own as NodeModel::setBlockTipHeight emits the blockTipHeightChanged signal. Maybe put it in its own PR?

Also is it worth to assert(QThread::currentThread() == thread()); in each function which emits signals?

Comment thread src/qml/pages/stub.qml
Comment on lines +25 to +42
NodeModel {
id: node_model
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does instantiating an object in QML have performance penalties comparing to instantiating it in C++ code?

@promag promag force-pushed the 2021-09-qml-nodemodel branch from 54924fc to c257c92 Compare October 25, 2021 12:30
@promag promag force-pushed the 2021-09-qml-nodemodel branch from c257c92 to cf8a278 Compare November 19, 2021 12:17
hebasto added a commit that referenced this pull request Oct 25, 2022
eca5252 qml: Update block tip in the gui thread (João Barbosa)

Pull request description:

  This takes commit cf8a278 from #38 and rebases it over changes on master; namely that we've dropped `GUIUtil::ObjectInvoke` in favor of `QMetaObject::invokeMethod`.

  This avoids non-thread-safe errors. I ran into a segfault here while running some data intensive monkey-code related to blocks, this change prevented it. I'm not going to actually use the code that caused the segfault, just to illustrate that this is useful here anyways.

  [![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/177)
  [![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/177)
  [![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/177)
  [![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/177)

ACKs for top commit:
  hebasto:
    ACK eca5252, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 9faf9b486af15f29ace20c9cb55ba65f3e20d23f4b7dc58908589f9a104b47cebe575a2a88935ed5f1a2fafc8bc41088084f28803990db64715e235da4c6461f
@promag promag closed this Aug 31, 2023
johnny9 pushed a commit to johnny9/bitcoin-core-app that referenced this pull request Jul 4, 2025
a8a7291 qml: Update block tip in the gui thread (João Barbosa)

Pull request description:

  This takes commit bitcoin-core/gui-qml@cf8a278 from bitcoin-core/gui-qml#38 and rebases it over changes on master; namely that we've dropped `GUIUtil::ObjectInvoke` in favor of `QMetaObject::invokeMethod`.

  This avoids non-thread-safe errors. I ran into a segfault here while running some data intensive monkey-code related to blocks, this change prevented it. I'm not going to actually use the code that caused the segfault, just to illustrate that this is useful here anyways.

  [![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/177)
  [![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/177)
  [![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/177)
  [![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/177)

ACKs for top commit:
  hebasto:
    ACK a8a7291, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 9faf9b486af15f29ace20c9cb55ba65f3e20d23f4b7dc58908589f9a104b47cebe575a2a88935ed5f1a2fafc8bc41088084f28803990db64715e235da4c6461f
tx-signer450 added a commit to tx-signer450/gui-qml that referenced this pull request Oct 20, 2025
a8a7291f744c0dc8c2bc9cf51766a4216b802fb9 qml: Update block tip in the gui thread (João Barbosa)

Pull request description:

  This takes commit bitcoin-core/gui-qml@cf8a278 from bitcoin-core/gui-qml#38 and rebases it over changes on master; namely that we've dropped `GUIUtil::ObjectInvoke` in favor of `QMetaObject::invokeMethod`.

  This avoids non-thread-safe errors. I ran into a segfault here while running some data intensive monkey-code related to blocks, this change prevented it. I'm not going to actually use the code that caused the segfault, just to illustrate that this is useful here anyways.

  [![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/177)
  [![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/177)
  [![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/177)
  [![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/177)

ACKs for top commit:
  hebasto:
    ACK a8a7291f744c0dc8c2bc9cf51766a4216b802fb9, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 9faf9b486af15f29ace20c9cb55ba65f3e20d23f4b7dc58908589f9a104b47cebe575a2a88935ed5f1a2fafc8bc41088084f28803990db64715e235da4c6461f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants