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

atspi: Accessible::get_state won't work #24

Closed
DataTriny opened this issue Oct 9, 2022 · 5 comments · Fixed by #25
Closed

atspi: Accessible::get_state won't work #24

DataTriny opened this issue Oct 9, 2022 · 5 comments · Fixed by #25

Comments

@DataTriny
Copy link
Contributor

The current implementation of Accessible::get_state can't work because the return type is more complex than the signature implies.

AT-SPI state set is an 8 bytes bitflag that is splitted in half and passed as a two-element 32-bit integers array through D-Bus.

I have a working implementation of this data structure on my AccessKit AT-SPI adapter branch. I would happily add it here but it currently requires two dependencies new to this project:

  • enumflags2: for representing the bitflag,
  • strum: to convert a state into its string representation for raising state changed events.

Of course, strum isn't really needed if you only care about the client side of AT-SPI, and even on the server-side you could still write the strings manually, but this is inconvenient. So that's why I don't directly open a PR here.

What do you guys think? Is it OK to bring these two dependencies? Would you prefer having a strum feature flag? Are you even interested in having the atspi crate useful for building AT-SPI providers?

@TTWNO
Copy link
Member

TTWNO commented Oct 9, 2022

We do have a bitflags dependency in another one of our related crates in the Odilia workspace already (common), so the bitflags dep isn't that big of a deal.

I can't seem to figure out what strum is for. In general I'd tend to lean towatds using serde. Is there any reason the (de)serialization couldn't be done with serde?

What a strange method of sending the bitflag data! Why not just have a (u32, u32) signature. Maybe it's obvious and I just can't see it.

We are interested in having the ability for atspi to be used as both a client and a server, yes. And we will work to make sure this crate can do that.

@DataTriny
Copy link
Contributor Author

@TTWNO

I used strum because the object state changed event requires a string and an int. The string must represent the name of the state being changed, and the int indicates whether the state is on or off. So strum was a convenient way of converting the enum variants into their kebab-case equivalent. For instance, if you want to say that an object just got a tooltip, you'd send this kind of event:

EventBody {
    kind: "has-tooltip", // name of state
    detail1: 1, // state is now set to true
    detail2: 0,
    any_data: 0,
    properties: ...
}

You can't use (u32, u32) because zbus would interpret this as a struct, and we absolutely want an array. Now, I will totally agree that they should have used an UINT64 (t) D-Bus data type to pass this state set, but that's unfortunately not the case...

@DataTriny
Copy link
Contributor Author

DataTriny commented Oct 9, 2022

Thinking more about this: you could have the EventBody struct be generic over a type that is Serialize + Deserialize and change the kind field to be of this type. This way you could have the StateType enum annotated with the #[serde(rename_all)] attribute and not need strum at all.

I can open a PR for this if this sounds reasonable to you.

@albertotirla
Copy link
Member

Thinking more about this: you could have the EventBody struct be generic over a type that is Serialize + Deserialize and change the kind field to be of this type. This way you could have the StateType enum annotated with the #[serde(rename_all)] attribute and not need strum at all.

I can open a PR for this if this sounds reasonable to you.

To my knowledge, this is how we handle events in the odilia crate, we have an event type and body, kinda like you described. So sure, feel free to open a pull request, it will certainly be useful when we add the ability to read status of focused objects, as well as when atspi is being used as a server side library. Btw, what else should be done so that our atspi implementation can be used on the server side as well? As we said in other occasions, we care about making a linux screenreader and that's our top priority. However, even if such a thing doesn't happen for some reason, at least we produced reusable components, so that our code can probably power other applications and do some good somewhere else.

@TTWNO
Copy link
Member

TTWNO commented Oct 10, 2022

I agree with @albertotirla , this would be a welcome PR!

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 a pull request may close this issue.

3 participants