From e9ac35154350169cb81b25d828b298062bd4e008 Mon Sep 17 00:00:00 2001 From: Matt Campbell Date: Sun, 8 Dec 2024 09:54:34 -0600 Subject: [PATCH 1/2] fix: Optimize dynamic string building --- consumer/src/node.rs | 83 ++++++++++++++++++++++++++------ consumer/src/text.rs | 19 +++++--- platforms/windows/src/adapter.rs | 9 ++-- platforms/windows/src/node.rs | 15 ++++-- platforms/windows/src/text.rs | 6 ++- platforms/windows/src/util.rs | 51 ++++++++++++++++---- 6 files changed, 141 insertions(+), 42 deletions(-) diff --git a/consumer/src/node.rs b/consumer/src/node.rs index 29d1cffd..13e86ac9 100644 --- a/consumer/src/node.rs +++ b/consumer/src/node.rs @@ -17,7 +17,7 @@ use alloc::{ sync::Arc, vec::Vec, }; -use core::iter::FusedIterator; +use core::{fmt, iter::FusedIterator}; use crate::filters::FilterResult; use crate::iterators::{ @@ -501,20 +501,37 @@ impl<'a> Node<'a> { } pub fn label(&self) -> Option { + let mut result = String::new(); + self.write_label(&mut result).unwrap().then_some(result) + } + + fn write_label_direct(&self, mut writer: W) -> Result { if let Some(label) = &self.data().label() { - Some(label.to_string()) + writer.write_str(label)?; + Ok(true) + } else { + Ok(false) + } + } + + pub fn write_label(&self, mut writer: W) -> Result { + if self.write_label_direct(&mut writer)? { + Ok(true) } else { - let labels = self - .labelled_by() - .filter_map(|node| { - if node.label_comes_from_value() { - node.value() - } else { - node.label() - } - }) - .collect::>(); - (!labels.is_empty()).then(move || labels.join(" ")) + let mut wrote_one = false; + for node in self.labelled_by() { + let writer = SpacePrefixingWriter { + inner: &mut writer, + need_prefix: wrote_one, + }; + let wrote_this_time = if node.label_comes_from_value() { + node.write_value(writer) + } else { + node.write_label_direct(writer) + }?; + wrote_one = wrote_one || wrote_this_time; + } + Ok(wrote_one) } } @@ -529,12 +546,19 @@ impl<'a> Node<'a> { } pub fn value(&self) -> Option { + let mut result = String::new(); + self.write_value(&mut result).unwrap().then_some(result) + } + + pub fn write_value(&self, mut writer: W) -> Result { if let Some(value) = &self.data().value() { - Some(value.to_string()) + writer.write_str(value)?; + Ok(true) } else if self.supports_text_ranges() && !self.is_multiline() { - Some(self.document_range().text()) + self.document_range().write_text(writer)?; + Ok(true) } else { - None + Ok(false) } } @@ -664,6 +688,33 @@ impl<'a> Node<'a> { } } +struct SpacePrefixingWriter { + inner: W, + need_prefix: bool, +} + +impl SpacePrefixingWriter { + fn write_prefix_if_needed(&mut self) -> fmt::Result { + if self.need_prefix { + self.inner.write_char(' ')?; + self.need_prefix = false; + } + Ok(()) + } +} + +impl fmt::Write for SpacePrefixingWriter { + fn write_str(&mut self, s: &str) -> fmt::Result { + self.write_prefix_if_needed()?; + self.inner.write_str(s) + } + + fn write_char(&mut self, c: char) -> fmt::Result { + self.write_prefix_if_needed()?; + self.inner.write_char(c) + } +} + #[cfg(test)] mod tests { use accesskit::{Node, NodeId, Point, Rect, Role, Tree, TreeUpdate}; diff --git a/consumer/src/text.rs b/consumer/src/text.rs index b8ba6a03..06863f99 100644 --- a/consumer/src/text.rs +++ b/consumer/src/text.rs @@ -7,7 +7,7 @@ use accesskit::{ NodeId, Point, Rect, Role, TextDirection, TextPosition as WeakPosition, TextSelection, }; use alloc::{string::String, vec::Vec}; -use core::{cmp::Ordering, iter::FusedIterator}; +use core::{cmp::Ordering, fmt, iter::FusedIterator}; use crate::{FilterResult, Node, TreeState}; @@ -549,7 +549,12 @@ impl<'a> Range<'a> { pub fn text(&self) -> String { let mut result = String::new(); - self.walk::<_, ()>(|node| { + self.write_text(&mut result).unwrap(); + result + } + + pub fn write_text(&self, mut writer: W) -> fmt::Result { + if let Some(err) = self.walk(|node| { let character_lengths = node.data().character_lengths(); let start_index = if node.id() == self.start.node.id() { self.start.character_index @@ -580,10 +585,12 @@ impl<'a> Range<'a> { .sum::(); &value[slice_start..slice_end] }; - result.push_str(s); - None - }); - result + writer.write_str(s).err() + }) { + Err(err) + } else { + Ok(()) + } } /// Returns the range's transformed bounding boxes relative to the tree's diff --git a/platforms/windows/src/adapter.rs b/platforms/windows/src/adapter.rs index ff00ec22..8ce5a2b7 100644 --- a/platforms/windows/src/adapter.rs +++ b/platforms/windows/src/adapter.rs @@ -111,11 +111,12 @@ impl TreeChangeHandler for AdapterChangeHandler<'_> { let old_wrapper = NodeWrapper(old_node); let new_wrapper = NodeWrapper(new_node); new_wrapper.enqueue_property_changes(&mut self.queue, &element, &old_wrapper); - if new_wrapper.name().is_some() + let new_name = new_wrapper.name(); + if new_name.is_some() && new_node.live() != Live::Off - && (new_wrapper.name() != old_wrapper.name() - || new_node.live() != old_node.live() - || filter(old_node) != FilterResult::Include) + && (new_node.live() != old_node.live() + || filter(old_node) != FilterResult::Include + || new_name != old_wrapper.name()) { self.queue.push(QueuedEvent::Simple { element, diff --git a/platforms/windows/src/node.rs b/platforms/windows/src/node.rs index 8a51196d..cae2b1a4 100644 --- a/platforms/windows/src/node.rs +++ b/platforms/windows/src/node.rs @@ -258,12 +258,15 @@ impl NodeWrapper<'_> { self.0.role_description() } - pub(crate) fn name(&self) -> Option { + pub(crate) fn name(&self) -> Option { + let mut result = WideString::default(); if self.0.label_comes_from_value() { - self.0.value() + self.0.write_value(&mut result) } else { - self.0.label() + self.0.write_label(&mut result) } + .unwrap() + .then_some(result) } fn description(&self) -> Option { @@ -339,8 +342,10 @@ impl NodeWrapper<'_> { self.0.numeric_value().is_some() } - fn value(&self) -> String { - self.0.value().unwrap() + fn value(&self) -> WideString { + let mut result = WideString::default(); + self.0.write_value(&mut result).unwrap(); + result } fn is_read_only(&self) -> bool { diff --git a/platforms/windows/src/text.rs b/platforms/windows/src/text.rs index 6f204a9d..45d149ff 100644 --- a/platforms/windows/src/text.rs +++ b/platforms/windows/src/text.rs @@ -483,7 +483,11 @@ impl ITextRangeProvider_Impl for PlatformRange_Impl { fn GetText(&self, _max_length: i32) -> Result { // The Microsoft docs imply that the provider isn't _required_ // to truncate text at the max length, so we just ignore it. - self.read(|range| Ok(range.text().into())) + self.read(|range| { + let mut result = WideString::default(); + range.write_text(&mut result).unwrap(); + Ok(result.into()) + }) } fn Move(&self, unit: TextUnit, count: i32) -> Result { diff --git a/platforms/windows/src/util.rs b/platforms/windows/src/util.rs index 0bf09601..69ecea4d 100644 --- a/platforms/windows/src/util.rs +++ b/platforms/windows/src/util.rs @@ -5,7 +5,10 @@ use accesskit::Point; use accesskit_consumer::TreeState; -use std::sync::{Arc, Weak}; +use std::{ + fmt, + sync::{Arc, Weak}, +}; use windows::{ core::*, Win32::{ @@ -18,6 +21,27 @@ use windows::{ use crate::window_handle::WindowHandle; +#[derive(Clone, Default, PartialEq, Eq)] +pub(crate) struct WideString(Vec); + +impl fmt::Write for WideString { + fn write_str(&mut self, s: &str) -> fmt::Result { + self.0.extend(s.encode_utf16()); + Ok(()) + } + + fn write_char(&mut self, c: char) -> fmt::Result { + self.0.extend_from_slice(c.encode_utf16(&mut [0; 2])); + Ok(()) + } +} + +impl From for BSTR { + fn from(value: WideString) -> Self { + Self::from_wide(&value.0).unwrap() + } +} + pub(crate) struct Variant(VARIANT); impl From for VARIANT { @@ -36,22 +60,29 @@ impl Variant { } } -impl From<&str> for Variant { - fn from(value: &str) -> Self { +impl From for Variant { + fn from(value: BSTR) -> Self { Self(value.into()) } } -impl From for Variant { - fn from(value: String) -> Self { - let value: BSTR = value.into(); - Self(value.into()) +impl From for Variant { + fn from(value: WideString) -> Self { + BSTR::from(value).into() } } -impl From for Variant { - fn from(value: BSTR) -> Self { - Self(value.into()) +impl From<&str> for Variant { + fn from(value: &str) -> Self { + let mut result = WideString::default(); + fmt::Write::write_str(&mut result, value).unwrap(); + result.into() + } +} + +impl From for Variant { + fn from(value: String) -> Self { + value.as_str().into() } } From a11788a43ca44f963fc058576259f607f45a07cc Mon Sep 17 00:00:00 2001 From: Matt Campbell Date: Sun, 8 Dec 2024 16:08:13 -0600 Subject: [PATCH 2/2] Optimize `toolkit_description` --- platforms/windows/src/util.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/platforms/windows/src/util.rs b/platforms/windows/src/util.rs index 69ecea4d..e674ba3b 100644 --- a/platforms/windows/src/util.rs +++ b/platforms/windows/src/util.rs @@ -6,7 +6,7 @@ use accesskit::Point; use accesskit_consumer::TreeState; use std::{ - fmt, + fmt::{self, Write}, sync::{Arc, Weak}, }; use windows::{ @@ -24,7 +24,7 @@ use crate::window_handle::WindowHandle; #[derive(Clone, Default, PartialEq, Eq)] pub(crate) struct WideString(Vec); -impl fmt::Write for WideString { +impl Write for WideString { fn write_str(&mut self, s: &str) -> fmt::Result { self.0.extend(s.encode_utf16()); Ok(()) @@ -75,7 +75,7 @@ impl From for Variant { impl From<&str> for Variant { fn from(value: &str) -> Self { let mut result = WideString::default(); - fmt::Write::write_str(&mut result, value).unwrap(); + result.write_str(value).unwrap(); result.into() } } @@ -247,13 +247,15 @@ pub(crate) fn window_title(hwnd: WindowHandle) -> Option { Some(BSTR::from_wide(&buffer).unwrap()) } -pub(crate) fn toolkit_description(state: &TreeState) -> Option { +pub(crate) fn toolkit_description(state: &TreeState) -> Option { state.toolkit_name().map(|name| { + let mut result = WideString::default(); + result.write_str(name).unwrap(); if let Some(version) = state.toolkit_version() { - format!("{} {}", name, version) - } else { - name.to_string() + result.write_char(' ').unwrap(); + result.write_str(version).unwrap(); } + result }) }