Update Odilia to atspi v0.21.0 #144
Conversation
|
Oooops, looks like I pushed previous commit to main by accident. |
Clippy
Adapt to i32 -> usize changes fmt
TTWNO
force-pushed
the
update-atspi-v0.21.0
branch
from
20d0eec
to
bcc54e1
Compare
There was a problem hiding this comment.
alright, here's a small review on this pr. It's shaping up very nicely indeed, with some extra clarifications in place perhaps and afew code changes as outlined in the following, if necesary, this would be mergeable. Some comments in there could be eronious, if so, it's because I'm not very familiar with this part of the codebase, especially the cache. If that's the case, feel free to mark the conversation as solved without a comment.
- Rename `get_children_caret` to `get_children_from_cursor` - Remove `async_trait` in favour of dep-less solution.
| async fn to_accessible(&self) -> Result<AccessibleProxy, Self::Error>; | ||
| fn to_accessible( | ||
| &self, | ||
| ) -> impl Future<Output = Result<AccessibleProxy, Self::Error>> + Send; |
There was a problem hiding this comment.
you don't have to manually return a future like that, the traits can still be async as long as we bump the msrv. Sure, no longer an object safe trait, but yeah, we may be able to live without that
There was a problem hiding this comment.
No, we need to specify the bounds.
There was a problem hiding this comment.
I mean, in stead of this:
fn to_accessible(
&self,
) -> impl Future<Output = Result<AccessibleProxy, Self::Error>> + Send;you could do something like this:
async fn to_accessible(
&self,
) -> Result<AccessibleProxy, Self::Error>>;I dk about accessible proxy, but essentially the send value shouldn't be required on the future itself, because the compiler boxes it, and futures are send by default unless otherwise noted, so yeah. I dk if that's possible to be done, but it will make for less a code complexity
| Err(e) => return Err(e), | ||
| }; | ||
| if update_state(state, &a11y_prim, event.state, state_value)? { | ||
| tracing::debug!("Updating of the state was not succesful! The item with id {:?} was not found in the cache.", a11y_prim.id); |
There was a problem hiding this comment.
debug is more acceptable, but also the spelling error remains. One can always lower it to trace as well, because cache is initially not consistent anyway, and even debug log can be clogged by this, but since it's not error anymore, this is just a suggestion
|
so then, is this mergeable? I'll merge it, if so |
No description provided.