Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions zbus_xmlgen/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use snakecase::ascii::to_snakecase;
use std::{
collections::HashMap,
error::Error,
fmt::{Display, Formatter, Write},
process::{Command, Stdio},
Expand Down Expand Up @@ -185,6 +186,11 @@ impl GenTrait<'_> {

let mut signals = iface.signals().to_vec();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, can't we just switch to be a HashMap instead? Wouldn't it make the code even simpler? 🤔

signals.sort_by(|a, b| a.name().partial_cmp(&b.name()).unwrap());

// collect signal method names for collision detection with implicit property change
// receivers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing . again. Sorry! I just like consistency too much. Although in these files I've allowed changes from contributors that missed . but it's inconsistent with rest of the code and even code here, so let's not add to that. 🙏 I'll add . to existing comments/docs in these files myself later.

let mut changed_suffix_signals = HashMap::new();

for signal in &signals {
let args = parse_signal_args(signal.args());
let name = to_identifier(&to_snakecase(signal.name().as_str()));
Expand All @@ -196,20 +202,41 @@ impl GenTrait<'_> {
writeln!(w, " #[zbus(signal)]")?;
}
writeln!(w, " fn {name}({args}) -> zbus::Result<()>;",)?;

if name.ends_with("_changed") {
changed_suffix_signals.insert(name, signal.name());
}
}

let mut props = iface.properties().to_vec();
props.sort_by(|a, b| a.name().partial_cmp(&b.name()).unwrap());
for p in props {
let name = to_identifier(&to_snakecase(p.name().as_str()));
let fn_attribute = if pascal_case(&name) != p.name().as_str() {
format!(" #[zbus(property, name = \"{}\")]", p.name())

let prop_change_receiver_name = format!("{name}_changed");
let signal_collision = changed_suffix_signals.get(&prop_change_receiver_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the previous naming better. The variable isn't holding a collision but rather the Signal. The name makes it sound like it's a boolean so makes code slightly harder to follow/read.


let (property_args, comment) = if let Some(signal_collision) = signal_collision {
(
"(emits_changed_signal = \"false\")",
format!(
" // Note: change signal is shadowed by `{}` signal.\n",
signal_collision.as_str()
),
)
} else {
("", "".to_string())
};
let name_arg = if pascal_case(&name) != p.name().as_str() {
format!(", name = \"{}\"", p.name())
} else {
" #[zbus(property)]".to_string()
"".to_string()
};
let fn_attribute = format!(" #[zbus(property{property_args}{name_arg})]");

writeln!(w)?;
writeln!(w, " /// {} property", p.name())?;
write!(w, "{comment}")?;
if p.access().read() {
writeln!(w, "{fn_attribute}")?;
let output = to_rust_type(p.ty(), false, false);
Expand Down
9 changes: 9 additions & 0 deletions zbus_xmlgen/tests/data/sample_object0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ pub trait SampleInterface0 {
#[zbus(signal)]
fn signal_value(&self, value: zbus::zvariant::Value<'_>) -> zbus::Result<()>;

/// StateChanged signal
#[zbus(signal)]
fn state_changed(&self, state: u32, something_else: u32) -> zbus::Result<()>;

/// Bar property
#[zbus(property)]
fn bar(&self) -> zbus::Result<u8>;
Expand All @@ -94,4 +98,9 @@ pub trait SampleInterface0 {
std::collections::HashMap<String, zbus::zvariant::OwnedValue>,
)>,
>;

/// State property
// Note: change signal is shadowed by `StateChanged` signal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • change signal -> changed signal.
  • I just realized btw: wouldn't it be better if this was instead:
Suggested change
// Note: change signal is shadowed by `StateChanged` signal.
// Note: change signal is shadowed by `state_changed` signal method.

? That would also make the xmlgen part simpler and you can also go back to HashSet but this time w/o the caveat of reallocation. 😉

#[zbus(property(emits_changed_signal = "false"))]
fn state(&self) -> zbus::Result<u32>;
}
6 changes: 6 additions & 0 deletions zbus_xmlgen/tests/data/sample_object0.xml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@
<signal name="SignalDictStringToValue">
<arg type="a{sv}" name="dict"/>
</signal>
<!-- Signal `StateChanged` should override `State`'s implicit `receive_state_changed` method -->
<signal name="StateChanged">
<arg name="state" type="u"/>
<arg name="something_else" type="u"/>
</signal>
<property name="State" type="u" access="read"/>
<property name="Bar" type="y" access="readwrite"/>
<property name="Foo-Bar" type="y" access="readwrite"/>
<property name="Matryoshkas" type="a(oiasta{sv})" access="read"/>
Expand Down
Loading