cxx-qt: remove handle_property_change instead rely on QML/C++

For now remove the handle_property_change this reduces complexity as a connection can be made on the QML/C++ side to an invokable. Later once properties live on the Rust side it might be possible to create connections on the Rust side of the bridge. Change-Id: I76db85f5f094964c1b0e543d8284b384b4767aa9
KDAB · Jul 14, 2022 · 911c057 · 911c057
1 parent 860c7e7
commit 911c057
Show file tree

Hide file tree

Showing 25 changed files with 40 additions and 315 deletions.
diff --git a/book/src/concepts/threading.md b/book/src/concepts/threading.md
@@ -15,8 +15,6 @@ This means that Rust code, such as invokables and handlers, which are directly c
 
 We provide a solution to prevent entering deadlocks from signal connections, eg if a property change signal was connected to an invokable on the C++/QML side this wouldn't be able to acquire a lock if the property change was triggered from a Rust invokable. The solution is to post events to a queue which could cause deadlocks, eg signal emisson, these are then executed once the next event loop occurs, and crucially, after the lock from the Rust invokable is released.
 
-If Rust code needs to listen to property changes, handlers can be implemented (eg PropertyChangeHandler) in the [RustObj Handlers](../qobject/handlers.md). These are called directly in the event loop from the Qt thread.
-
 <div style="background-color: white; padding: 1rem; text-align: center;">
 
 ![Threading Abstract](../images/threading_abstract.svg)

diff --git a/book/src/images/threading_abstract.dia b/book/src/images/threading_abstract.dia
diff --git a/book/src/images/threading_abstract.svg b/book/src/images/threading_abstract.svg
diff --git a/book/src/qobject/data_struct.md b/book/src/qobject/data_struct.md
@@ -19,10 +19,6 @@ Note that you can also use serde on the Data struct and derive `Deserialize` and
 
 If you want to provide default values for your QObject, then instead of deriving implement the `Default` trait for the struct `Data`.
 
-## Property enum
-
-An enum called `Property` is automatically generated from the names of the fields in the `Data` struct, this can then be used in the [`PropertyChangeHandler`](./handlers.md).
-
 ## Deserialisation or Serialisation
 
 Using [Serde](https://serde.rs/) the Data struct can be (de)seralised, by adding the dervive attributes as normal.

diff --git a/book/src/qobject/handlers.md b/book/src/qobject/handlers.md
@@ -11,21 +11,8 @@ Handlers are used to react to events on the Qt event loop thread. This allows Ru
 
 The following handlers are available
 
-  * PropertyChangeHandler to handle when a property value has changed
   * UpdateRequestHandler to process update requests on the Qt event loop thread, see [threading](../concepts/threading.md) for more info.
 
-## PropertyChangeHandler
-
-When a property defined in the [data struct](./data_struct.md) is changed, either via Rust calling a setter or via QML / C++ calling a setter, we can be notified of this change by using the `PropertyChangeHandler`.
-
-The example below listens to the number property and `handle_property_change` is triggered when the property `number` changes. It uses a `Property` enum which is automatically generated from the names of the properties defined in the [data struct](./data_struct.md).
-
-Note that this is called from the Qt event loop thread.
-
-```rust,ignore,noplayground
-{{#include ../../../examples/qml_features/src/handler_property_change.rs:book_macro_code}}
-```
-
 ## UpdateRequestHandler
 
 When a background Rust thread uses an `UpdateRequester` to request the Qt thread to synchronise via calling `request_update` this triggers the `handle_update_request` method of the `UpdateRequestHandler`.

diff --git a/cxx-qt-gen/src/extract.rs b/cxx-qt-gen/src/extract.rs
@@ -189,8 +189,6 @@ pub struct QObject {
     pub(crate) original_passthrough_decls: Vec<Item>,
     /// The Rust impl that has optionally been provided to handle updates
     pub(crate) handle_updates_impl: Option<ItemImpl>,
-    /// The Rust impl that has optionally been provided to handle property changes
-    pub(crate) handle_property_change_impl: Option<ItemImpl>,
 }
 
 /// Describe the error type from extract_qt_type and extract_type_ident
@@ -806,9 +804,6 @@ pub fn extract_qobject(
     // Determines if (and how) this object can respond to update requests
     let mut handle_updates_impl = None;
 
-    // Determines if (and how) this object can respond to property changes
-    let mut handle_property_change_impl = None;
-
     // Process each of the items in the mod
     for item in items.drain(..) {
         match item {
@@ -935,9 +930,6 @@ pub fn extract_qobject(
                                     "UpdateRequestHandler" => {
                                         handle_updates_impl = Some(original_impl.to_owned())
                                     }
-                                    "PropertyChangeHandler" => {
-                                        handle_property_change_impl = Some(original_impl.to_owned())
-                                    }
                                     _others => original_trait_impls.push(original_impl.to_owned()),
                                 }
                             } else {
@@ -1024,7 +1016,6 @@ pub fn extract_qobject(
         original_trait_impls,
         original_passthrough_decls,
         handle_updates_impl,
-        handle_property_change_impl,
     })
 }
 

diff --git a/cxx-qt-gen/src/gen_cpp.rs b/cxx-qt-gen/src/gen_cpp.rs
@@ -442,7 +442,6 @@ fn generate_invokables_cpp(
 fn generate_properties_cpp(
     struct_ident: &Ident,
     properties: &[Property],
-    has_property_change_handler: bool,
 ) -> Result<Vec<CppProperty>, TokenStream> {
     let mut items: Vec<CppProperty> = vec![];
 
@@ -513,24 +512,6 @@ fn generate_properties_cpp(
         // this is used for the owned member and extra pointer specific methods
         let parameter_ident_pascal = parameter.ident.to_case(Case::Pascal);
 
-        let call_property_change_handler = if has_property_change_handler {
-            formatdoc! {
-                r#"
-                const auto handlePropertySuccess = QMetaObject::invokeMethod(this, [&]() {{
-                    const std::lock_guard<std::mutex> guard(m_rustObjMutex);
-                    m_rustObj->handlePropertyChange(*this, Property::{ident});
-                }}, Qt::QueuedConnection);
-                Q_ASSERT(handlePropertySuccess);
-                "#,
-                ident = parameter_ident_pascal
-            }
-        } else {
-            "".to_owned()
-        };
-
-        // TODO: we eventually want to also support handling property changes inside subobjects
-        // by wiring up the relevant changed signals to m_rustObj->handlePropertyChange.
-
         // If we are a pointer type then add specific methods
         if parameter.type_ident.is_ptr() {
             // Pointers are stored in the C++ object, so build a member and owned ident
@@ -560,8 +541,6 @@ fn generate_properties_cpp(
 
                         const auto signalSuccess = QMetaObject::invokeMethod(this, "{ident_changed}", Qt::QueuedConnection);
                         Q_ASSERT(signalSuccess);
-
-                        {call_property_change_handler}
                     }}
                 }}
                 "#,
@@ -575,7 +554,6 @@ fn generate_properties_cpp(
                 member_owned_ident = member_owned_ident,
                 struct_ident = struct_ident.to_string(),
                 type_ident = type_ident,
-                call_property_change_handler = call_property_change_handler,
             });
 
             // Add members to the reference and own it
@@ -626,8 +604,6 @@ fn generate_properties_cpp(
 
                   const auto signalSuccess = QMetaObject::invokeMethod(this, "{ident_changed}", Qt::QueuedConnection);
                   Q_ASSERT(signalSuccess);
-
-                  {call_change_handler}
                 }}
                 "#,
                 ident_changed = ident_changed,
@@ -637,7 +613,6 @@ fn generate_properties_cpp(
                 member_owned_ident = member_owned_ident,
                 struct_ident = struct_ident.to_string(),
                 type_ident = type_ident,
-                call_change_handler = call_property_change_handler,
             ));
         } else {
             cpp_property.source.push(formatdoc! {
@@ -661,8 +636,6 @@ fn generate_properties_cpp(
 
                         const auto signalSuccess = QMetaObject::invokeMethod(this, "{ident_changed}", Qt::QueuedConnection);
                         Q_ASSERT(signalSuccess);
-
-                        {call_change_handler}
                     }}
                 }}
                 "#,
@@ -675,7 +648,6 @@ fn generate_properties_cpp(
                 struct_ident = struct_ident.to_string(),
                 type_ident = type_ident,
                 member_ident = format!("m_{}", parameter.ident),
-                call_change_handler = call_property_change_handler,
             });
 
             // Own the member on the C++ side
@@ -824,33 +796,29 @@ pub fn generate_qobject_cpp(obj: &QObject) -> Result<CppObject, TokenStream> {
     }
 
     // Build CppProperty's for the object, then drain them into our CppPropertyHelper
-    let properties = generate_properties_cpp(
-        &obj.ident,
-        &obj.properties,
-        obj.handle_property_change_impl.is_some(),
-    )?
-    .drain(..)
-    .fold(
-        CppPropertyHelper {
-            headers_includes: vec![],
-            headers_members: vec![],
-            headers_meta: vec![],
-            headers_public: vec![],
-            headers_signals: vec![],
-            headers_slots: vec![],
-            sources: vec![],
-        },
-        |mut acc, mut property| {
-            acc.headers_includes.append(&mut property.header_includes);
-            acc.headers_meta.append(&mut property.header_meta);
-            acc.headers_members.append(&mut property.header_members);
-            acc.headers_public.append(&mut property.header_public);
-            acc.headers_signals.append(&mut property.header_signals);
-            acc.headers_slots.append(&mut property.header_slots);
-            acc.sources.append(&mut property.source);
-            acc
-        },
-    );
+    let properties = generate_properties_cpp(&obj.ident, &obj.properties)?
+        .drain(..)
+        .fold(
+            CppPropertyHelper {
+                headers_includes: vec![],
+                headers_members: vec![],
+                headers_meta: vec![],
+                headers_public: vec![],
+                headers_signals: vec![],
+                headers_slots: vec![],
+                sources: vec![],
+            },
+            |mut acc, mut property| {
+                acc.headers_includes.append(&mut property.header_includes);
+                acc.headers_meta.append(&mut property.header_meta);
+                acc.headers_members.append(&mut property.header_members);
+                acc.headers_public.append(&mut property.header_public);
+                acc.headers_signals.append(&mut property.header_signals);
+                acc.headers_slots.append(&mut property.header_slots);
+                acc.sources.append(&mut property.source);
+                acc
+            },
+        );
 
     // A helper which allows us to flatten data from vec of invokables
     struct CppInvokableHelper {

diff --git a/cxx-qt-gen/src/gen_rs.rs b/cxx-qt-gen/src/gen_rs.rs
@@ -510,43 +510,19 @@ pub fn generate_qobject_cxx(
         quote! {}
     };
 
-    // Define a function to handle property changes if we have one
-    let handle_property_change = if obj.handle_property_change_impl.is_some() {
-        quote! {
-            #[cxx_name = "handlePropertyChange"]
-            fn call_handle_property_change(self: &mut #rust_class_name, cpp: Pin<&mut #class_name>, property: Property);
-        }
-    } else {
-        quote! {}
-    };
-
     // Build the import path for the C++ header
     let import_path = format!("cxx-qt-gen/include/{}.h", ident_snake);
 
-    // Generate an enum representing all the properties that the object has
-    let property_enum = generate_property_enum(obj);
-
     // TODO: ideally we only want to add the "type QString = cxx_qt_lib::QStringCpp;"
     // if we actually generate some code that uses QString.
 
     // Build the namespace string, rust::module
     let namespace = obj.namespace.join("::");
 
-    // Build the properties FFI
-    let property_ffi = if !obj.properties.is_empty() {
-        quote! {
-            pub type Property = ffi::Property;
-        }
-    } else {
-        quote! {}
-    };
-
     // Build the CXX bridge
     let output = quote! {
         #[cxx::bridge(namespace = #namespace)]
         mod ffi {
-            #property_enum
-
             unsafe extern "C++" {
                 include!(#import_path);
 
@@ -601,12 +577,10 @@ pub fn generate_qobject_cxx(
                 fn initialise_cpp(cpp: Pin<&mut #class_name>);
 
                 #handle_update_request
-                #handle_property_change
             }
         }
 
         pub type FFICppObj = ffi::#class_name;
-        #property_ffi
     };
 
     Ok(output.into_token_stream())
@@ -627,25 +601,6 @@ fn generate_cpp_object_initialiser(obj: &QObject) -> TokenStream {
     output.into_token_stream()
 }
 
-/// Generate an enum representing all the properties that a QObject has
-fn generate_property_enum(obj: &QObject) -> TokenStream {
-    let properties = obj.properties.iter().map(|property| {
-        let ident_str = &property.ident.rust_ident.to_string();
-        let ident = format_ident!("{}", ident_str.to_case(Case::Pascal));
-        quote! { #ident }
-    });
-
-    if properties.len() > 0 {
-        quote! {
-            enum Property {
-                #(#properties),*
-            }
-        }
-    } else {
-        quote! {}
-    }
-}
-
 fn generate_property_methods_rs(obj: &QObject) -> Result<Vec<TokenStream>, TokenStream> {
     // Build a list of property methods impls
     let mut property_methods = Vec::new();
@@ -1088,26 +1043,13 @@ pub fn generate_qobject_rs(
         quote! {}
     };
 
-    // Define a function to handle property changes if we have one
-    let handle_property_change = if obj.handle_property_change_impl.is_some() {
-        quote! {
-            fn call_handle_property_change(&mut self, cpp: std::pin::Pin<&mut FFICppObj>, property: Property) {
-                let mut cpp = CppObj::new(cpp);
-                self.handle_property_change(&mut cpp, property);
-            }
-        }
-    } else {
-        quote! {}
-    };
-
     let rust_struct_impl = quote! {
         impl #rust_class_name {
             #(#invokable_method_wrappers)*
             #(#invokable_methods)*
             #(#normal_methods)*
 
             #handle_update_request
-            #handle_property_change
         }
     };
 
@@ -1174,14 +1116,10 @@ pub fn generate_qobject_rs(
     if obj.handle_updates_impl.is_some() {
         use_traits.push(quote! { use cxx_qt_lib::UpdateRequestHandler; });
     }
-    if obj.handle_property_change_impl.is_some() {
-        use_traits.push(quote! { use cxx_qt_lib::PropertyChangeHandler; });
-    }
     // TODO: only push if we have an opaque param or return type?
     use_traits.push(quote! { use cxx_qt_lib::ToUniquePtr; });
 
     let handle_updates_impl = &obj.handle_updates_impl;
-    let handle_property_change_impl = &obj.handle_property_change_impl;
 
     // Build our rewritten module that replaces the input from the macro
     let output = quote! {
@@ -1211,8 +1149,6 @@ pub fn generate_qobject_rs(
 
             #handle_updates_impl
 
-            #handle_property_change_impl
-
             fn create_rs() -> std::boxed::Box<RustObj> {
                 std::default::Default::default()
             }

diff --git a/cxx-qt-gen/test_inputs/handlers.rs b/cxx-qt-gen/test_inputs/handlers.rs
@@ -8,12 +8,6 @@ mod my_object {
     #[derive(Default)]
     struct RustObj;
 
-    impl PropertyChangeHandler<CppObj, Property> for RustObj {
-        fn handle_property_change(&mut self, _cpp: &mut CppObj, _property: Property) {
-            println!("change")
-        }
-    }
-
     impl UpdateRequestHandler<CppObj> for RustObj {
         fn handle_update_request(&mut self, _cpp: &mut CppObj) {
             println!("update")