Skip to content
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.

Sign up for GitHub

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

refactor!: Rename name to label and use value for label content #475

Merged
merged 3 commits into from
Oct 29, 2024
+102 −102
Merged

refactor!: Rename name to label and use value for label content #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4cd597
refactor!: Rename `name` to `label` and use `value` for label content
mwcampbell Oct 29, 2024
f6dc393
Add some documentation on the label property.
mwcampbell Oct 29, 2024
c73b612
Address review feedback
mwcampbell Oct 29, 2024
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
12 changes: 8 additions & 4 deletions common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ enum PropertyId {
PopupFor,

// String
Name,
Label,
Description,
Value,
AccessKey,
Expand Down Expand Up @@ -1537,7 +1537,11 @@ node_id_property_methods! {
}

string_property_methods! {
(Name, name, set_name, clear_name),
/// The label of a control that can have a label. If the label is specified
/// via the [`Node::labelled_by`] relation, this doesn't need to be set.
/// Note that the text content of a node with the [`Role::Label`] role
/// should be provided via [`Node::value`], not this property.
(Label, label, set_label, clear_label),
(Description, description, set_description, clear_description),
(Value, value, set_value, clear_value),
/// A single character, usually part of this node's name, that can be pressed,
Expand Down Expand Up @@ -1982,7 +1986,7 @@ impl<'de> Visitor<'de> for PropertiesVisitor {
PopupFor
},
String {
Name,
Label,
Description,
Value,
AccessKey,
Expand Down Expand Up @@ -2129,7 +2133,7 @@ impl JsonSchema for Properties {
PopupFor
},
Box<str> {
Name,
Label,
Description,
Value,
AccessKey,
Expand Down
14 changes: 7 additions & 7 deletions consumer/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ mod tests {
};
let label_0_0_ignored = {
let mut node = Node::new(Role::Label);
node.set_name("label_0_0_ignored");
node.set_value("label_0_0_ignored");
node
};
let paragraph_1_ignored = {
Expand All @@ -83,7 +83,7 @@ mod tests {
};
let button_1_0_hidden = {
let mut node = Node::new(Role::Button);
node.set_name("button_1_0_hidden");
node.set_label("button_1_0_hidden");
node.set_hidden();
node.set_children(vec![CONTAINER_1_0_0_HIDDEN_ID]);
node
Expand All @@ -101,12 +101,12 @@ mod tests {
x1: 90.0,
y1: 30.0,
});
node.set_name("label_1_1");
node.set_value("label_1_1");
node
};
let button_1_2_hidden = {
let mut node = Node::new(Role::Button);
node.set_name("button_1_2_hidden");
node.set_label("button_1_2_hidden");
node.set_hidden();
node.set_children(vec![CONTAINER_1_2_0_HIDDEN_ID]);
node
Expand All @@ -123,7 +123,7 @@ mod tests {
};
let label_2_0 = {
let mut node = Node::new(Role::Label);
node.set_name("label_2_0");
node.set_label("label_2_0");
node
};
let paragraph_3_ignored = {
Expand All @@ -145,12 +145,12 @@ mod tests {
};
let label_3_1_0 = {
let mut node = Node::new(Role::Label);
node.set_name("label_3_1_0");
node.set_value("label_3_1_0");
node
};
let button_3_2 = {
let mut node = Node::new(Role::Button);
node.set_name("button_3_2");
node.set_label("button_3_2");
node
};
let empty_container_3_3_ignored = Node::new(Role::GenericContainer);
Expand Down
70 changes: 40 additions & 30 deletions consumer/src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,15 +492,25 @@ impl<'a> Node<'a> {
}
}

pub fn name(&self) -> Option<String> {
if let Some(name) = &self.data().name() {
Some(name.to_string())
pub fn label_comes_from_value(&self) -> bool {
self.role() == Role::Label
}

pub fn label(&self) -> Option<String> {
if let Some(label) = &self.data().label() {
Some(label.to_string())
} else {
let names = self
let labels = self
.labelled_by()
.filter_map(|node| node.name())
.filter_map(|node| {
if node.label_comes_from_value() {
node.value()
} else {
node.label()
}
})
.collect::<Vec<String>>();
(!names.is_empty()).then(move || names.join(" "))
(!labels.is_empty()).then(move || labels.join(" "))
}
}

Expand Down Expand Up @@ -916,7 +926,7 @@ mod tests {
}

#[test]
fn no_name_or_labelled_by() {
fn no_label_or_labelled_by() {
let update = TreeUpdate {
nodes: vec![
(NodeId(0), {
Expand All @@ -930,11 +940,11 @@ mod tests {
focus: NodeId(0),
};
let tree = crate::Tree::new(update, false);
assert_eq!(None, tree.state().node_by_id(NodeId(1)).unwrap().name());
assert_eq!(None, tree.state().node_by_id(NodeId(1)).unwrap().label());
}

#[test]
fn name_from_labelled_by() {
fn label_from_labelled_by() {
// The following mock UI probably isn't very localization-friendly,
// but it's good for this test.
const LABEL_1: &str = "Check email every";
Expand All @@ -954,7 +964,7 @@ mod tests {
}),
(NodeId(2), {
let mut node = Node::new(Role::Label);
node.set_name(LABEL_1);
node.set_value(LABEL_1);
node
}),
(NodeId(3), {
Expand All @@ -964,7 +974,7 @@ mod tests {
}),
(NodeId(4), {
let mut node = Node::new(Role::Label);
node.set_name(LABEL_2);
node.set_value(LABEL_2);
node
}),
],
Expand All @@ -974,16 +984,16 @@ mod tests {
let tree = crate::Tree::new(update, false);
assert_eq!(
Some([LABEL_1, LABEL_2].join(" ")),
tree.state().node_by_id(NodeId(1)).unwrap().name()
tree.state().node_by_id(NodeId(1)).unwrap().label()
);
assert_eq!(
Some(LABEL_2.into()),
tree.state().node_by_id(NodeId(3)).unwrap().name()
tree.state().node_by_id(NodeId(3)).unwrap().label()
);
}

#[test]
fn name_from_descendant_label() {
fn label_from_descendant_label() {
const ROOT_ID: NodeId = NodeId(0);
const DEFAULT_BUTTON_ID: NodeId = NodeId(1);
const DEFAULT_BUTTON_LABEL_ID: NodeId = NodeId(2);
Expand Down Expand Up @@ -1034,7 +1044,7 @@ mod tests {
}),
(DEFAULT_BUTTON_LABEL_ID, {
let mut node = Node::new(Role::Image);
node.set_name(DEFAULT_BUTTON_LABEL);
node.set_label(DEFAULT_BUTTON_LABEL);
node
}),
(LINK_ID, {
Expand All @@ -1049,7 +1059,7 @@ mod tests {
}),
(LINK_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(LINK_LABEL);
node.set_value(LINK_LABEL);
node
}),
(CHECKBOX_ID, {
Expand All @@ -1059,7 +1069,7 @@ mod tests {
}),
(CHECKBOX_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(CHECKBOX_LABEL);
node.set_value(CHECKBOX_LABEL);
node
}),
(RADIO_BUTTON_ID, {
Expand All @@ -1069,7 +1079,7 @@ mod tests {
}),
(RADIO_BUTTON_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(RADIO_BUTTON_LABEL);
node.set_value(RADIO_BUTTON_LABEL);
node
}),
(MENU_BUTTON_ID, {
Expand All @@ -1079,7 +1089,7 @@ mod tests {
}),
(MENU_BUTTON_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(MENU_BUTTON_LABEL);
node.set_value(MENU_BUTTON_LABEL);
node
}),
(MENU_ID, {
Expand All @@ -1094,7 +1104,7 @@ mod tests {
}),
(MENU_ITEM_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(MENU_ITEM_LABEL);
node.set_value(MENU_ITEM_LABEL);
node
}),
(MENU_ITEM_CHECKBOX_ID, {
Expand All @@ -1104,7 +1114,7 @@ mod tests {
}),
(MENU_ITEM_CHECKBOX_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(MENU_ITEM_CHECKBOX_LABEL);
node.set_value(MENU_ITEM_CHECKBOX_LABEL);
node
}),
(MENU_ITEM_RADIO_ID, {
Expand All @@ -1114,7 +1124,7 @@ mod tests {
}),
(MENU_ITEM_RADIO_LABEL_ID, {
let mut node = Node::new(Role::Label);
node.set_name(MENU_ITEM_RADIO_LABEL);
node.set_value(MENU_ITEM_RADIO_LABEL);
node
}),
],
Expand All @@ -1124,38 +1134,38 @@ mod tests {
let tree = crate::Tree::new(update, false);
assert_eq!(
Some(DEFAULT_BUTTON_LABEL.into()),
tree.state().node_by_id(DEFAULT_BUTTON_ID).unwrap().name()
tree.state().node_by_id(DEFAULT_BUTTON_ID).unwrap().label()
);
assert_eq!(
Some(LINK_LABEL.into()),
tree.state().node_by_id(LINK_ID).unwrap().name()
tree.state().node_by_id(LINK_ID).unwrap().label()
);
assert_eq!(
Some(CHECKBOX_LABEL.into()),
tree.state().node_by_id(CHECKBOX_ID).unwrap().name()
tree.state().node_by_id(CHECKBOX_ID).unwrap().label()
);
assert_eq!(
Some(RADIO_BUTTON_LABEL.into()),
tree.state().node_by_id(RADIO_BUTTON_ID).unwrap().name()
tree.state().node_by_id(RADIO_BUTTON_ID).unwrap().label()
);
assert_eq!(
Some(MENU_BUTTON_LABEL.into()),
tree.state().node_by_id(MENU_BUTTON_ID).unwrap().name()
tree.state().node_by_id(MENU_BUTTON_ID).unwrap().label()
);
assert_eq!(
Some(MENU_ITEM_LABEL.into()),
tree.state().node_by_id(MENU_ITEM_ID).unwrap().name()
tree.state().node_by_id(MENU_ITEM_ID).unwrap().label()
);
assert_eq!(
Some(MENU_ITEM_CHECKBOX_LABEL.into()),
tree.state()
.node_by_id(MENU_ITEM_CHECKBOX_ID)
.unwrap()
.name()
.label()
);
assert_eq!(
Some(MENU_ITEM_RADIO_LABEL.into()),
tree.state().node_by_id(MENU_ITEM_RADIO_ID).unwrap().name()
tree.state().node_by_id(MENU_ITEM_RADIO_ID).unwrap().label()
);
}
}
14 changes: 7 additions & 7 deletions consumer/src/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ mod tests {
}),
(NodeId(1), {
let mut node = child_node.clone();
node.set_name("foo");
node.set_label("foo");
node
}),
],
Expand All @@ -657,12 +657,12 @@ mod tests {
let mut tree = super::Tree::new(first_update, false);
assert_eq!(
Some("foo".into()),
tree.state().node_by_id(NodeId(1)).unwrap().name()
tree.state().node_by_id(NodeId(1)).unwrap().label()
);
let second_update = TreeUpdate {
nodes: vec![(NodeId(1), {
let mut node = child_node;
node.set_name("bar");
node.set_label("bar");
node
})],
tree: None,
Expand All @@ -680,8 +680,8 @@ mod tests {
}
fn node_updated(&mut self, old_node: &crate::Node, new_node: &crate::Node) {
if new_node.id() == NodeId(1)
&& old_node.name() == Some("foo".into())
&& new_node.name() == Some("bar".into())
&& old_node.label() == Some("foo".into())
&& new_node.label() == Some("bar".into())
{
self.got_updated_child_node = true;
return;
Expand All @@ -706,7 +706,7 @@ mod tests {
assert!(handler.got_updated_child_node);
assert_eq!(
Some("bar".into()),
tree.state().node_by_id(NodeId(1)).unwrap().name()
tree.state().node_by_id(NodeId(1)).unwrap().label()
);
}

Expand All @@ -725,7 +725,7 @@ mod tests {
}),
(NodeId(1), {
let mut node = Node::new(Role::Button);
node.set_name("foo");
node.set_label("foo");
node
}),
],
Expand Down
6 changes: 5 additions & 1 deletion platforms/atspi-common/src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ pub(crate) struct NodeWrapper<'a>(pub(crate) &'a Node<'a>);

impl<'a> NodeWrapper<'a> {
pub(crate) fn name(&self) -> Option<String> {
self.0.name()
if self.0.label_comes_from_value() {
self.0.value()
} else {
self.0.label()
}
}

pub(crate) fn description(&self) -> Option<String> {
Expand Down
Loading