Clientside Collection traversals

[C-Loftus]

As mentioned in #299 it is useful to have client-side traversal helpers since Collection is often unimplemented by many apps.

This PR begins this work. I am submitting it here for review so we can see if I am on the same page. I will PR into this PR as a stack.

PR Structure

I have structured this to begin with an explicit TraversalHelper struct. This helps to encapsulate behavior and store variables like the atspi connection so that they don't need to be passed in the functions. I think this is cleanest so the function definitions stay the same.

I may add a few new atspi errors that are specific to clientside traversals like timeouts or max depth.

Roadmap

• get_active_descendant
• get_matches
• get_matches_from
• get_matches_to
• Unified Collection trait with fallback behavior
  • will adapt some of Luuk's general structure here: Helper functions for resuable tree traversals? #299 (comment)
  • i.e. query without needing to know if it was supported clientside or serverside
  • this will be done by wrapping the Collection trait once I have CollectionClientside fully implemented
• Add tests

[codecov]

Codecov Report

❌ Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.57%. Comparing base (f486755) to head (99f95f6).

Files with missing lines	Patch %	Lines
atspi-proxies/src/traversal_helper.rs	0.00%	34 Missing ⚠️
atspi-common/src/error.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #315      +/-   ##
==========================================
- Coverage   75.17%   74.57%   -0.60%     
==========================================
  Files          44       45       +1     
  Lines        4374     4409      +35     
==========================================
  Hits         3288     3288              
- Misses       1086     1121      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[luukvanderduim]

Love the tidiness: the added docs and supplied example.

Considering this method does not match the signature of Collection's get_active_descendant, I wonder if you already know how to make it fit into the 'hybrid'?

The VecDeque is a way to preserve the "vertical order" / sibling adjacency? Nice find!
There may be an opportunity to save space and compute by maybe grouping the siblings because the loop seems to continue if the first child that exceeds the level is found and there may be many siblings before you find the first that is of lower level.. - will you ever find one with a lower level given how these are inserted?

[luukvanderduim]

Before it gets lost, here is the branch I created for you last week that outlines what could be the abstracted Collection.
https://github.com/odilia-app/atspi/compare/hybrid-collection

Keep up the good work!

[C-Loftus]

Thanks for your comments Luuk, will make relevant changes and think about that optimization once I make a bit more progress on this.

Considering this method does not match the signature of Collection's get_active_descendant, I wonder if you already know how to make it fit into the 'hybrid'?

Yeah this is a good point and something I am still thinking on. Correct me if I am wrong, I thought I needed to deviate from the signature and return an error with more context than a zbus error, to handle the case in which a timeout occurs

As of right now, I think I will a pattern similar to your code. Namely, by using a wrapper struct and having it check if the Collection proxy works and falling back to the clientside traversal if not. Will play around with the API a bit to see what is most natural.

[TTWNO]

@C-Loftus if possible, move to a local (starting with odilia-app/ instead of C-Loftus/) branch—you should have permission now. It makes it easier at the reviewing stage.

[luukvanderduim]

Yeah this is a good point and something I am still thinking on. Correct me if I am wrong, I thought I needed to deviate from the signature and return an error with more context than a zbus error, to handle the case in which a timeout occurs

With respect to the timeout, I think your added AtspiError is fine. There could be discussion I have missed or overlooked..
Maybe there is even an existing zbus error we could use as zbus errors pertain to the bus and AtspiError encloses those, could be a great fit, but I haven't looked.

I just re-read the discussion and it seems @TTWNO and I may have somewhat differing north-stars..
Tait suggested something to extend AccessibleProxy, a TraversalExt extension of Accessible objects - and he also dreamt about caching for efficiency. He alludes to something better than simple DFS, but I would not know what.

Then I suggested abstracting over Collection and Accessible proxies such that we could use the Collection API on any object. Reviving a hybrid approach we had quite a while ago.

One can certainly be a building block for the other.
In the sense that Collection's methods require traversing, but I would urge to look at how things are supposed to work to ensure your primitives are generic enough.
Good starting points are:

• the suggestions at the bottom of our related discussion.
• The types, comments and links in atspi-common/src/object_match.rs
• atspi-proxies/src/collection.rs

Note that some things in object_match can be a bit awkward, such as the String uses I have there in argument positions of eg. attributes or the abstraction over StateSet as IntoIterator<Item = Borrow<State>, I know I wrote that but I forgot why and we're probably better of simplifying that. Otherwise you'll get the gist.
I have an outstanding self-assigned issue to clean up and improve.

As of right now, I think I will a pattern similar to your code. Namely, by using a wrapper struct and having it check if the Collection proxy works and falling back to the clientside traversal if not. Will play around with the API a bit to see what is most natural.

Great!

[C-Loftus]

Tait suggested something to extend AccessibleProxy, a TraversalExt extension of Accessible objects - and he also dreamt about caching for efficiency. He alludes to something better than simple DFS, but I would not know what.

Regarding caching, I think that will be useful, but I think we should get a baseline first to get a sense of the API and the performance. In my mind that would be a followup PR since I think it would be rather complicated.

I did explore having a trait like TraversalExt but I felt it obscured the fact that the traversal is a custom helper and not actually something provided by ATSPI at the protocol level. (And also given the fact that the function signatures are different)

I like having a separate struct since it clearly distinguishes that this code is a custom helper. However, happy to chat further if you disagree.

[TTWNO]

Yeah let's not add the burden of the second hardest problem in computer science to this PR. We will get traversal working first, and then move on to caching at some (unspecified) future date.

[TTWNO]

Glad to see progress coming anew to this section of code. Thanks, Colton.

[C-Loftus]

Glad to see progress coming anew to this section of code. Thanks, Colton.

Sure thing!

FYI my plan is to continuously push to this PR branch so everything is backed up and its clear my progress on things. I think you can mute to prevent notifications on this sort of thing until I formally request a review, but if you prefer I do a different workflow let me know!

[TTWNO]

Noted. Thanks.

[C-Loftus]

Hi @TTWNO , @luukvanderduim I have 2 questions for you both:

1. Is it ok to skip implementing traverse and SortOrder for my clientside traversal or do you prefer these are implemented in this PR? If we do want to implement these I might need to take some more time to dig into things / message you on matrix for clarity.

   • I have to say both of these are a bit confusing to me. I'm happy to defer whatever is best, but I'm just not sure the investment is worth the time.
   • If such arguments aren't ever supported application-side, it would be impossible to have a wrapper that does a fallback with the same args clientside vs application-side.

2. I honestly have similar thoughts about get_matches_from and get_matches_to. I have no issue implementing these, but I am wondering if these would actually be used anywhere. Do you think these should be implemented all in the same large PR?

---

As context:

1. traverse is commented out with:

traverse - Not supported by the known implementation (atk-collection).

2. all SortOrders but Canonical is stated to not be supported in the native traversals. Example:

	/// Flow sort order
	///
	/// Unimplemented
	Flow,

[TTWNO]

I am fine with splitting this up into smaller PRs :)

But yes, there are a lot of confusing issues in traversal semantics... as you have seen, haha!