-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
[proposal] add 'notify' to '#[var]' #946
base: master
Are you sure you want to change the base?
Changes from all commits
278044b
8f76506
39ab450
3e7f9fc
8e3fdec
c50fae4
55334b2
49119d1
9428e45
c658926
464da2d
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 |
---|---|---|
@@ -0,0 +1,14 @@ | ||
pub struct PropertyUpdate<'a, C, T> { | ||
pub new_value: T, | ||
pub field_name: &'a str, // might also be &'a StringName, depending on what's available | ||
pub get_field_mut: fn(&mut C) -> &mut T, | ||
} | ||
Comment on lines
+1
to
+5
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. Re public fields, I mentioned something here:
What's your take on this? |
||
|
||
impl<C, T> PropertyUpdate<'_, C, T> { | ||
pub fn set(self, obj: &mut C) { | ||
*(self.get_field_mut)(obj) = self.new_value; | ||
} | ||
pub fn set_custom(self, obj: &mut C, value: T) { | ||
*(self.get_field_mut)(obj) = value; | ||
} | ||
} | ||
Comment on lines
+7
to
+14
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. See this comment about naming. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,19 +13,68 @@ use crate::class::{ | |||||
FuncDefinition, | ||||||
}; | ||||||
use crate::util::make_funcs_collection_constant; | ||||||
use crate::util::KvParser; | ||||||
use crate::util::{bail, KvParser}; | ||||||
use crate::{util, ParseResult}; | ||||||
|
||||||
/// Store info from `#[var]` attribute. | ||||||
#[derive(Clone, Debug)] | ||||||
pub struct FieldVar { | ||||||
pub getter: GetterSetter, | ||||||
pub setter: GetterSetter, | ||||||
pub notify: Option<Ident>, | ||||||
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 still uses "notify" terminology, although the semantics have changed to "general_set". To be clear, I don't think we should provide both. Notify should be implementable via general-set, otherwise we're not designing the latter powerful enough. |
||||||
pub hint: FieldHint, | ||||||
pub usage_flags: UsageFlags, | ||||||
pub span: Span, | ||||||
} | ||||||
|
||||||
fn parse_notify(parser: &mut KvParser, key: &str) -> ParseResult<Option<Ident>> { | ||||||
match parser.handle_any(key) { | ||||||
// No `notify` argument | ||||||
None => Ok(None), | ||||||
Some(value) => match value { | ||||||
// `notify` without value is an error | ||||||
None => { | ||||||
bail!( | ||||||
parser.span(), | ||||||
"The correct syntax is 'notify = callback_fn'" | ||||||
) | ||||||
} | ||||||
// `notify = expr` | ||||||
Some(value) => match value.ident() { | ||||||
Ok(ident) => Ok(Some(ident)), | ||||||
Err(_) => bail!( | ||||||
parser.span(), | ||||||
"The correct syntax is 'notify = callback_fn'" | ||||||
), | ||||||
}, | ||||||
}, | ||||||
} | ||||||
} | ||||||
|
||||||
fn parse_setter_ex(parser: &mut KvParser, key: &str) -> ParseResult<Option<Ident>> { | ||||||
match parser.handle_any(key) { | ||||||
// No `notify` argument | ||||||
None => Ok(None), | ||||||
Some(value) => match value { | ||||||
// `notify` without value is an error | ||||||
None => { | ||||||
bail!( | ||||||
parser.span(), | ||||||
"The correct syntax is 'setter_ex = callback_fn'" | ||||||
) | ||||||
} | ||||||
// `notify = expr` | ||||||
Some(value) => match value.ident() { | ||||||
Ok(ident) => Ok(Some(ident)), | ||||||
Err(_) => bail!( | ||||||
parser.span(), | ||||||
"The correct syntax is 'setter_ex = callback_fn'" | ||||||
), | ||||||
}, | ||||||
}, | ||||||
} | ||||||
} | ||||||
|
||||||
impl FieldVar { | ||||||
/// Parse a `#[var]` attribute to a `FieldVar` struct. | ||||||
/// | ||||||
|
@@ -39,12 +88,32 @@ impl FieldVar { | |||||
let span = parser.span(); | ||||||
let mut getter = GetterSetter::parse(parser, "get")?; | ||||||
let mut setter = GetterSetter::parse(parser, "set")?; | ||||||
let notify = parse_notify(parser, "notify")?; | ||||||
let setter_ex = parse_setter_ex(parser, "set_ex")?; | ||||||
|
||||||
if getter.is_omitted() && setter.is_omitted() { | ||||||
if getter.is_omitted() && setter.is_omitted() && setter_ex.is_none() { | ||||||
getter = GetterSetter::Generated; | ||||||
setter = GetterSetter::Generated; | ||||||
} | ||||||
|
||||||
if notify.is_some() && !setter.is_generated() { | ||||||
return bail!( | ||||||
parser.span(), | ||||||
"When using 'notify', the property must also use an autogenerated 'set'" | ||||||
); | ||||||
} | ||||||
|
||||||
if setter_ex.is_some() && !setter.is_omitted() { | ||||||
return bail!( | ||||||
parser.span(), | ||||||
"You may not use 'set' and 'set_ex' at the same time, remove one" | ||||||
); | ||||||
} | ||||||
|
||||||
if let Some(ident) = setter_ex { | ||||||
setter = GetterSetter::Ex(ident); | ||||||
} | ||||||
|
||||||
let hint = parser.handle_ident("hint")?; | ||||||
|
||||||
let hint = if let Some(hint) = hint { | ||||||
|
@@ -72,6 +141,7 @@ impl FieldVar { | |||||
Ok(FieldVar { | ||||||
getter, | ||||||
setter, | ||||||
notify, | ||||||
hint, | ||||||
usage_flags, | ||||||
span, | ||||||
|
@@ -87,6 +157,7 @@ impl Default for FieldVar { | |||||
hint: Default::default(), | ||||||
usage_flags: Default::default(), | ||||||
span: Span::call_site(), | ||||||
notify: Default::default(), | ||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -102,6 +173,9 @@ pub enum GetterSetter { | |||||
|
||||||
/// Getter/setter is handwritten by the user, and here is its identifier. | ||||||
Custom(Ident), | ||||||
|
||||||
/// only applicable to setter. A generic setter that takes 'PropertyUpdate<C, T>' is handwritten by the user | ||||||
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. Proper sentence please 😉 |
||||||
Ex(Ident), | ||||||
} | ||||||
|
||||||
impl GetterSetter { | ||||||
|
@@ -128,36 +202,48 @@ impl GetterSetter { | |||||
&self, | ||||||
class_name: &Ident, | ||||||
kind: GetSet, | ||||||
notify: Option<Ident>, | ||||||
field: &Field, | ||||||
) -> Option<GetterSetterImpl> { | ||||||
match self { | ||||||
GetterSetter::Omitted => None, | ||||||
GetterSetter::Generated => Some(GetterSetterImpl::from_generated_impl( | ||||||
class_name, kind, field, | ||||||
class_name, kind, notify, field, | ||||||
)), | ||||||
GetterSetter::Custom(function_name) => { | ||||||
Some(GetterSetterImpl::from_custom_impl(function_name)) | ||||||
} | ||||||
GetterSetter::Ex(_function_name) => { | ||||||
assert!(matches!(kind, GetSet::SetEx(_))); | ||||||
Some(GetterSetterImpl::from_generated_impl( | ||||||
class_name, kind, notify, field, | ||||||
)) | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
pub fn is_omitted(&self) -> bool { | ||||||
matches!(self, GetterSetter::Omitted) | ||||||
} | ||||||
|
||||||
pub fn is_generated(&self) -> bool { | ||||||
matches!(self, GetterSetter::Generated) | ||||||
} | ||||||
} | ||||||
|
||||||
/// Used to determine whether a [`GetterSetter`] is supposed to be a getter or setter. | ||||||
#[derive(Copy, Clone, Eq, PartialEq, Debug)] | ||||||
#[derive(Clone, Eq, PartialEq, Debug)] | ||||||
pub enum GetSet { | ||||||
Get, | ||||||
Set, | ||||||
SetEx(Ident), | ||||||
} | ||||||
|
||||||
impl GetSet { | ||||||
pub fn prefix(&self) -> &'static str { | ||||||
match self { | ||||||
GetSet::Get => "get_", | ||||||
GetSet::Set => "set_", | ||||||
GetSet::Set | GetSet::SetEx(_) => "set_", | ||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -171,7 +257,12 @@ pub struct GetterSetterImpl { | |||||
} | ||||||
|
||||||
impl GetterSetterImpl { | ||||||
fn from_generated_impl(class_name: &Ident, kind: GetSet, field: &Field) -> Self { | ||||||
fn from_generated_impl( | ||||||
class_name: &Ident, | ||||||
kind: GetSet, | ||||||
notify: Option<Ident>, | ||||||
field: &Field, | ||||||
) -> Self { | ||||||
let Field { | ||||||
name: field_name, | ||||||
ty: field_type, | ||||||
|
@@ -196,9 +287,54 @@ impl GetterSetterImpl { | |||||
signature = quote! { | ||||||
fn #function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) | ||||||
}; | ||||||
function_body = quote! { | ||||||
|
||||||
let function_body_set = quote! { | ||||||
<#field_type as ::godot::register::property::Var>::set_property(&mut self.#field_name, #field_name); | ||||||
}; | ||||||
|
||||||
function_body = match notify { | ||||||
Some(ident) => { | ||||||
quote! { | ||||||
let prev_value = self.#field_name; | ||||||
#function_body_set | ||||||
if prev_value != self.#field_name { | ||||||
self.#ident(); | ||||||
} | ||||||
} | ||||||
} | ||||||
None => function_body_set, | ||||||
}; | ||||||
} | ||||||
GetSet::SetEx(callback_fn_ident) => { | ||||||
signature = quote! { | ||||||
fn #function_name(&mut self, #field_name: <#field_type as ::godot::meta::GodotConvert>::Via) | ||||||
}; | ||||||
|
||||||
let field_name_string_constant = field_name.to_string(); | ||||||
|
||||||
let function_body_set = quote! { | ||||||
|
||||||
let new_value = ::godot::meta::FromGodot::from_godot(#field_name); | ||||||
let property_update = ::godot::meta::PropertyUpdate { | ||||||
new_value: new_value, | ||||||
field_name: #field_name_string_constant, | ||||||
get_field_mut: |c: &mut #class_name|&mut c.#field_name | ||||||
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. Formatting:
Suggested change
|
||||||
}; | ||||||
self.#callback_fn_ident(property_update); | ||||||
}; | ||||||
|
||||||
function_body = match notify { | ||||||
Some(ident) => { | ||||||
quote! { | ||||||
let prev_value = self.#field_name; | ||||||
#function_body_set | ||||||
if prev_value != self.#field_name { | ||||||
self.#ident(); | ||||||
} | ||||||
} | ||||||
} | ||||||
None => function_body_set, | ||||||
}; | ||||||
} | ||||||
} | ||||||
|
||||||
|
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.
Should not be a
pub
module, just re-export it publicly.I think
register::property
would fit better as a module though, could you move these imports +property_update.rs
?