-
Notifications
You must be signed in to change notification settings - Fork 78
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-lib: Add binding for QQmlApplicationEngine::singletonInstance #1140
base: main
Are you sure you want to change the base?
cxx-qt-lib: Add binding for QQmlApplicationEngine::singletonInstance #1140
Conversation
void* | ||
qqmlapplicationengineSingletonInstance(QQmlApplicationEngine& engine, QAnyStringView uri, QAnyStringView typeName) | ||
{ | ||
return reinterpret_cast<void*>(engine.singletonInstance<QObject*>(uri, typeName)); |
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.
I kind of hate everything about this, but this somehow works. Suggestions appreciated, since I don't know of any other method supported in cxx-qt that has a template parameter.
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.
Hmm, once we have up/down casting and QObject already as a type #562 then this would be much easier. We would like this have this implemented in this cycle as it seems useful in multiple places.
It would be return a pointer to a QObject, then use the downcast_ptr method to get your original type back.
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.
Hm, right, this could be a decent way to solve this issue 🤔
02a5168
to
3a86460
Compare
1f6d532
to
55647b9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1140 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 71 71
Lines 11967 11967
=========================================
Hits 11967 11967 ☔ View full report in Codecov by Sentry. |
@@ -79,13 +82,33 @@ mod ffi { | |||
) -> Pin<&mut QQmlEngine>; | |||
} | |||
|
|||
#[cfg(any(cxxqt_qt_version_at_least_7, cxxqt_qt_version_at_least_6_5))] | |||
unsafe extern "C++" { |
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.
This #[cfg] doesn't work I guess, so Qt5 and <6.5 builds will fail. A possible solution could be to separate this into a different cxx::bridge as suggested in this issue but that seems extreme?
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 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.
That does work, but the QAnyStringView inclusion is hitting the same problems as seen in #1141 :(
if ptr.is_null() { | ||
return None; | ||
} | ||
Some(&mut *(ptr as *mut T)) |
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.
Hm, this is definitely not safe to do here!
As you could basically "spoof" T to be different from the actual type that the singleton has.
In the most extreme case, I could just do:
engine.singleton_instance::<i32>(...);
We could mark the function itself as unsafe
to move that responsibility up one level, but it's still not great...
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.
I have added some lifetime checks to improve it a bit, but yes this is inherently unsafe due to a lack of type-checking. Note that once we have the ability to do type checking (like in #562, or akiselev's comment), this should stay unsafe.
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.
I'll leave it up to your discretion whether we should include this inherently-unsafe version now, and then have a proper solution in tandem with #562 or engineer it all here and right now. IMO, it would be really useful even in it's current state :-)
For what it's worth, I implemented this using a template helper: template <typename T>
T *qqmlengineSingletonInstance(QQmlEngine &engine, const QString &module, const QString &name)
{
auto ptr =
engine.template singletonInstance<T *>(
QAnyStringView(module), QAnyStringView(name));
return ptr;
} Then in the C++ bridge: #[rust_name = "get_theme_singleton"]
unsafe fn qqmlengineSingletonInstance(
engine: Pin<&mut QQmlEngine>,
module: &QString,
name: &QString,
) -> *mut ThemeSingleton; and finally the safe Rust wrapper: fn get_theme_singleton(self: Pin<&mut Self>) -> Option<Pin<&mut ThemeSingleton>> {
unsafe {
// This is safe because we're checking if it's null and only handing out a pinned mut ref
// Qt does its own type safety checks here too
let ptr = get_theme_singleton(
self.as_qqmlengine()
&QString::from("App.Core"),
&QString::from("Theme"),
);
ptr.as_mut().map(|p| Pin::new_unchecked(p))
}
} Then wrap it in a pair of traits: pub trait GetSingleton<T> {
fn try_singleton(self: Pin<&mut Self>) -> Option<Pin<&mut T>>;
fn get_singleton(self: Pin<&mut Self>) -> Pin<&mut T> {
self.try_singleton().expect("Singleton is not initialized")
}
}
pub trait SingletonType {
unsafe fn get_instance(engine: Pin<&mut QQmlEngine>) -> *mut Self;
}
impl<T: SingletonType> GetSingleton<T> for QQmlEngine {
fn try_singleton(self: Pin<&mut Self>) -> Option<Pin<&mut T>> {
unsafe {
// This is safe because we're checking if it's null and only handing out a pinned mut ref
// Qt does its own type safety checks here too
let ptr = T::get_instance(self);
ptr.as_mut().map(|p| Pin::new_unchecked(p))
}
}
}
impl SingletonType for ThemeSingleton {
unsafe fn get_instance(engine: Pin<&mut QQmlEngine>) -> *mut Self {
ffi::get_theme_instance(
engine,
&QString::from("App.Core"),
&QString::from("Theme"),
)
}
} |
This allows accessing QObject singleton instances registered in the QML engine (using #[qml_singleton]) from the Rust side.
55647b9
to
892f14f
Compare
This allows accessing QObject singleton instances registered in the QML engine (using #[qml_singleton]) from the Rust side.