From 1ffe9743a78cb5e350465a19beb8bf7e355fbbfb Mon Sep 17 00:00:00 2001 From: Andrew Hayzen Date: Mon, 12 Jun 2023 14:34:13 +0100 Subject: [PATCH] cxx-qt-gen: do not generate getter/setter for fields anymore As we move qproperty to an attribute we need to disable generating helpers for fields as we won't be able to find them when the struct is outside the bridge. There are three options going forwards - let the developer create their own getters/setters (this commit) - add a #[field(T, NAME)] as an attribute to the qobject later - remove the (un)safe for qproperty with notify so that rust_mut is safe Related to #559 --- crates/cxx-qt-gen/src/parser/qobject.rs | 8 ++- crates/cxx-qt-gen/test_outputs/inheritance.rs | 17 ------- crates/cxx-qt-gen/test_outputs/properties.rs | 51 ------------------- examples/demo_threading/rust/src/lib.rs | 10 ++-- examples/qml_features/rust/src/containers.rs | 45 +++++++++------- .../rust/src/custom_base_class.rs | 15 ++++-- examples/qml_features/rust/src/invokables.rs | 7 +-- examples/qml_features/rust/src/signals.rs | 7 +-- tests/basic_cxx_qt/rust/src/lib.rs | 11 ++-- tests/basic_cxx_qt/rust/src/locking.rs | 9 ++-- 10 files changed, 67 insertions(+), 113 deletions(-) diff --git a/crates/cxx-qt-gen/src/parser/qobject.rs b/crates/cxx-qt-gen/src/parser/qobject.rs index b6d4a16fd..10604dd91 100644 --- a/crates/cxx-qt-gen/src/parser/qobject.rs +++ b/crates/cxx-qt-gen/src/parser/qobject.rs @@ -99,7 +99,7 @@ impl ParsedQObject { // Parse any properties in the struct // and remove the #[qproperty] attribute - let (properties, fields) = Self::parse_struct_fields(&mut qobject_struct.fields)?; + let (properties, _) = Self::parse_struct_fields(&mut qobject_struct.fields)?; // Ensure that the QObject is marked as pub otherwise the error is non obvious // https://github.com/KDAB/cxx-qt/issues/457 @@ -119,7 +119,11 @@ impl ParsedQObject { inherited_methods: vec![], passthrough_impl_items: vec![], properties, - fields, + // Do not generate helpers for fields as they are going to move out of the bridge + // + // TODO: we may bring #[field(T, NAME)] as a way of doing this + // if we want to keep the unsafety vs safety of property changes with or without notify + fields: vec![], qml_metadata, locking: true, threading: false, diff --git a/crates/cxx-qt-gen/test_outputs/inheritance.rs b/crates/cxx-qt-gen/test_outputs/inheritance.rs index 113c90e62..1abeb41df 100644 --- a/crates/cxx-qt-gen/test_outputs/inheritance.rs +++ b/crates/cxx-qt-gen/test_outputs/inheritance.rs @@ -84,23 +84,6 @@ pub mod cxx_qt_inheritance { pub struct MyObject { data: Vec, } - impl MyObjectQt { - fn data(&self) -> &Vec { - &self.rust().data - } - } - impl MyObjectQt { - fn data_mut<'a>(self: Pin<&'a mut Self>) -> &'a mut Vec { - unsafe { &mut self.rust_mut().get_unchecked_mut().data } - } - } - impl MyObjectQt { - fn set_data(self: Pin<&mut Self>, value: Vec) { - unsafe { - self.rust_mut().data = value; - } - } - } impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; diff --git a/crates/cxx-qt-gen/test_outputs/properties.rs b/crates/cxx-qt-gen/test_outputs/properties.rs index 7a1774dd8..ab664a011 100644 --- a/crates/cxx-qt-gen/test_outputs/properties.rs +++ b/crates/cxx-qt-gen/test_outputs/properties.rs @@ -233,57 +233,6 @@ pub mod cxx_qt_ffi { self.connect_trivial_changed(func, CxxQtConnectionType::AutoConnection) } } - impl MyObjectQt { - fn opaque(&self) -> &UniquePtr { - &self.rust().opaque - } - } - impl MyObjectQt { - fn opaque_mut<'a>(self: Pin<&'a mut Self>) -> &'a mut UniquePtr { - unsafe { &mut self.rust_mut().get_unchecked_mut().opaque } - } - } - impl MyObjectQt { - fn set_opaque(self: Pin<&mut Self>, value: UniquePtr) { - unsafe { - self.rust_mut().opaque = value; - } - } - } - impl MyObjectQt { - fn private_rust_field(&self) -> &i32 { - &self.rust().private_rust_field - } - } - impl MyObjectQt { - fn private_rust_field_mut<'a>(self: Pin<&'a mut Self>) -> &'a mut i32 { - unsafe { &mut self.rust_mut().get_unchecked_mut().private_rust_field } - } - } - impl MyObjectQt { - fn set_private_rust_field(self: Pin<&mut Self>, value: i32) { - unsafe { - self.rust_mut().private_rust_field = value; - } - } - } - impl MyObjectQt { - pub fn public_rust_field(&self) -> &f64 { - &self.rust().public_rust_field - } - } - impl MyObjectQt { - pub fn public_rust_field_mut<'a>(self: Pin<&'a mut Self>) -> &'a mut f64 { - unsafe { &mut self.rust_mut().get_unchecked_mut().public_rust_field } - } - } - impl MyObjectQt { - pub fn set_public_rust_field(self: Pin<&mut Self>, value: f64) { - unsafe { - self.rust_mut().public_rust_field = value; - } - } - } impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; diff --git a/examples/demo_threading/rust/src/lib.rs b/examples/demo_threading/rust/src/lib.rs index 79c8dd5ed..b18432358 100644 --- a/examples/demo_threading/rust/src/lib.rs +++ b/examples/demo_threading/rust/src/lib.rs @@ -105,7 +105,7 @@ use uuid::Uuid; impl ffi::EnergyUsageQt { /// A Q_INVOKABLE that returns the current power usage for a given uuid fn sensor_power(self: Pin<&mut Self>, uuid: &QString) -> f64 { - let sensors = SensorsWorker::read_sensors(self.sensors_map_mut()); + let sensors = SensorsWorker::read_sensors(&unsafe { self.rust_mut() }.sensors_map); if let Ok(uuid) = Uuid::parse_str(&uuid.to_string()) { sensors.get(&uuid).map(|v| v.power).unwrap_or_default() @@ -129,16 +129,16 @@ impl ffi::EnergyUsageQt { let sensors_changed = Arc::new(AtomicBool::new(false)); // Make relevent clones so that we can pass them to the threads - let accumulator_sensors = Arc::clone(self.as_mut().sensors_map_mut()); + let accumulator_sensors = Arc::clone(&unsafe { self.as_mut().rust_mut() }.sensors_map); let accumulator_sensors_changed = Arc::clone(&sensors_changed); let accumulator_qt_thread = self.qt_thread(); - let sensors = Arc::clone(self.as_mut().sensors_map_mut()); + let sensors = Arc::clone(&unsafe { self.as_mut().rust_mut() }.sensors_map); let sensors_qt_thread = self.qt_thread(); - let timeout_sensors = Arc::clone(self.as_mut().sensors_map_mut()); + let timeout_sensors = Arc::clone(&unsafe { self.as_mut().rust_mut() }.sensors_map); let timeout_network_tx = network_tx.clone(); // Start our threads - *self.join_handles_mut() = Some([ + unsafe { self.rust_mut() }.join_handles = Some([ // Create a TimeoutWorker // If a sensor is not seen for N seconds then a disconnect is requested std::thread::spawn(move || { diff --git a/examples/qml_features/rust/src/containers.rs b/examples/qml_features/rust/src/containers.rs index 9b62389c4..e202d0b71 100644 --- a/examples/qml_features/rust/src/containers.rs +++ b/examples/qml_features/rust/src/containers.rs @@ -53,7 +53,7 @@ pub mod ffi { pub(crate) list: QList_i32, // Expose as a Q_PROPERTY so that QML tests can ensure that QVariantMap works #[qproperty] - map: QMap_QString_QVariant, + pub(crate) map: QMap_QString_QVariant, pub(crate) set: QSet_i32, pub(crate) vector: QVector_i32, } @@ -86,6 +86,7 @@ pub mod ffi { } use core::pin::Pin; +use cxx_qt::CxxQtType; use cxx_qt_lib::{ QHash, QHashPair_QString_QVariant, QList, QMap, QMapPair_QString_QVariant, QSet, QString, QVariant, QVector, @@ -96,41 +97,44 @@ use cxx_qt_lib::{ impl ffi::RustContainersQt { /// Reset all the containers fn reset(mut self: Pin<&mut Self>) { - self.as_mut() - .set_hash(QHash::::default()); - self.as_mut().set_list(QList::::default()); - self.as_mut() - .set_map(QMap::::default()); - self.as_mut().set_set(QSet::::default()); - self.as_mut().set_vector(QVector::::default()); + // Update the private rust fields via the rust_mut + { + let mut rust_mut = unsafe { self.as_mut().rust_mut() }; + rust_mut.hash = QHash::::default(); + rust_mut.list = QList::::default(); + rust_mut.set = QSet::::default(); + rust_mut.vector = QVector::::default(); + } + + self.as_mut().set_map(QMap::::default()); self.update_strings(); } /// Append the given number to the vector container fn append_vector(mut self: Pin<&mut Self>, value: i32) { - self.as_mut().vector_mut().append(value); + unsafe { self.as_mut().rust_mut() }.vector.append(value); self.update_strings(); } /// Append the given number to the list container fn append_list(mut self: Pin<&mut Self>, value: i32) { - self.as_mut().list_mut().append(value); + unsafe { self.as_mut().rust_mut() }.list.append(value); self.update_strings(); } /// Insert the given number into the set container fn insert_set(mut self: Pin<&mut Self>, value: i32) { - self.as_mut().set_mut().insert(value); + unsafe { self.as_mut().rust_mut() }.set.insert(value); self.update_strings(); } /// Insert the given string and variant to the hash container fn insert_hash(mut self: Pin<&mut Self>, key: QString, value: QVariant) { - self.as_mut().hash_mut().insert(key, value); + unsafe { self.as_mut().rust_mut() }.hash.insert(key, value); self.update_strings(); } @@ -139,7 +143,7 @@ impl ffi::RustContainersQt { fn insert_map(mut self: Pin<&mut Self>, key: QString, value: QVariant) { // SAFETY: map is a Q_PROPERTY so ensure we manually trigger changed unsafe { - self.as_mut().map_mut().insert(key, value); + self.as_mut().rust_mut().map.insert(key, value); self.as_mut().map_changed(); } @@ -149,7 +153,8 @@ impl ffi::RustContainersQt { fn update_strings(mut self: Pin<&mut Self>) { let hash_items = self .as_ref() - .hash() + .rust() + .hash .iter() .map(|(key, value)| { let value = value.value::().unwrap_or(0); @@ -161,7 +166,8 @@ impl ffi::RustContainersQt { let list_items = self .as_ref() - .list() + .rust() + .list .iter() .map(|value| value.to_string()) .collect::>() @@ -170,7 +176,8 @@ impl ffi::RustContainersQt { let map_items = self .as_ref() - .map() + .rust() + .map .iter() .map(|(key, value)| { let value = value.value::().unwrap_or(0); @@ -182,7 +189,8 @@ impl ffi::RustContainersQt { let set_items = self .as_ref() - .set() + .rust() + .set .iter() .map(|value| value.to_string()) .collect::>() @@ -191,7 +199,8 @@ impl ffi::RustContainersQt { let vector_items = self .as_ref() - .vector() + .rust() + .vector .iter() .map(|value| value.to_string()) .collect::>() diff --git a/examples/qml_features/rust/src/custom_base_class.rs b/examples/qml_features/rust/src/custom_base_class.rs index 3a895344e..0cd4b78ca 100644 --- a/examples/qml_features/rust/src/custom_base_class.rs +++ b/examples/qml_features/rust/src/custom_base_class.rs @@ -207,16 +207,21 @@ impl ffi::CustomBaseClassQt { } fn add_cpp_context(mut self: Pin<&mut Self>) { - let count = self.vector().len(); + let count = self.as_ref().rust().vector.len(); unsafe { self.as_mut() .begin_insert_rows(&QModelIndex::default(), count as i32, count as i32); - let id = *self.id(); - self.as_mut().set_id(id + 1); + let id = self.as_ref().rust().id; + self.as_mut().rust_mut().id = id + 1; self.as_mut().vector_mut().push((id, (id as f64) / 3.0)); self.as_mut().end_insert_rows(); } } + + /// Safe helper to access mutate the vector field + pub fn vector_mut<'a>(self: Pin<&'a mut Self>) -> &'a mut Vec<(u32, f64)> { + &mut unsafe { self.rust_mut().get_unchecked_mut() }.vector + } } // ANCHOR: book_inherit_clear @@ -225,7 +230,7 @@ impl ffi::CustomBaseClassQt { pub fn clear(mut self: Pin<&mut Self>) { unsafe { self.as_mut().begin_reset_model(); - self.as_mut().set_id(0); + self.as_mut().rust_mut().id = 0; self.as_mut().vector_mut().clear(); self.as_mut().end_reset_model(); } @@ -250,7 +255,7 @@ impl ffi::CustomBaseClassQt { /// Remove the row with the given index pub fn remove(mut self: Pin<&mut Self>, index: i32) { - if index < 0 || (index as usize) >= self.vector().len() { + if index < 0 || (index as usize) >= self.as_ref().rust().vector.len() { return; } diff --git a/examples/qml_features/rust/src/invokables.rs b/examples/qml_features/rust/src/invokables.rs index 9a53c8740..e856c5411 100644 --- a/examples/qml_features/rust/src/invokables.rs +++ b/examples/qml_features/rust/src/invokables.rs @@ -76,9 +76,10 @@ impl ffi::RustInvokablesQt { /// Mutable C++ context method that helps to store the color fn store_helper(mut self: Pin<&mut Self>, red: f32, green: f32, blue: f32) { - self.as_mut().set_red(red); - self.as_mut().set_green(green); - self.as_mut().set_blue(blue); + let mut rust_mut = unsafe { self.as_mut().rust_mut() }; + rust_mut.red = red; + rust_mut.green = green; + rust_mut.blue = blue; } } // ANCHOR_END: book_invokable_impl diff --git a/examples/qml_features/rust/src/signals.rs b/examples/qml_features/rust/src/signals.rs index 37efe6e74..a78888f7f 100644 --- a/examples/qml_features/rust/src/signals.rs +++ b/examples/qml_features/rust/src/signals.rs @@ -63,6 +63,7 @@ pub mod ffi { } use core::pin::Pin; +use cxx_qt::CxxQtType; use cxx_qt_lib::{ConnectionType, QString, QUrl}; // TODO: this will change to qobject::RustSignals once @@ -92,7 +93,7 @@ impl ffi::RustSignalsQt { // Determine if logging is enabled if *qobject.as_ref().logging_enabled() { // If no connections have been made, then create them - if qobject.as_ref().connections().is_none() { + if qobject.as_ref().rust().connections.is_none() { // ANCHOR: book_signals_connect let connections = [ qobject.as_mut().on_connected(|_, url| { @@ -109,7 +110,7 @@ impl ffi::RustSignalsQt { ConnectionType::QueuedConnection, ), ]; - qobject.as_mut().set_connections(Some(connections)); + unsafe { qobject.as_mut().rust_mut() }.connections = Some(connections); // ANCHOR_END: book_signals_connect } } else { @@ -117,7 +118,7 @@ impl ffi::RustSignalsQt { // ANCHOR: book_signals_disconnect // By making connections None, we trigger a drop on the connections // this then causes disconnections - qobject.as_mut().set_connections(None); + unsafe { qobject.as_mut().rust_mut() }.connections = None; // ANCHOR_END: book_signals_disconnect } }) diff --git a/tests/basic_cxx_qt/rust/src/lib.rs b/tests/basic_cxx_qt/rust/src/lib.rs index 24bde2c5b..8c322f63b 100644 --- a/tests/basic_cxx_qt/rust/src/lib.rs +++ b/tests/basic_cxx_qt/rust/src/lib.rs @@ -84,15 +84,16 @@ impl ffi::MyObjectQt { fn queue_test(self: Pin<&mut Self>) { let qt_thread = self.qt_thread(); qt_thread - .queue(|ctx| { - *ctx.update_call_count_mut() += 1; + .queue(|qobject| { + unsafe { qobject.rust_mut() }.update_call_count += 1; }) .unwrap(); } fn queue_test_multi_thread(self: Pin<&mut Self>) { static N_THREADS: usize = 100; - static N_REQUESTS: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + static N_REQUESTS: std::sync::atomic::AtomicUsize = + std::sync::atomic::AtomicUsize::new(0); let mut handles = Vec::new(); let qt_thread = self.qt_thread(); @@ -100,8 +101,8 @@ impl ffi::MyObjectQt { let qt_thread_cloned = qt_thread.clone(); handles.push(std::thread::spawn(move || { qt_thread_cloned - .queue(|ctx| { - *ctx.update_call_count_mut() += 1; + .queue(|qobject| { + unsafe { qobject.rust_mut() }.update_call_count += 1; }) .unwrap(); N_REQUESTS.fetch_add(1, std::sync::atomic::Ordering::Relaxed); diff --git a/tests/basic_cxx_qt/rust/src/locking.rs b/tests/basic_cxx_qt/rust/src/locking.rs index 9a4eadaf2..10eea393c 100644 --- a/tests/basic_cxx_qt/rust/src/locking.rs +++ b/tests/basic_cxx_qt/rust/src/locking.rs @@ -42,19 +42,20 @@ pub mod ffi { } use core::pin::Pin; +use cxx_qt::CxxQtType; use std::{sync::atomic::Ordering, thread, time::Duration}; // TODO: this will change to qobject::RustLockingEnabled once // https://github.com/KDAB/cxx-qt/issues/559 is done impl ffi::RustLockingEnabledQt { fn get_counter(&self) -> u32 { - self.counter().load(Ordering::Acquire) + self.rust().counter.load(Ordering::Acquire) } fn increment(self: Pin<&mut Self>) { let counter = self.as_ref().get_counter(); thread::sleep(Duration::from_millis(100)); - self.counter().store(counter + 1, Ordering::Release); + self.rust().counter.store(counter + 1, Ordering::Release); } } @@ -62,12 +63,12 @@ impl ffi::RustLockingEnabledQt { // https://github.com/KDAB/cxx-qt/issues/559 is done impl ffi::RustLockingDisabledQt { fn get_counter(&self) -> u32 { - self.counter().load(Ordering::Acquire) + self.rust().counter.load(Ordering::Acquire) } fn increment(self: Pin<&mut Self>) { let counter = self.as_ref().get_counter(); thread::sleep(Duration::from_millis(100)); - self.counter().store(counter + 1, Ordering::Release); + self.rust().counter.store(counter + 1, Ordering::Release); } }