fix: Allow generated methods to be reused by other properties #1241
Conversation
BenFordTytherington
force-pushed
the
fix-unknown-generated-signals
branch
from
75b1012 to
08b0ec4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1241 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 73 73
Lines 12612 12658 +46
=========================================
+ Hits 12612 12658 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenFordTytherington
force-pushed
the
fix-unknown-generated-signals
branch
from
08b0ec4 to
12627f2
Compare
- Add list of methods pending generation - Perform a lookup in the pending methods if they aren't found in the bridge - Use name of pending method if found
BenFordTytherington
force-pushed
the
fix-unknown-generated-signals
branch
from
12627f2 to
03e3d57
Compare
| let lookup = lookup_fn(structured_qobject, ident); | ||
| let name = if let Ok(name) = lookup { | ||
| name | ||
| } else if let Some(name) = structured_qobject.pending_lookup(ident) { | ||
| // If a bridge method isn't found, it may be pending generation | ||
| name | ||
| } else { | ||
| // return the not found error if the method wasn't pending either | ||
| lookup? | ||
| }; | ||
| Self::Custom(name) |
There was a problem hiding this comment.
Why is the lookup of auto-generated functions/signals split into the separate pending_lookup function?
These are normal functions, just auto-generated. So why treat them any different in lookup?
IMO method_lookup and signal_lookup should just return them.
There was a problem hiding this comment.
Because at this stage, the actual function signatures don't exist, the pending_lookup is simply searching through a set of names of the functions which are pending generation, whereas the signal and method lookups are finding actual methods which have been associated during structuring, so they have a declaration, and all the associated data already, whereas the pending ones just have a name, and will be generated later.
There was a problem hiding this comment.
I should note as well, those lookups are reading from vectors of ParsedSignal or ParsedMethod in StructuredQObject, so putting those pending methods in there would require building those structs just from the name.
| if let Some(FlagState::Auto) = &self.flags.notify { | ||
| pending.push(notify_name_from_property(&self.name)); | ||
| } |
There was a problem hiding this comment.
This is a signal, not a method, so needs to be treated differently during lookup.
We wouldn't want someone to e.g. use:
#[qproperty(i32, number, READ, NOTIFY=number)]
Which cxx-qt-gen would now accept.
| /// Searches for a name in the list of functions pending generation | ||
| pub fn pending_lookup(&self, id: &Ident) -> Option<Name> { | ||
| lookup(&self.pending_generation, id, |pending| pending) | ||
| } |
There was a problem hiding this comment.
I don't think this should be a separate function, the lookup should just work via method_lookup and signal_lookup respectively.
If you want to look up whether a certain function can be named, then I don't think you need to care whether it's a generated function or a manually written one.
Should fix #1232