refactor: Add NonNullObjectRef as part of ObjectRef
#301
Conversation
We'd like to encode in our APIs that some functions should not be
called with a null-reference. This required us to rethink how we
organize our references.
We landed on:
```rust
enum ObjectRef<'o> {
Null,
NonNull(NonNullObjectRef<'o>),
}
enum NonNullObjectRef<'o> {
Borrowed { name: UniqueName<'o>, path: ObjectPath<'o> },
Owned { name: OwnedUniqueName, path: OwnedObjectPath }
}
struct ObjectRefOwned(ObjectRef<'static>)
```
Which offers us an `ObjectRef` and `ObjectRefOwned` that can be
deserialized from any reference, including null-referenes. Good for
receiving references.
`NonNullObjectRef` is particularly well suited where we want to
accept object references.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
- Coverage 75.17% 74.79% -0.38%
==========================================
Files 44 44
Lines 4374 4586 +212
==========================================
+ Hits 3288 3430 +142
- Misses 1086 1156 +70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luukvanderduim
force-pushed
the
non-null-object-refs
branch
from
e2b64ec to
3720c34
Compare
|
The fixes to the Fixing it involves a bit of "forensics". I need to recreate the flow as it was when the example ran when we still had null references that could be made into proxies without problems. Performing calls would error, very different from what we have now. On the bright side the macros I had before are now gone, in favor of mostly this pattern: // Skip null reference and convert to NonNullObjectRef.
let Ok(non_null_ref) = NonNullObjectRef::try_from(app.clone()) else { continue }; |
luukvanderduim
force-pushed
the
non-null-object-refs
branch
2 times, most recently
from
186fc6b to
0d82a2e
Compare
TTWNO
left a comment
TTWNO
left a comment
There was a problem hiding this comment.
Good work so far. Sorry about my limited free time.
| /// In AT-SPI2, objects in the applications' UI object tree are uniquely identified | ||
| /// using an application's bus name and object path. "(so)" | ||
| /// | ||
| /// Emitted by `RemoveAccessible` and `Available` |
|
|
||
| /// Returns the name of the object reference. | ||
| #[must_use] | ||
| #[allow(clippy::match_same_arms)] // Arms differ by lifetime |
There was a problem hiding this comment.
Name the two arms' name variable differently in the two cases, and then you don't need the clippy allow.
|
|
||
| /// Returns the path of the object reference. | ||
| #[must_use] | ||
| #[allow(clippy::match_same_arms)] // Arms differ by lifetime |
| // because PartialEq does not consider it either. | ||
| // | ||
| // This to uphold the contract that if a == b, then a.hash() == b.hash() must hold true. | ||
| impl Hash for NonNullObjectRef<'_> { |
There was a problem hiding this comment.
Good observation that derive-ing would have missed!
There was a problem hiding this comment.
If you haven't already, please add tests to ensure that this remains the case.
| } | ||
|
|
||
| #[test] | ||
| fn non_null_hash_and_object_coherence() { |
There was a problem hiding this comment.
Oh good! There is a test after all :D
atspi-connection/src/p2p.rs
Outdated
| let mut peers = Vec::with_capacity(accessible_applications.len()); | ||
|
|
||
| for app in accessible_applications { | ||
| if app.is_null() { |
There was a problem hiding this comment.
This should probably be:
let app = match app.is_null() {
true => continue,
// etc.
};IMO easier to reason about.
atspi/examples/accessible-counts.rs
Outdated
| for child in root.get_children().await?.iter() { | ||
| let proxy = child.clone().into_accessible_proxy(conn).await?; | ||
| for child in root.get_children().await?.into_iter() { | ||
| let Ok(child) = NonNullObjectRef::try_from(child) else { continue }; |
There was a problem hiding this comment.
This should be the same as the other case; show a unified way to deal with null-refs.
let x = match child.is_null() {
false => continue,
_ => // etc.
};|
|
||
| for app in apps.iter() { | ||
| let proxy = app.clone().into_accessible_proxy(conn).await?; | ||
| for app in apps.into_iter().flat_map(|or| NonNullObjectRef::try_from(or).ok()) { |
There was a problem hiding this comment.
On second thought.... this pattern is very common: continue if null, otherwise try to convert to proxy. That should probably just be a method?
|
If you are looking at other ways to do the "if null continue, else convert to proxy" thing., you could also try |
Example watched mouse events but it doesn't actually use them anywhere
This commit adds local git hooks based on those distributed with [zbus](https://github.com/dbus2/zbus). This adds a "cargo machete" pass to check for unused dependencies to the pre-commit script. Considering `.git/hooks` is not part of the distribution, contributors are advised to configure git to enable these hooks for this repository: ```sh git config core.hooksPath .githooks ``` Thank you @C-Loftus for bringing this up and @jonathanhood for the tidy helper script. Addresses: #298
The event enum had a mistake in the `DBusMatchRule` impl. Fixes: #306
- `GetUri` -> `GetURI` - `Nanchors` -> `NAnchors`
Make `event_stream` to parse signals with prefix "org.a11y.atspi." only. Otherwise it will error on non-atspi signals this connection expects. (`NameOwnerChanged` of "org.freedesktop.DBus") Fixes: #307
and.. remove bad lifetime requirements on return in methods `name()` / `path()`. We no more need to opt out of clippy.
Rustc can only guarantee the static arm if the impl is static too. But this comes with the disadvantage that a second 'static impl cannot have same name methods. I had to reinstate the opt-out for match-on-same-arm pedantic clippy warning. fixes doctests
luukvanderduim
force-pushed
the
non-null-object-refs
branch
from
0d82a2e to
841f994
Compare
3 participants
To encode in the APIs that functions should not be called with a null-reference and to prevent this from happening by accident, we introduce
NonNullObjectRef.To prevent repeating ourselves, this refactor changes the
ObjectReftypes to:Which offers an
ObjectRefandObjectRefOwnedthat can be deserialized from any reference, including null-referenes. Good for receiving references 'bus-side'.NonNullObjectRefis particularly well suited where we want to accept object references.Addresses: #296