Skip to content

Commit a86901d

Browse files
mwcampbellmwcampbell
authored
fix: Optimize dynamic string building (#491)
1 parent 484fd7c commit a86901d

File tree

6 files changed

+147
-46
lines changed

6 files changed

+147
-46
lines changed

‎consumer/src/node.rs‎

Lines changed: 67 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use alloc::{
1717
sync::Arc,
1818
vec::Vec,
1919
};
20-
use core::iter::FusedIterator;
20+
use core::{fmt, iter::FusedIterator};
2121

2222
use crate::filters::FilterResult;
2323
use crate::iterators::{
@@ -501,20 +501,37 @@ impl<'a> Node<'a> {
501501
}
502502

503503
pub fn label(&self) -> Option<String> {
504+
let mut result = String::new();
505+
self.write_label(&mut result).unwrap().then_some(result)
506+
}
507+
508+
fn write_label_direct<W: fmt::Write>(&self, mut writer: W) -> Result<bool, fmt::Error> {
504509
if let Some(label) = &self.data().label() {
505-
Some(label.to_string())
510+
writer.write_str(label)?;
511+
Ok(true)
512+
} else {
513+
Ok(false)
514+
}
515+
}
516+
517+
pub fn write_label<W: fmt::Write>(&self, mut writer: W) -> Result<bool, fmt::Error> {
518+
if self.write_label_direct(&mut writer)? {
519+
Ok(true)
506520
} else {
507-
let labels = self
508-
.labelled_by()
509-
.filter_map(|node| {
510-
if node.label_comes_from_value() {
511-
node.value()
512-
} else {
513-
node.label()
514-
}
515-
})
516-
.collect::<Vec<String>>();
517-
(!labels.is_empty()).then(move || labels.join(" "))
521+
let mut wrote_one = false;
522+
for node in self.labelled_by() {
523+
let writer = SpacePrefixingWriter {
524+
inner: &mut writer,
525+
need_prefix: wrote_one,
526+
};
527+
let wrote_this_time = if node.label_comes_from_value() {
528+
node.write_value(writer)
529+
} else {
530+
node.write_label_direct(writer)
531+
}?;
532+
wrote_one = wrote_one || wrote_this_time;
533+
}
534+
Ok(wrote_one)
518535
}
519536
}
520537

@@ -529,12 +546,19 @@ impl<'a> Node<'a> {
529546
}
530547

531548
pub fn value(&self) -> Option<String> {
549+
let mut result = String::new();
550+
self.write_value(&mut result).unwrap().then_some(result)
551+
}
552+
553+
pub fn write_value<W: fmt::Write>(&self, mut writer: W) -> Result<bool, fmt::Error> {
532554
if let Some(value) = &self.data().value() {
533-
Some(value.to_string())
555+
writer.write_str(value)?;
556+
Ok(true)
534557
} else if self.supports_text_ranges() && !self.is_multiline() {
535-
Some(self.document_range().text())
558+
self.document_range().write_text(writer)?;
559+
Ok(true)
536560
} else {
537-
None
561+
Ok(false)
538562
}
539563
}
540564

@@ -664,6 +688,33 @@ impl<'a> Node<'a> {
664688
}
665689
}
666690

691+
struct SpacePrefixingWriter<W: fmt::Write> {
692+
inner: W,
693+
need_prefix: bool,
694+
}
695+
696+
impl<W: fmt::Write> SpacePrefixingWriter<W> {
697+
fn write_prefix_if_needed(&mut self) -> fmt::Result {
698+
if self.need_prefix {
699+
self.inner.write_char(' ')?;
700+
self.need_prefix = false;
701+
}
702+
Ok(())
703+
}
704+
}
705+
706+
impl<W: fmt::Write> fmt::Write for SpacePrefixingWriter<W> {
707+
fn write_str(&mut self, s: &str) -> fmt::Result {
708+
self.write_prefix_if_needed()?;
709+
self.inner.write_str(s)
710+
}
711+
712+
fn write_char(&mut self, c: char) -> fmt::Result {
713+
self.write_prefix_if_needed()?;
714+
self.inner.write_char(c)
715+
}
716+
}
717+
667718
#[cfg(test)]
668719
mod tests {
669720
use accesskit::{Node, NodeId, Point, Rect, Role, Tree, TreeUpdate};

‎consumer/src/text.rs‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use accesskit::{
77
NodeId, Point, Rect, Role, TextDirection, TextPosition as WeakPosition, TextSelection,
88
};
99
use alloc::{string::String, vec::Vec};
10-
use core::{cmp::Ordering, iter::FusedIterator};
10+
use core::{cmp::Ordering, fmt, iter::FusedIterator};
1111

1212
use crate::{FilterResult, Node, TreeState};
1313

@@ -549,7 +549,12 @@ impl<'a> Range<'a> {
549549

550550
pub fn text(&self) -> String {
551551
let mut result = String::new();
552-
self.walk::<_, ()>(|node| {
552+
self.write_text(&mut result).unwrap();
553+
result
554+
}
555+
556+
pub fn write_text<W: fmt::Write>(&self, mut writer: W) -> fmt::Result {
557+
if let Some(err) = self.walk(|node| {
553558
let character_lengths = node.data().character_lengths();
554559
let start_index = if node.id() == self.start.node.id() {
555560
self.start.character_index
@@ -580,10 +585,12 @@ impl<'a> Range<'a> {
580585
.sum::<usize>();
581586
&value[slice_start..slice_end]
582587
};
583-
result.push_str(s);
584-
None
585-
});
586-
result
588+
writer.write_str(s).err()
589+
}) {
590+
Err(err)
591+
} else {
592+
Ok(())
593+
}
587594
}
588595

589596
/// Returns the range's transformed bounding boxes relative to the tree's

‎platforms/windows/src/adapter.rs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,12 @@ impl TreeChangeHandler for AdapterChangeHandler<'_> {
111111
let old_wrapper = NodeWrapper(old_node);
112112
let new_wrapper = NodeWrapper(new_node);
113113
new_wrapper.enqueue_property_changes(&mut self.queue, &element, &old_wrapper);
114-
if new_wrapper.name().is_some()
114+
let new_name = new_wrapper.name();
115+
if new_name.is_some()
115116
&& new_node.live() != Live::Off
116-
&& (new_wrapper.name() != old_wrapper.name()
117-
|| new_node.live() != old_node.live()
118-
|| filter(old_node) != FilterResult::Include)
117+
&& (new_node.live() != old_node.live()
118+
|| filter(old_node) != FilterResult::Include
119+
|| new_name != old_wrapper.name())
119120
{
120121
self.queue.push(QueuedEvent::Simple {
121122
element,

‎platforms/windows/src/node.rs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -258,12 +258,15 @@ impl NodeWrapper<'_> {
258258
self.0.role_description()
259259
}
260260

261-
pub(crate) fn name(&self) -> Option<String> {
261+
pub(crate) fn name(&self) -> Option<WideString> {
262+
let mut result = WideString::default();
262263
if self.0.label_comes_from_value() {
263-
self.0.value()
264+
self.0.write_value(&mut result)
264265
} else {
265-
self.0.label()
266+
self.0.write_label(&mut result)
266267
}
268+
.unwrap()
269+
.then_some(result)
267270
}
268271

269272
fn description(&self) -> Option<String> {
@@ -339,8 +342,10 @@ impl NodeWrapper<'_> {
339342
self.0.numeric_value().is_some()
340343
}
341344

342-
fn value(&self) -> String {
343-
self.0.value().unwrap()
345+
fn value(&self) -> WideString {
346+
let mut result = WideString::default();
347+
self.0.write_value(&mut result).unwrap();
348+
result
344349
}
345350

346351
fn is_read_only(&self) -> bool {

‎platforms/windows/src/text.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,11 @@ impl ITextRangeProvider_Impl for PlatformRange_Impl {
483483
fn GetText(&self, _max_length: i32) -> Result<BSTR> {
484484
// The Microsoft docs imply that the provider isn't _required_
485485
// to truncate text at the max length, so we just ignore it.
486-
self.read(|range| Ok(range.text().into()))
486+
self.read(|range| {
487+
let mut result = WideString::default();
488+
range.write_text(&mut result).unwrap();
489+
Ok(result.into())
490+
})
487491
}
488492

489493
fn Move(&self, unit: TextUnit, count: i32) -> Result<i32> {

‎platforms/windows/src/util.rs‎

Lines changed: 47 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@
55

66
use accesskit::Point;
77
use accesskit_consumer::TreeState;
8-
use std::sync::{Arc, Weak};
8+
use std::{
9+
fmt::{self, Write},
10+
sync::{Arc, Weak},
11+
};
912
use windows::{
1013
core::*,
1114
Win32::{
@@ -18,6 +21,27 @@ use windows::{
1821

1922
use crate::window_handle::WindowHandle;
2023

24+
#[derive(Clone, Default, PartialEq, Eq)]
25+
pub(crate) struct WideString(Vec<u16>);
26+
27+
impl Write for WideString {
28+
fn write_str(&mut self, s: &str) -> fmt::Result {
29+
self.0.extend(s.encode_utf16());
30+
Ok(())
31+
}
32+
33+
fn write_char(&mut self, c: char) -> fmt::Result {
34+
self.0.extend_from_slice(c.encode_utf16(&mut [0; 2]));
35+
Ok(())
36+
}
37+
}
38+
39+
impl From<WideString> for BSTR {
40+
fn from(value: WideString) -> Self {
41+
Self::from_wide(&value.0).unwrap()
42+
}
43+
}
44+
2145
pub(crate) struct Variant(VARIANT);
2246

2347
impl From<Variant> for VARIANT {
@@ -36,22 +60,29 @@ impl Variant {
3660
}
3761
}
3862

39-
impl From<&str> for Variant {
40-
fn from(value: &str) -> Self {
63+
impl From<BSTR> for Variant {
64+
fn from(value: BSTR) -> Self {
4165
Self(value.into())
4266
}
4367
}
4468

45-
impl From<String> for Variant {
46-
fn from(value: String) -> Self {
47-
let value: BSTR = value.into();
48-
Self(value.into())
69+
impl From<WideString> for Variant {
70+
fn from(value: WideString) -> Self {
71+
BSTR::from(value).into()
4972
}
5073
}
5174

52-
impl From<BSTR> for Variant {
53-
fn from(value: BSTR) -> Self {
54-
Self(value.into())
75+
impl From<&str> for Variant {
76+
fn from(value: &str) -> Self {
77+
let mut result = WideString::default();
78+
result.write_str(value).unwrap();
79+
result.into()
80+
}
81+
}
82+
83+
impl From<String> for Variant {
84+
fn from(value: String) -> Self {
85+
value.as_str().into()
5586
}
5687
}
5788

@@ -216,13 +247,15 @@ pub(crate) fn window_title(hwnd: WindowHandle) -> Option<BSTR> {
216247
Some(BSTR::from_wide(&buffer).unwrap())
217248
}
218249

219-
pub(crate) fn toolkit_description(state: &TreeState) -> Option<String> {
250+
pub(crate) fn toolkit_description(state: &TreeState) -> Option<WideString> {
220251
state.toolkit_name().map(|name| {
252+
let mut result = WideString::default();
253+
result.write_str(name).unwrap();
221254
if let Some(version) = state.toolkit_version() {
222-
format!("{} {}", name, version)
223-
} else {
224-
name.to_string()
255+
result.write_char(' ').unwrap();
256+
result.write_str(version).unwrap();
225257
}
258+
result
226259
})
227260
}
228261

0 commit comments

Comments
 (0)