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.

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

fix: Optimize dynamic string building #491

Merged
merged 2 commits into from
Dec 15, 2024
Merged
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
83 changes: 67 additions & 16 deletions consumer/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -501,20 +501,37 @@ impl<'a> Node<'a> {
}

pub fn label(&self) -> Option<String> {
let mut result = String::new();
self.write_label(&mut result).unwrap().then_some(result)
}

fn write_label_direct<W: fmt::Write>(&self, mut writer: W) -> Result<bool, fmt::Error> {
if let Some(label) = &self.data().label() {
Some(label.to_string())
writer.write_str(label)?;
Ok(true)
} else {
Ok(false)
}
}

pub fn write_label<W: fmt::Write>(&self, mut writer: W) -> Result<bool, fmt::Error> {
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::<Vec<String>>();
(!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)
}
}

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

pub fn value(&self) -> Option<String> {
let mut result = String::new();
self.write_value(&mut result).unwrap().then_some(result)
}

pub fn write_value<W: fmt::Write>(&self, mut writer: W) -> Result<bool, fmt::Error> {
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)
}
}

Expand Down Expand Up @@ -664,6 +688,33 @@ impl<'a> Node<'a> {
}
}

struct SpacePrefixingWriter<W: fmt::Write> {
inner: W,
need_prefix: bool,
}

impl<W: fmt::Write> SpacePrefixingWriter<W> {
fn write_prefix_if_needed(&mut self) -> fmt::Result {
if self.need_prefix {
self.inner.write_char(' ')?;
self.need_prefix = false;
}
Ok(())
}
}

impl<W: fmt::Write> fmt::Write for SpacePrefixingWriter<W> {
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};
Expand Down
19 changes: 13 additions & 6 deletions consumer/src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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<W: fmt::Write>(&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
Expand Down Expand Up @@ -580,10 +585,12 @@ impl<'a> Range<'a> {
.sum::<usize>();
&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
Expand Down
9 changes: 5 additions & 4 deletions platforms/windows/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
15 changes: 10 additions & 5 deletions platforms/windows/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,15 @@ impl NodeWrapper<'_> {
self.0.role_description()
}

pub(crate) fn name(&self) -> Option<String> {
pub(crate) fn name(&self) -> Option<WideString> {
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<String> {
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 5 additions & 1 deletion platforms/windows/src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,11 @@ impl ITextRangeProvider_Impl for PlatformRange_Impl {
fn GetText(&self, _max_length: i32) -> Result<BSTR> {
// 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<i32> {
Expand Down
61 changes: 47 additions & 14 deletions platforms/windows/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@

use accesskit::Point;
use accesskit_consumer::TreeState;
use std::sync::{Arc, Weak};
use std::{
fmt::{self, Write},
sync::{Arc, Weak},
};
use windows::{
core::*,
Win32::{
Expand All @@ -18,6 +21,27 @@ use windows::{

use crate::window_handle::WindowHandle;

#[derive(Clone, Default, PartialEq, Eq)]
pub(crate) struct WideString(Vec<u16>);

impl 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<WideString> for BSTR {
fn from(value: WideString) -> Self {
Self::from_wide(&value.0).unwrap()
}
}

pub(crate) struct Variant(VARIANT);

impl From<Variant> for VARIANT {
Expand All @@ -36,22 +60,29 @@ impl Variant {
}
}

impl From<&str> for Variant {
fn from(value: &str) -> Self {
impl From<BSTR> for Variant {
fn from(value: BSTR) -> Self {
Self(value.into())
}
}

impl From<String> for Variant {
fn from(value: String) -> Self {
let value: BSTR = value.into();
Self(value.into())
impl From<WideString> for Variant {
fn from(value: WideString) -> Self {
BSTR::from(value).into()
}
}

impl From<BSTR> 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();
result.write_str(value).unwrap();
result.into()
}
}

impl From<String> for Variant {
fn from(value: String) -> Self {
value.as_str().into()
}
}

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

pub(crate) fn toolkit_description(state: &TreeState) -> Option<String> {
pub(crate) fn toolkit_description(state: &TreeState) -> Option<WideString> {
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
})
}

Expand Down
Loading