-
Notifications
You must be signed in to change notification settings - Fork 92
Allow immutable signals #1317
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
base: main
Are you sure you want to change the base?
Allow immutable signals #1317
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ use crate::{ | |
}; | ||
use core::ops::Deref; | ||
use std::ops::DerefMut; | ||
use syn::{spanned::Spanned, Attribute, Error, ForeignItemFn, Result, Visibility}; | ||
use syn::{Attribute, ForeignItemFn, Result, Visibility}; | ||
|
||
#[derive(Clone)] | ||
/// Describes an individual Signal | ||
|
@@ -42,12 +42,13 @@ impl ParsedSignal { | |
let fields = MethodFields::parse(method, auto_case)?; | ||
let attrs = require_attributes(&fields.method.attrs, &Self::ALLOWED_ATTRS)?; | ||
|
||
if !fields.mutable { | ||
return Err(Error::new( | ||
fields.method.span(), | ||
"signals must be mutable, use Pin<&mut T> instead of T for the self type", | ||
)); | ||
} | ||
// TODO: add proper checks | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, as @LeonMatthesKDAB suggested, we probably only want const signals support inside |
||
// if !fields.mutable { | ||
// return Err(Error::new( | ||
// fields.method.span(), | ||
// "signals must be mutable, use Pin<&mut T> instead of T for the self type", | ||
// )); | ||
// } | ||
|
||
let inherit = attrs.contains_key("inherit"); | ||
|
||
|
@@ -96,17 +97,13 @@ mod tests { | |
assert_parse_errors! { | ||
|input| ParsedSignal::parse(input, CaseConversion::none()) => | ||
|
||
// No immutable signals | ||
{ fn ready(self: &MyObject); } | ||
// No namespaces | ||
{ | ||
// No namespaces | ||
#[namespace = "disallowed_namespace"] | ||
fn ready(self: Pin<&mut MyObject>); | ||
} | ||
// Missing self | ||
{ fn ready(x: f64); } | ||
// Self needs to be receiver like self: &T instead of &self | ||
{ fn ready(&self); } | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,9 @@ mod ffi { | |
#[qsignal] | ||
fn ready(self: Pin<&mut Self>); | ||
|
||
#[qsignal] | ||
fn const_ready(&self); | ||
Comment on lines
+29
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would need to move up into the |
||
|
||
#[qsignal] | ||
fn data_changed( | ||
self: Pin<&mut Self>, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,11 @@ pub mod ffi { | |
#[qsignal] | ||
fn triggered(self: Pin<&mut Self>); | ||
|
||
/// const signal that is emitted when trigger is fired | ||
#[qsignal] | ||
#[rust_name = "triggered_const_signal"] | ||
fn triggeredConstSignal(&self); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should add a call to this here https://github.com/KDAB/cxx-qt/blob/main/examples/qml_features/cpp/external_qobject.cpp#L18 And copy the connection here https://github.com/KDAB/cxx-qt/blob/main/examples/qml_features/rust/src/externcxxqt.rs#L69 And then you can add a signal spy for it and check that they are triggered here so it's tested :-) https://github.com/KDAB/cxx-qt/blob/main/examples/qml_features/tests/tst_externcxxqt.qml#L41 |
||
|
||
/// Private signal that is emitted when trigger is fired | ||
#[qsignal] | ||
#[rust_name = "triggered_private_signal"] | ||
|
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.
can remove the commented code :-)
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.
guess we need to ensure there is a unit test that reaches this for coverage though