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 our Collection story #196

Merged
merged 30 commits into from
Jun 25, 2024
Merged

Fix our Collection story #196

merged 30 commits into from
Jun 25, 2024

Conversation

luukvanderduim
Copy link
Collaborator

@luukvanderduim luukvanderduim commented Jun 22, 2024

ToDo:

  • document collection module level
  • document collection methods
  • document collection method arguments.

The above may take some digging in remote sources or asking around.
Note that 'collection.xml' isn't that helpful here.

- Move from `MatchArgs` to `ObjectMatchRule` (note the "Object" prefix to avoid confusion with `zbus::MatchRule`)
- Remove comment block on "Role fields" which are now a proper type `ObjectMatchRule`
- Place the clippy suppression on the item that violates the rule.

ToDo:
- [  ] Document the methods properly
This probably requires some digging in sources to find out what all arguments actually (were meant to) mean.
-  `Live` is now known as `Politeness`
-  `ObjectMatchRule` not verifiable
`FromIterator` allows us to `.collect()` into an `InterfaceSet`
`Default` impl returns an empty set.
- Remove `MatchArgs`
- Move over `MatchType`, `TreeTraversalType`   to `object_match.rs`
- Introduce `ObjectMatchRule` with builder pattern
- Move over / create tests
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 97.81421% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.22%. Comparing base (eaa55b1) to head (1eee245).
Report is 2 commits behind head on main.

Files Patch % Lines
atspi-common/src/interface.rs 90.62% 3 Missing ⚠️
atspi-common/src/object_match.rs 99.02% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
+ Coverage   86.51%   87.22%   +0.71%     
==========================================
  Files          39       40       +1     
  Lines        3344     3508     +164     
==========================================
+ Hits         2893     3060     +167     
+ Misses        451      448       -3     

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

Missing item fixed.
Only `SortOrder::Canonical` and `SortOrder::ReverseCanonical` seem implemented.

[upstream issue 140](Are all the AtspiCollectionSortOrder types really a thing?)
Seems to be unimplemented as our upstream libatspi peers say it is.
Move `SortOrder` to object_match module
Delete duplicate tests.
@luukvanderduim luukvanderduim marked this pull request as ready for review June 23, 2024 14:32
@TTWNO
Copy link
Member

TTWNO commented Jun 23, 2024

Just took a cursory glance and am very excited! I can review this today, but probably still a couple hours out. Only thing I noticed is that there's link as to why no more then N objects will be returned from certain methods.

Thank you Luuk! This looks like quite the upgrade!

@luukvanderduim
Copy link
Collaborator Author

Just took a cursory glance and am very excited! I can review this today, but probably still a couple hours out. Only thing I noticed is that there's link as to why no more then N objects will be returned from certain methods.

You raise a better point than I would like.

In atk-adaptor of at-spi2-core, we find that

#define MAX_CHILDREN 65536

and both implemented sort-orders check for it.

However, atk-adaptor may not be the only implementer on the bus.
And older versions may not even implement that so there is nothing that guarantees this check.

Accesskit does not implement Collection.

Thank you Luuk! This looks like quite the upgrade!

Thanks!
Joanmarie shared positive Collection experiences in the old calc-issue, this motivated me to look into what we needed to get going.
The ObjectMatchRule and builder I still had lying around so what was left was integration, cleanups and - in this case very important - documentation.

What we need is a few experiments to see if things indeed work as we think it does.

Remove maximum object number from documentation.
Even though atk-adaptor implements the maximum as of present, they may not always have and it may not be the only implementation on the bus either.
atspi-common/src/interface.rs Show resolved Hide resolved
atspi-common/src/interface.rs Show resolved Hide resolved
atspi-common/src/object_match.rs Outdated Show resolved Hide resolved
atspi-common/src/object_match.rs Show resolved Hide resolved

/// Insert a slice of `Interface`s
#[must_use]
pub fn interfaces(mut self, interfaces: &[Interface], mt: MatchType) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Possibly consider interfaces: T where T: IntoIteer<Interface>? I think we've implemented that on InterfaceSet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea - thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

assert_eq!(rule.states, StateSet::new(State::Active));
assert_eq!(
rule.attr,
[("name".to_string(), "value".to_string())].iter().cloned().collect()
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implemented!

atspi-proxies/src/collection.rs Outdated Show resolved Hide resolved
atspi-proxies/src/collection.rs Show resolved Hide resolved
atspi-proxies/src/collection.rs Outdated Show resolved Hide resolved
atspi-proxies/src/collection.rs Show resolved Hide resolved
Only `Canonical` and `ReverseCanonical` are in use in atk-adaptor.
Other or future implementation might use other variants. (but this is unlikely at this point in time.)
Items may be either &Interface or Interface
`IntoIterator` uses `StateSet`s own iter, which returned `impl Iterator<Item = State>`,

The `IntoIterator` implementation does not accept the opaque type, it needs the specific type to coincide with its own type, so the return type on `StateSet::iter` is adjusted to enumflags2::Iter<State>`.
atk-adaptor does not offer this method, but another maybe might.
Added information in docs.
@luukvanderduim luukvanderduim merged commit f619312 into main Jun 25, 2024
13 checks passed
@luukvanderduim luukvanderduim deleted the atspi-collection branch June 25, 2024 20:24
@luukvanderduim luukvanderduim self-assigned this Jun 25, 2024
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.

2 participants