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

cxx-qt-gen: use impl cxx_qt::QObject<T> block for invokables #178

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

ahayzen-kdab
Copy link
Collaborator

This helps later as the invokables are implemented onto a different
struct, so this clearly defines the split between C++ and Rust
methods etc.

@ahayzen-kdab
Copy link
Collaborator Author

@LeonMatthesKDAB did we decide if we were going to call this impl Qt<T> or impl cxx_qt<T> ? As i think the Qt felt more natural for saying these are "Qt" invokables, but the cxx_qt was better to avoid clashes ? Maybe even cxx_qt::invokables could be considered as that then matches cxx_qt::signals etc ?

cxx-qt-gen/src/extract.rs Outdated Show resolved Hide resolved
@ahayzen-kdab
Copy link
Collaborator Author

@LeonMatthesKDAB did we decide if we were going to call this impl Qt<T> or impl cxx_qt<T> ? As i think the Qt felt more natural for saying these are "Qt" invokables, but the cxx_qt was better to avoid clashes ? Maybe even cxx_qt::invokables could be considered as that then matches cxx_qt::signals etc ?

So i'm wondering if this is the route (assuming valid syntax)

impl cxx_qt::invokables<MyObject> {
  #[invokable]
  pub fn my_invokable(self: Pin<&mut Self>) {

  }
}

impl MyObject {
  pub fn rust_only_method(&self) {

  }
}

@LeonMatthesKDAB where did our discussion end up on having like #[invokable(return-as-value)] and #[property(as-value)] stuff for UniquePtr end up? I can't see it now as there were options like #[invokable(return QColor)] as well...

@Be-ing Be-ing requested a review from LeonMatthesKDAB August 2, 2022 15:07
@ahayzen-kdab ahayzen-kdab marked this pull request as draft August 2, 2022 15:45
@ahayzen-kdab ahayzen-kdab force-pushed the use-impl-block-for-invokables branch from b11a3e8 to b1f6009 Compare August 2, 2022 17:06
@LeonMatthesKDAB
Copy link
Collaborator

So i'm wondering if this is the route (assuming valid syntax)

impl cxx_qt::invokables<MyObject> {
  #[invokable]
  pub fn my_invokable(self: Pin<&mut Self>) {

  }
}

impl MyObject {
  pub fn rust_only_method(&self) {

  }
}

We shouldn't use impl invokables<MyObject>, as it still makes sense to have non-invokable (aka not to C++ exposed) methods defined within the impl Qt<MyObject> (or whatever we end up calling it).
Whilst you can define Rust-only methods directly on MyObject, you might want to have a private method that is still capable of emitting a Signal for example. That you would implement on Qt<MyObject> but without #[invokable].

But regarding the cxx_qt:: prefix. That does make sense for consistencies sake.
So something like: impl cxx_qt::QObject<MyObject> or impl cxx_qt::Qt<MyObject>. My local playground project even uses impl cxx_qt<MyObject>, but maybe impl cxx_qt::Qt<MyObject> is a bit more consistent with the other macros we have.
Later we could add the ability to add a use cxx_qt::Qt or use cxx_qt::prelude::*, to reduce the boilerplate.

@LeonMatthesKDAB where did our discussion end up on having like #[invokable(return-as-value)] and #[property(as-value)] stuff for UniquePtr end up? I can't see it now as there were options like #[invokable(return QColor)] as well...

I'm unsure whether we reached a consensus on that or whether we were still just discussing options 🤔
Do you need a decision on this for this PR already?

Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

Going in the right direction. A bit of clarification regarding the purposes of impl Qt<...> vs impl ... are still necessary :)

examples/qml_with_threaded_logic/src/lib.rs Show resolved Hide resolved
cxx-qt-gen/src/extract.rs Outdated Show resolved Hide resolved
book/src/qobject/rustobj_struct.md Outdated Show resolved Hide resolved
book/src/qobject/rustobj_struct.md Outdated Show resolved Hide resolved
@ahayzen-kdab ahayzen-kdab force-pushed the use-impl-block-for-invokables branch 3 times, most recently from e47236e to c4d0ee7 Compare August 3, 2022 12:12
@ahayzen-kdab ahayzen-kdab changed the title cxx-qt-gen: use impl Qt<T> block for invokables cxx-qt-gen: use impl cxx_qt::QObject<T> block for invokables Aug 3, 2022
@ahayzen-kdab ahayzen-kdab marked this pull request as ready for review August 3, 2022 12:13
cxx-qt-gen/src/extract.rs Show resolved Hide resolved
cxx-qt-gen/test_outputs/invokables.h Outdated Show resolved Hide resolved
cxx-qt-gen/test_outputs/invokables.rs Outdated Show resolved Hide resolved
cxx-qt/src/lib.rs Show resolved Hide resolved
examples/qml_with_threaded_logic/src/lib.rs Show resolved Hide resolved
@ahayzen-kdab ahayzen-kdab force-pushed the use-impl-block-for-invokables branch 3 times, most recently from 35ee3ca to 450abae Compare August 3, 2022 14:23
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

Just one typo, then we're good to go :)

book/src/qobject/rustobj_struct.md Outdated Show resolved Hide resolved
This helps later as the invokables are implemented onto a different
struct, so this clearly defines the split between C++ and Rust
methods etc.
@ahayzen-kdab ahayzen-kdab force-pushed the use-impl-block-for-invokables branch from 450abae to ec9f655 Compare August 3, 2022 14:54
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

LGTM

@ahayzen-kdab ahayzen-kdab enabled auto-merge (rebase) August 3, 2022 15:10
@ahayzen-kdab ahayzen-kdab merged commit 2b3b6bc into KDAB:main Aug 3, 2022
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.

3 participants