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

Expose init executor instance #37

Closed
wants to merge 1 commit into from

Conversation

promag
Copy link
Contributor

@promag promag commented Sep 26, 2021

This PR exposes InitExecutor instance to QML so that we can:

  • show the window as early as possible
  • display loading feedback on that window
  • connect/bind in QML to the relevant object's properties/signals to update the UI

The InitExecutor is then kept in the main GUI thread (otherwise QML engine complains) and the instance is exposed in the root engine context.

This change also removes the "proxy role" of NodeModel. Further simplifications/improvements will follow, but I'd like to present this approach first before going forward.

@promag
Copy link
Contributor Author

promag commented Sep 26, 2021

Related bitcoin-core/gui#434

@hebasto
Copy link
Member

hebasto commented Sep 27, 2021

Concept ACK.

@promag promag force-pushed the 2021-09-initexecutor branch 2 times, most recently from b234d28 to 7ce287c Compare September 27, 2021 21:58
@hebasto
Copy link
Member

hebasto commented Oct 2, 2021

Concept ACK.

To elaborate, I'm "Concept ACK" on:

  • show the window as early as possible

  • display loading feedback on that window

But I'm not sure it is required to expose an InitExecutor instance to achieve these goals.

A relevant discussion in bitcoin-core/gui#434:

On of the main goals of starting this project is involving Bitcoin Design community in the development process from the day 0. And to achieve it, keeping QML files as simple as possible is important for the project.

As were mentioned in #31, with the current approach:

  • QML files are easy to read, reason about, and modify. Designers could easy work on them

Anyway, as bitcoin-core/gui#434 is merged, this PR requires rebasing on top of the updated sync with the main repo that is currently blocked by #41.

Copy link
Contributor Author

@promag promag left a comment

Choose a reason for hiding this comment

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

Currently the designer is already exposed to nodeModel and nodeModel.startNodeInitializionThread(). And eventually it will have to be exposed to more stuff since that's the way QML works.

This change at least removes the wiring from InitExecutor and NodeModel.

Follow-ups can improve how QML deals with the application state.

@hebasto
Copy link
Member

hebasto commented Oct 3, 2021

Currently the designer is already exposed to nodeModel and nodeModel.startNodeInitializionThread().

It is all about the same object :)

And eventually it will have to be exposed to more stuff since that's the way QML works.

"more stuff" could remain the same single nodeModel object.

This change at least removes the wiring from InitExecutor and NodeModel.

But it exposes another object into QML code.

Having the only object, which is exposed to the QML, has a simple benefit of a guarantee that its internal state, i.e. internal state of the C++ backend, is consistent at anytime.

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