Skip to content

Conversation

@mwcampbell
Copy link
Contributor

This PR has the following high-level design changes:

  • Removed WeakNode from accesskit_consumer. I found that this approach forced AccessKit to do the full node lookup process whether or not it was necessary for a particular platform adapter method. Also, for some future uses of accesskit_consumer, such as an in-process self-voicing module, something like WeakNode won't be needed at all.
  • In accesskit_consumer, the methods to perform actions are now on Tree rather than Node. This change combined with the previous one means that Node no longer needs a reference (whether direct or indirect) back to Tree.
  • As a result of these changes, we can drop the TreeReader struct. Node only needs a reference to tree::State, which is now exported as TreeState, and Tree::read now returns the RwLock read guard as an opaque object that dereferences to TreeState.
  • Tree::update_and_process_changes now takes an implementation of a trait with multiple functions, rather than a single closure. The functions in this trait correspond to the variants of the old Change enum. This way, the compiler can separately decide whether to inline each function at the place(s) where it's called, instead of having to work with one big closure that does pattern-matching.
  • The consumer API now avoids copying node IDs in function parameters and return values.
  • accesskit_consumer::Node has new methods for returning parent, child, and sibling IDs, without resolving them to full nodes. This is only possible when not checking the node's ignored status.
  • Dropped the FollowingSiblings and PrecedingSiblings iterator implementations; we can just use slices.
  • The remaining iterators are no longer exported separately. These should be internal implementation details.

There are also many changes to the Windows platform adapter to take advantage of these design changes, and some smaller optimizations. Some UIA method implementations no longer need to look up a full node. In a few cases, this PR reverses a previous decision to resolve the node even though we didn't need to, because I now think that the optimization benefit outweighs the theoretical risk of leaking implementation details about how we handle nodes that no longer exist.

@DataTriny
Copy link
Member

I did a first pass and it looks good to me. I'll think a bit more about all the changes before approving though.

@mwcampbell
Copy link
Contributor Author

Now I'm not so sure about some of these changes myself. In particular, I might undo the change to pass and return node IDs by reference. The optimizer can eliminate many copy operations anyway.

@mwcampbell
Copy link
Contributor Author

I rolled back the change to pass node IDs by reference rather than value. I also decided to keep the existing FollowingSiblings and PrecedingSiblings iterators. When I then re-introduced the optimization to navigate between nodes without resolving the target node, I found that the earlier change to pass node IDs by reference rather than value made no difference in optimized builds, whether optimizing for size or speed. Now the diff for this PR is about 700 lines less than it was before.

@mwcampbell mwcampbell force-pushed the refactor-tree-read branch 2 times, most recently from 11d5a22 to 5992d6b Compare October 9, 2022 15:38
@mwcampbell
Copy link
Contributor Author

@DataTriny Please let me know if you're still concerned about any of the changes on this PR. Some of the smaller optimizations in the Windows adapter might not have been the best use of my time, but I think the remaining API changes in the consumer crate are still good.

Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

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

Very good design improvements. Just a cosmetic remark, feel free to ignore it.

@mwcampbell mwcampbell merged commit 765ab74 into main Oct 9, 2022
@mwcampbell mwcampbell deleted the refactor-tree-read branch October 9, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants